[dpdk-dev,v4] devtools: rework abi checker script

Message ID 20170920091253.15794-1-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Olivier Matz Sept. 20, 2017, 9:12 a.m. UTC
  The initial version of the script had some limitations:
- cannot work on a non-clean workspace
- environment variables are not documented
- no compilation log in case of failure
- return success even it abi is incompatible

This patch addresses these issues and rework the code.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

v3->v4:
- clarify logs on incompatible abi
- log when an error returned an error
- [really] fix the report path
- log the output of make config in the proper file

v2->v3:
- fix when not launched from dpdk root dir
- use "-Og -Wno-error" instead of "-O0"
- fix typo in commit log

v1->v2:
- use /usr/bin/env to find bash (which is required)
- fix displayed path to html reports
- reword help for -f option


 devtools/validate-abi.sh | 397 ++++++++++++++++++++++++-----------------------
 1 file changed, 205 insertions(+), 192 deletions(-)
  

Comments

Neil Horman Sept. 21, 2017, 3:40 p.m. UTC | #1
On Wed, Sep 20, 2017 at 11:12:53AM +0200, Olivier Matz wrote:
> The initial version of the script had some limitations:
> - cannot work on a non-clean workspace
> - environment variables are not documented
> - no compilation log in case of failure
> - return success even it abi is incompatible
> 
> This patch addresses these issues and rework the code.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> 
> v3->v4:
> - clarify logs on incompatible abi
> - log when an error returned an error
> - [really] fix the report path
> - log the output of make config in the proper file
> 
> v2->v3:
> - fix when not launched from dpdk root dir
> - use "-Og -Wno-error" instead of "-O0"
> - fix typo in commit log
> 
> v1->v2:
> - use /usr/bin/env to find bash (which is required)
> - fix displayed path to html reports
> - reword help for -f option
> 
> 
>  devtools/validate-abi.sh | 397 ++++++++++++++++++++++++-----------------------
>  1 file changed, 205 insertions(+), 192 deletions(-)
> 
This looks better, thank you for the iterations.  One last note: The abi dumper
utility errors out with error code of 12 if a given object has no exported
symbols, and I see a few of those.  You may want to consider catching that
error, logging an appropriate message and skipping the error emit.  That can be
handled later though, as its a corner case.  I'd go with this patch, and then
do a incremental improvement later

Acked-by: Neil Horman <nhorman@tuxdriver.com>

> diff --git a/devtools/validate-abi.sh b/devtools/validate-abi.sh
> index 0accc99b1..8caf43e83 100755
> --- a/devtools/validate-abi.sh
> +++ b/devtools/validate-abi.sh
> @@ -1,7 +1,8 @@
> -#!/bin/sh
> +#!/usr/bin/env bash
>  #   BSD LICENSE
>  #
>  #   Copyright(c) 2015 Neil Horman. All rights reserved.
> +#   Copyright(c) 2017 6WIND S.A.
>  #   All rights reserved.
>  #
>  #   Redistribution and use in source and binary forms, with or without
> @@ -27,236 +28,248 @@
>  #   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>  #   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
> -TAG1=$1
> -TAG2=$2
> -TARGET=$3
> -ABI_DIR=`mktemp -d -p /tmp ABI.XXXXXX`
> +set -e
>  
> -usage() {
> -	echo "$0 <REV1> <REV2> <TARGET>"
> -}
> +abicheck=abi-compliance-checker
> +abidump=abi-dumper
> +default_dst=abi-check
> +default_target=x86_64-native-linuxapp-gcc
>  
> -log() {
> -	local level=$1
> -	shift
> -	echo "$*"
> +# trap on error
> +err_report() {
> +    echo "$0: error at line $1"
>  }
> +trap 'err_report $LINENO' ERR
>  
> -validate_tags() {
> +print_usage () {
> +	cat <<- END_OF_HELP
> +	$(basename $0) [options] <rev1> <rev2>
>  
> -	if [ -z "$HASH1" ]
> -	then
> -		echo "invalid revision: $TAG1"
> -		return
> -	fi
> -	if [ -z "$HASH2" ]
> -	then
> -		echo "invalid revision: $TAG2"
> -		return
> -	fi
> +	This script compares the ABI of 2 git revisions of the current
> +	workspace. The output is a html report and a compilation log.
> +
> +	The objective is to make sure that applications built against
> +	DSOs from the first revision can still run when executed using
> +	the DSOs built from the second revision.
> +
> +	<rev1> and <rev2> are git commit id or tags.
> +
> +	Options:
> +	  -h		show this help
> +	  -j <num>	enable parallel compilation with <num> threads
> +	  -v		show compilation logs on the console
> +	  -d <dir>	change working directory (default is ${default_dst})
> +	  -t <target>	the dpdk target to use (default is ${default_target})
> +	  -f		overwrite existing files in destination directory
> +
> +	The script returns 0 on success, or the value of last failing
> +	call of ${abicheck} (incompatible abi or the tool has run with errors).
> +	The errors returned by ${abidump} are ignored.
> +
> +	END_OF_HELP
>  }
>  
> -validate_args() {
> -	if [ -z "$TAG1" ]
> -	then
> -		echo "Must Specify REV1"
> -		return
> -	fi
> -	if [ -z "$TAG2" ]
> -	then
> -		echo "Must Specify REV2"
> -		return
> -	fi
> -	if [ -z "$TARGET" ]
> -	then
> -		echo "Must Specify a build target"
> +# log in the file, and on stdout if verbose
> +# $1: level string
> +# $2: string to be logged
> +log() {
> +	echo "$1: $2"
> +	if [ "${verbose}" != "true" ]; then
> +		echo "$1: $2" >&3
>  	fi
>  }
>  
> +# launch a command and log it, taking care of surrounding spaces with quotes
> +cmd() {
> +	local i s whitespace ret
> +	s=""
> +	whitespace="[[:space:]]"
> +	for i in "$@"; do
> +		if [[ $i =~ $whitespace ]]; then
> +			i=\"$i\"
> +		fi
> +		if [ -z "$s" ]; then
> +			s="$i"
> +		else
> +			s="$s $i"
> +		fi
> +	done
> +
> +	ret=0
> +	log "CMD" "$s"
> +	"$@" || ret=$?
> +	if [ "$ret" != "0" ]; then
> +		log "CMD" "previous command returned $ret"
> +	fi
> +
> +	return $ret
> +}
>  
> -cleanup_and_exit() {
> -	rm -rf $ABI_DIR
> -	git checkout $CURRENT_BRANCH
> -	exit $1
> +# redirect or copy stderr/stdout to a file
> +# the syntax is unfamiliar, but it makes the rest of the
> +# code easier to read, avoiding the use of pipes
> +set_log_file() {
> +	# save original stdout and stderr in fd 3 and 4
> +	exec 3>&1
> +	exec 4>&2
> +	# create a new fd 5 that send to a file
> +	exec 5> >(cat > $1)
> +	# send stdout and stderr to fd 5
> +	if [ "${verbose}" = "true" ]; then
> +		exec 1> >(tee /dev/fd/5 >&3)
> +		exec 2> >(tee /dev/fd/5 >&4)
> +	else
> +		exec 1>&5
> +		exec 2>&5
> +	fi
>  }
>  
>  # Make sure we configure SHARED libraries
>  # Also turn off IGB and KNI as those require kernel headers to build
>  fixup_config() {
> -	sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> -	sed -i -e"$ a\CONFIG_RTE_NEXT_ABI=n" config/defconfig_$TARGET
> -	sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> -	sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> -	sed -i -e"$ a\CONFIG_RTE_KNI_KMOD=n" config/defconfig_$TARGET
> +	local conf=config/defconfig_$target
> +	cmd sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" $conf
> +	cmd sed -i -e"$ a\CONFIG_RTE_NEXT_ABI=n" $conf
> +	cmd sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" $conf
> +	cmd sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" $conf
> +	cmd sed -i -e"$ a\CONFIG_RTE_KNI_KMOD=n" $conf
>  }
>  
> -###########################################
> -#START
> -############################################
> +# build dpdk for the given tag and dump abi
> +# $1: hash of the revision
> +gen_abi() {
> +	local i
> +
> +	cmd git clone ${dpdkroot} ${dst}/${1}
> +	cmd cd ${dst}/${1}
> +
> +	log "INFO" "Checking out version ${1} of the dpdk"
> +	# Move to the old version of the tree
> +	cmd git checkout ${1}
> +
> +	fixup_config
> +
> +	# Now configure the build
> +	log "INFO" "Configuring DPDK ${1}"
> +	cmd make config T=$target O=$target
> +
> +	# Checking abi compliance relies on using the dwarf information in
> +	# the shared objects. Build with -g to include them.
> +	log "INFO" "Building DPDK ${1}. This might take a moment"
> +	cmd make -j$parallel O=$target V=1 EXTRA_CFLAGS="-g -Og -Wno-error" \
> +	    EXTRA_LDFLAGS="-g" || log "INFO" "The build failed"
> +
> +	# Move to the lib directory
> +	cmd cd ${PWD}/$target/lib
> +	log "INFO" "Collecting ABI information for ${1}"
> +	for i in *.so; do
> +		[ -e "$i" ] || break
> +		cmd $abidump ${i} -o $dst/${1}/${i}.dump -lver ${1} || true
> +		# hack to ignore empty SymbolsInfo section (no public ABI)
> +		if grep -q "'SymbolInfo' => {}," $dst/${1}/${i}.dump \
> +				2> /dev/null; then
> +			log "INFO" "${i} has no public ABI, remove dump file"
> +			cmd rm -f $dst/${1}/${i}.dump
> +		fi
> +	done
> +}
>  
> -#trap on ctrl-c to clean up
> -trap cleanup_and_exit SIGINT
> +verbose=false
> +parallel=1
> +dst=${default_dst}
> +target=${default_target}
> +force=0
> +while getopts j:vd:t:fh ARG ; do
> +	case $ARG in
> +		j ) parallel=$OPTARG ;;
> +		v ) verbose=true ;;
> +		d ) dst=$OPTARG ;;
> +		t ) target=$OPTARG ;;
> +		f ) force=1 ;;
> +		h ) print_usage ; exit 0 ;;
> +		? ) print_usage ; exit 1 ;;
> +	esac
> +done
> +shift $(($OPTIND - 1))
>  
> -if [ -z "$DPDK_MAKE_JOBS" ]
> -then
> -	# This counts the number of cpus on the system
> -	if [ -e /usr/bin/lscpu ]
> -	then
> -		DPDK_MAKE_JOBS=`lscpu -p=cpu | grep -v "#" | wc -l`
> -	else
> -		DPDK_MAKE_JOBS=1
> -	fi
> +if [ $# != 2 ]; then
> +	print_usage
> +	exit 1
>  fi
>  
> -#Save the current branch
> -CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
> +tag1=$1
> +tag2=$2
>  
> -if [ -z "$CURRENT_BRANCH" ]
> -then
> -	CURRENT_BRANCH=`git log --pretty=format:%H HEAD~1..HEAD`
> -fi
> +# convert path to absolute
> +case "${dst}" in
> +	/*) ;;
> +	*) dst=${PWD}/${dst} ;;
> +esac
> +dpdkroot=$(readlink -e $(dirname $0)/..)
>  
> -if [ -n "$VERBOSE" ]
> -then
> -	export VERBOSE=/dev/stdout
> -else
> -	export VERBOSE=/dev/null
> +if [ -e "${dst}" -a "$force" = 0 ]; then
> +	echo "The ${dst} directory is not empty. Remove it, use another"
> +	echo "one (-d <dir>), or force overriding (-f)"
> +	exit 1
>  fi
>  
> -# Validate that we have all the arguments we need
> -res=$(validate_args)
> -if [ -n "$res" ]
> -then
> -	echo $res
> -	usage
> -	cleanup_and_exit 1
> -fi
> +rm -rf ${dst}
> +mkdir -p ${dst}
> +set_log_file ${dst}/abi-check.log
> +log "INFO" "Logs available in ${dst}/abi-check.log"
>  
> -HASH1=$(git show -s --format=%H "$TAG1" -- 2> /dev/null | tail -1)
> -HASH2=$(git show -s --format=%H "$TAG2" -- 2> /dev/null | tail -1)
> +command -v ${abicheck} || log "INFO" "Can't find ${abicheck} utility"
> +command -v ${abidump} || log "INFO" "Can't find ${abidump} utility"
>  
> -# Make sure our tags exist
> -res=$(validate_tags)
> -if [ -n "$res" ]
> -then
> -	echo $res
> -	cleanup_and_exit 1
> -fi
> +hash1=$(git show -s --format=%h "$tag1" -- 2> /dev/null | tail -1)
> +hash2=$(git show -s --format=%h "$tag2" -- 2> /dev/null | tail -1)
>  
>  # Make hashes available in output for non-local reference
> -TAG1="$TAG1 ($HASH1)"
> -TAG2="$TAG2 ($HASH2)"
> -
> -ABICHECK=`which abi-compliance-checker 2>/dev/null`
> -if [ $? -ne 0 ]
> -then
> -	log "INFO" "Can't find abi-compliance-checker utility"
> -	cleanup_and_exit 1
> -fi
> -
> -ABIDUMP=`which abi-dumper 2>/dev/null`
> -if [ $? -ne 0 ]
> -then
> -	log "INFO" "Can't find abi-dumper utility"
> -	cleanup_and_exit 1
> -fi
> +tag1="$tag1 ($hash1)"
> +tag2="$tag2 ($hash2)"
>  
> -log "INFO" "We're going to check and make sure that applications built"
> -log "INFO" "against DPDK DSOs from version $TAG1 will still run when executed"
> -log "INFO" "against DPDK DSOs built from version $TAG2."
> -log "INFO" ""
> -
> -# Check to make sure we have a clean tree
> -git status | grep -q clean
> -if [ $? -ne 0 ]
> -then
> -	log "WARN" "Working directory not clean, aborting"
> -	cleanup_and_exit 1
> +if [ "$hash1" = "$hash2" ]; then
> +	log "ERROR" "$tag1 and $tag2 are the same revisions"
> +	exit 1
>  fi
>  
> -# Move to the root of the git tree
> -cd $(dirname $0)/..
> +cmd mkdir -p ${dst}
>  
> -log "INFO" "Checking out version $TAG1 of the dpdk"
> -# Move to the old version of the tree
> -git checkout $HASH1
> +# dump abi for each revision
> +gen_abi ${hash1}
> +gen_abi ${hash2}
>  
> -fixup_config
> +# compare the abi dumps
> +cmd cd ${dst}
> +ret=0
> +list=""
> +for i in ${hash2}/*.dump; do
> +	name=`basename $i`
> +	libname=${name%.dump}
>  
> -# Checking abi compliance relies on using the dwarf information in
> -# The shared objects.  Thats only included in the DSO's if we build
> -# with -g
> -export EXTRA_CFLAGS="$EXTRA_CFLAGS -g -O0"
> -export EXTRA_LDFLAGS="$EXTRA_LDFLAGS -g"
> -
> -# Now configure the build
> -log "INFO" "Configuring DPDK $TAG1"
> -make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> -
> -log "INFO" "Building DPDK $TAG1. This might take a moment"
> -make -j$DPDK_MAKE_JOBS O=$TARGET > $VERBOSE 2>&1
> -
> -if [ $? -ne 0 ]
> -then
> -	log "INFO" "THE BUILD FAILED.  ABORTING"
> -	cleanup_and_exit 1
> -fi
> +	if [ ! -f ${hash1}/$name ]; then
> +		log "INFO" "$NAME does not exist in $tag1. skipping..."
> +		continue
> +	fi
>  
> -# Move to the lib directory
> -cd $TARGET/lib
> -log "INFO" "COLLECTING ABI INFORMATION FOR $TAG1"
> -for i in `ls *.so`
> -do
> -	$ABIDUMP $i -o $ABI_DIR/$i-ABI-0.dump -lver $HASH1
> +	local_ret=0
> +	cmd $abicheck -l $libname \
> +	    -old ${hash1}/$name -new ${hash2}/$name || local_ret=$?
> +	if [ $local_ret != 0 ]; then
> +		log "NOTICE" "$abicheck returned $local_ret"
> +		ret=$local_ret
> +		list="$list $libname"
> +	fi
>  done
> -cd ../..
> -
> -# Now clean the tree, checkout the second tag, and rebuild
> -git clean -f -d
> -git reset --hard
> -# Move to the new version of the tree
> -log "INFO" "Checking out version $TAG2 of the dpdk"
> -git checkout $HASH2
> -
> -fixup_config
> -
> -# Now configure the build
> -log "INFO" "Configuring DPDK $TAG2"
> -make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
> -
> -log "INFO" "Building DPDK $TAG2. This might take a moment"
> -make -j$DPDK_MAKE_JOBS O=$TARGET > $VERBOSE 2>&1
>  
> -if [ $? -ne 0 ]
> -then
> -	log "INFO" "THE BUILD FAILED.  ABORTING"
> -	cleanup_and_exit 1
> +if [ $ret != 0 ]; then
> +	log "NOTICE" "ABI may be incompatible, check reports/logs for details."
> +	log "NOTICE" "Incompatible list: $list"
> +else
> +	log "NOTICE" "No error detected, ABI is compatible."
>  fi
>  
> -cd $TARGET/lib
> -log "INFO" "COLLECTING ABI INFORMATION FOR $TAG2"
> -for i in `ls *.so`
> -do
> -	$ABIDUMP $i -o $ABI_DIR/$i-ABI-1.dump -lver $HASH2
> -done
> -cd ../..
> -
> -# Start comparison of ABI dumps
> -for i in `ls $ABI_DIR/*-1.dump`
> -do
> -	NEWNAME=`basename $i`
> -	OLDNAME=`basename $i | sed -e"s/1.dump/0.dump/"`
> -	LIBNAME=`basename $i | sed -e"s/-ABI-1.dump//"`
> -
> -	if [ ! -f $ABI_DIR/$OLDNAME ]
> -	then
> -		log "INFO" "$OLDNAME DOES NOT EXIST IN $TAG1. SKIPPING..."
> -	fi
> -
> -	#compare the abi dumps
> -	$ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME
> -done
> +log "INFO" "Logs are in ${dst}/abi-check.log"
> +log "INFO" "HTML reports are in ${dst}/compat_reports directory"
>  
> -git reset --hard
> -log "INFO" "ABI CHECK COMPLETE.  REPORTS ARE IN compat_report directory"
> -cleanup_and_exit 0
> +exit $ret
> -- 
> 2.11.0
> 
>
  
Olivier Matz Sept. 25, 2017, 9:11 a.m. UTC | #2
On Thu, Sep 21, 2017 at 11:40:35AM -0400, Neil Horman wrote:
> On Wed, Sep 20, 2017 at 11:12:53AM +0200, Olivier Matz wrote:
> > The initial version of the script had some limitations:
> > - cannot work on a non-clean workspace
> > - environment variables are not documented
> > - no compilation log in case of failure
> > - return success even it abi is incompatible
> >
> > This patch addresses these issues and rework the code.
> >
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> >
> > v3->v4:
> > - clarify logs on incompatible abi
> > - log when an error returned an error
> > - [really] fix the report path
> > - log the output of make config in the proper file
> >
> > v2->v3:
> > - fix when not launched from dpdk root dir
> > - use "-Og -Wno-error" instead of "-O0"
> > - fix typo in commit log
> >
> > v1->v2:
> > - use /usr/bin/env to find bash (which is required)
> > - fix displayed path to html reports
> > - reword help for -f option
> >
> >
> >  devtools/validate-abi.sh | 397 ++++++++++++++++++++++++-----------------------
> >  1 file changed, 205 insertions(+), 192 deletions(-)
> >
> This looks better, thank you for the iterations.  One last note: The abi dumper
> utility errors out with error code of 12 if a given object has no exported
> symbols, and I see a few of those.  You may want to consider catching that
> error, logging an appropriate message and skipping the error emit.  That can be
> handled later though, as its a corner case.  I'd go with this patch, and then
> do a incremental improvement later

Unfortunately the error code 12 does not exist on my version of abi-dumper
(debian stable, v0.99.16). I'm currently doing this as a workaround:

    cmd $abidump ${i} -o $dst/${1}/${i}.dump -lver ${1} || true
    # hack to ignore empty SymbolsInfo section (no public ABI)
    if grep -q "'SymbolInfo' => {}," $dst/${1}/${i}.dump 2> /dev/null; then
        log "INFO" "${i} has no public ABI, remove dump file"
        cmd rm -f $dst/${1}/${i}.dump
    fi

I tested with the latest abi-dumper version, and I indeed see
these errors in the logs. It seems we don't go inside the 'if'
above with a recent abi-dumper, and the .dump file is not generated.

I can add a check to display the same additional log
"INFO" "${i} has no public ABI, remove dump file" if abi-dumper
returns 12. Something like this:

    ret=0
    cmd $abidump ${i} -o $dst/${1}/${i}.dump -lver ${1} || ret=$?
    # hack to ignore empty SymbolsInfo section (no public ABI)
    if [ ${ret} = 12 ]; then
            log "INFO" "${i} has no public ABI"
    fi
    if grep -q "'SymbolInfo' => {}," $dst/${1}/${i}.dump 2> /dev/null; then
        log "INFO" "${i} has no public ABI, remove dump file"
        cmd rm -f $dst/${1}/${i}.dump
    fi


Olivier
  
Neil Horman Sept. 25, 2017, 11:21 a.m. UTC | #3
On Mon, Sep 25, 2017 at 11:11:20AM +0200, Olivier MATZ wrote:
> On Thu, Sep 21, 2017 at 11:40:35AM -0400, Neil Horman wrote:
> > On Wed, Sep 20, 2017 at 11:12:53AM +0200, Olivier Matz wrote:
> > > The initial version of the script had some limitations:
> > > - cannot work on a non-clean workspace
> > > - environment variables are not documented
> > > - no compilation log in case of failure
> > > - return success even it abi is incompatible
> > >
> > > This patch addresses these issues and rework the code.
> > >
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > ---
> > >
> > > v3->v4:
> > > - clarify logs on incompatible abi
> > > - log when an error returned an error
> > > - [really] fix the report path
> > > - log the output of make config in the proper file
> > >
> > > v2->v3:
> > > - fix when not launched from dpdk root dir
> > > - use "-Og -Wno-error" instead of "-O0"
> > > - fix typo in commit log
> > >
> > > v1->v2:
> > > - use /usr/bin/env to find bash (which is required)
> > > - fix displayed path to html reports
> > > - reword help for -f option
> > >
> > >
> > >  devtools/validate-abi.sh | 397 ++++++++++++++++++++++++-----------------------
> > >  1 file changed, 205 insertions(+), 192 deletions(-)
> > >
> > This looks better, thank you for the iterations.  One last note: The abi dumper
> > utility errors out with error code of 12 if a given object has no exported
> > symbols, and I see a few of those.  You may want to consider catching that
> > error, logging an appropriate message and skipping the error emit.  That can be
> > handled later though, as its a corner case.  I'd go with this patch, and then
> > do a incremental improvement later
> 
> Unfortunately the error code 12 does not exist on my version of abi-dumper
> (debian stable, v0.99.16). I'm currently doing this as a workaround:
> 
>     cmd $abidump ${i} -o $dst/${1}/${i}.dump -lver ${1} || true
>     # hack to ignore empty SymbolsInfo section (no public ABI)
>     if grep -q "'SymbolInfo' => {}," $dst/${1}/${i}.dump 2> /dev/null; then
>         log "INFO" "${i} has no public ABI, remove dump file"
>         cmd rm -f $dst/${1}/${i}.dump
>     fi
> 
> I tested with the latest abi-dumper version, and I indeed see
> these errors in the logs. It seems we don't go inside the 'if'
> above with a recent abi-dumper, and the .dump file is not generated.
> 
> I can add a check to display the same additional log
> "INFO" "${i} has no public ABI, remove dump file" if abi-dumper
> returns 12. Something like this:
> 
>     ret=0
>     cmd $abidump ${i} -o $dst/${1}/${i}.dump -lver ${1} || ret=$?
>     # hack to ignore empty SymbolsInfo section (no public ABI)
>     if [ ${ret} = 12 ]; then
>             log "INFO" "${i} has no public ABI"
>     fi
>     if grep -q "'SymbolInfo' => {}," $dst/${1}/${i}.dump 2> /dev/null; then
>         log "INFO" "${i} has no public ABI, remove dump file"
>         cmd rm -f $dst/${1}/${i}.dump
>     fi
> 
Agreed, that makes sense.
Thanks.
Neil

> 
> Olivier
>
  

Patch

diff --git a/devtools/validate-abi.sh b/devtools/validate-abi.sh
index 0accc99b1..8caf43e83 100755
--- a/devtools/validate-abi.sh
+++ b/devtools/validate-abi.sh
@@ -1,7 +1,8 @@ 
-#!/bin/sh
+#!/usr/bin/env bash
 #   BSD LICENSE
 #
 #   Copyright(c) 2015 Neil Horman. All rights reserved.
+#   Copyright(c) 2017 6WIND S.A.
 #   All rights reserved.
 #
 #   Redistribution and use in source and binary forms, with or without
@@ -27,236 +28,248 @@ 
 #   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 #   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-TAG1=$1
-TAG2=$2
-TARGET=$3
-ABI_DIR=`mktemp -d -p /tmp ABI.XXXXXX`
+set -e
 
-usage() {
-	echo "$0 <REV1> <REV2> <TARGET>"
-}
+abicheck=abi-compliance-checker
+abidump=abi-dumper
+default_dst=abi-check
+default_target=x86_64-native-linuxapp-gcc
 
-log() {
-	local level=$1
-	shift
-	echo "$*"
+# trap on error
+err_report() {
+    echo "$0: error at line $1"
 }
+trap 'err_report $LINENO' ERR
 
-validate_tags() {
+print_usage () {
+	cat <<- END_OF_HELP
+	$(basename $0) [options] <rev1> <rev2>
 
-	if [ -z "$HASH1" ]
-	then
-		echo "invalid revision: $TAG1"
-		return
-	fi
-	if [ -z "$HASH2" ]
-	then
-		echo "invalid revision: $TAG2"
-		return
-	fi
+	This script compares the ABI of 2 git revisions of the current
+	workspace. The output is a html report and a compilation log.
+
+	The objective is to make sure that applications built against
+	DSOs from the first revision can still run when executed using
+	the DSOs built from the second revision.
+
+	<rev1> and <rev2> are git commit id or tags.
+
+	Options:
+	  -h		show this help
+	  -j <num>	enable parallel compilation with <num> threads
+	  -v		show compilation logs on the console
+	  -d <dir>	change working directory (default is ${default_dst})
+	  -t <target>	the dpdk target to use (default is ${default_target})
+	  -f		overwrite existing files in destination directory
+
+	The script returns 0 on success, or the value of last failing
+	call of ${abicheck} (incompatible abi or the tool has run with errors).
+	The errors returned by ${abidump} are ignored.
+
+	END_OF_HELP
 }
 
-validate_args() {
-	if [ -z "$TAG1" ]
-	then
-		echo "Must Specify REV1"
-		return
-	fi
-	if [ -z "$TAG2" ]
-	then
-		echo "Must Specify REV2"
-		return
-	fi
-	if [ -z "$TARGET" ]
-	then
-		echo "Must Specify a build target"
+# log in the file, and on stdout if verbose
+# $1: level string
+# $2: string to be logged
+log() {
+	echo "$1: $2"
+	if [ "${verbose}" != "true" ]; then
+		echo "$1: $2" >&3
 	fi
 }
 
+# launch a command and log it, taking care of surrounding spaces with quotes
+cmd() {
+	local i s whitespace ret
+	s=""
+	whitespace="[[:space:]]"
+	for i in "$@"; do
+		if [[ $i =~ $whitespace ]]; then
+			i=\"$i\"
+		fi
+		if [ -z "$s" ]; then
+			s="$i"
+		else
+			s="$s $i"
+		fi
+	done
+
+	ret=0
+	log "CMD" "$s"
+	"$@" || ret=$?
+	if [ "$ret" != "0" ]; then
+		log "CMD" "previous command returned $ret"
+	fi
+
+	return $ret
+}
 
-cleanup_and_exit() {
-	rm -rf $ABI_DIR
-	git checkout $CURRENT_BRANCH
-	exit $1
+# redirect or copy stderr/stdout to a file
+# the syntax is unfamiliar, but it makes the rest of the
+# code easier to read, avoiding the use of pipes
+set_log_file() {
+	# save original stdout and stderr in fd 3 and 4
+	exec 3>&1
+	exec 4>&2
+	# create a new fd 5 that send to a file
+	exec 5> >(cat > $1)
+	# send stdout and stderr to fd 5
+	if [ "${verbose}" = "true" ]; then
+		exec 1> >(tee /dev/fd/5 >&3)
+		exec 2> >(tee /dev/fd/5 >&4)
+	else
+		exec 1>&5
+		exec 2>&5
+	fi
 }
 
 # Make sure we configure SHARED libraries
 # Also turn off IGB and KNI as those require kernel headers to build
 fixup_config() {
-	sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
-	sed -i -e"$ a\CONFIG_RTE_NEXT_ABI=n" config/defconfig_$TARGET
-	sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
-	sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
-	sed -i -e"$ a\CONFIG_RTE_KNI_KMOD=n" config/defconfig_$TARGET
+	local conf=config/defconfig_$target
+	cmd sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_NEXT_ABI=n" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" $conf
+	cmd sed -i -e"$ a\CONFIG_RTE_KNI_KMOD=n" $conf
 }
 
-###########################################
-#START
-############################################
+# build dpdk for the given tag and dump abi
+# $1: hash of the revision
+gen_abi() {
+	local i
+
+	cmd git clone ${dpdkroot} ${dst}/${1}
+	cmd cd ${dst}/${1}
+
+	log "INFO" "Checking out version ${1} of the dpdk"
+	# Move to the old version of the tree
+	cmd git checkout ${1}
+
+	fixup_config
+
+	# Now configure the build
+	log "INFO" "Configuring DPDK ${1}"
+	cmd make config T=$target O=$target
+
+	# Checking abi compliance relies on using the dwarf information in
+	# the shared objects. Build with -g to include them.
+	log "INFO" "Building DPDK ${1}. This might take a moment"
+	cmd make -j$parallel O=$target V=1 EXTRA_CFLAGS="-g -Og -Wno-error" \
+	    EXTRA_LDFLAGS="-g" || log "INFO" "The build failed"
+
+	# Move to the lib directory
+	cmd cd ${PWD}/$target/lib
+	log "INFO" "Collecting ABI information for ${1}"
+	for i in *.so; do
+		[ -e "$i" ] || break
+		cmd $abidump ${i} -o $dst/${1}/${i}.dump -lver ${1} || true
+		# hack to ignore empty SymbolsInfo section (no public ABI)
+		if grep -q "'SymbolInfo' => {}," $dst/${1}/${i}.dump \
+				2> /dev/null; then
+			log "INFO" "${i} has no public ABI, remove dump file"
+			cmd rm -f $dst/${1}/${i}.dump
+		fi
+	done
+}
 
-#trap on ctrl-c to clean up
-trap cleanup_and_exit SIGINT
+verbose=false
+parallel=1
+dst=${default_dst}
+target=${default_target}
+force=0
+while getopts j:vd:t:fh ARG ; do
+	case $ARG in
+		j ) parallel=$OPTARG ;;
+		v ) verbose=true ;;
+		d ) dst=$OPTARG ;;
+		t ) target=$OPTARG ;;
+		f ) force=1 ;;
+		h ) print_usage ; exit 0 ;;
+		? ) print_usage ; exit 1 ;;
+	esac
+done
+shift $(($OPTIND - 1))
 
-if [ -z "$DPDK_MAKE_JOBS" ]
-then
-	# This counts the number of cpus on the system
-	if [ -e /usr/bin/lscpu ]
-	then
-		DPDK_MAKE_JOBS=`lscpu -p=cpu | grep -v "#" | wc -l`
-	else
-		DPDK_MAKE_JOBS=1
-	fi
+if [ $# != 2 ]; then
+	print_usage
+	exit 1
 fi
 
-#Save the current branch
-CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
+tag1=$1
+tag2=$2
 
-if [ -z "$CURRENT_BRANCH" ]
-then
-	CURRENT_BRANCH=`git log --pretty=format:%H HEAD~1..HEAD`
-fi
+# convert path to absolute
+case "${dst}" in
+	/*) ;;
+	*) dst=${PWD}/${dst} ;;
+esac
+dpdkroot=$(readlink -e $(dirname $0)/..)
 
-if [ -n "$VERBOSE" ]
-then
-	export VERBOSE=/dev/stdout
-else
-	export VERBOSE=/dev/null
+if [ -e "${dst}" -a "$force" = 0 ]; then
+	echo "The ${dst} directory is not empty. Remove it, use another"
+	echo "one (-d <dir>), or force overriding (-f)"
+	exit 1
 fi
 
-# Validate that we have all the arguments we need
-res=$(validate_args)
-if [ -n "$res" ]
-then
-	echo $res
-	usage
-	cleanup_and_exit 1
-fi
+rm -rf ${dst}
+mkdir -p ${dst}
+set_log_file ${dst}/abi-check.log
+log "INFO" "Logs available in ${dst}/abi-check.log"
 
-HASH1=$(git show -s --format=%H "$TAG1" -- 2> /dev/null | tail -1)
-HASH2=$(git show -s --format=%H "$TAG2" -- 2> /dev/null | tail -1)
+command -v ${abicheck} || log "INFO" "Can't find ${abicheck} utility"
+command -v ${abidump} || log "INFO" "Can't find ${abidump} utility"
 
-# Make sure our tags exist
-res=$(validate_tags)
-if [ -n "$res" ]
-then
-	echo $res
-	cleanup_and_exit 1
-fi
+hash1=$(git show -s --format=%h "$tag1" -- 2> /dev/null | tail -1)
+hash2=$(git show -s --format=%h "$tag2" -- 2> /dev/null | tail -1)
 
 # Make hashes available in output for non-local reference
-TAG1="$TAG1 ($HASH1)"
-TAG2="$TAG2 ($HASH2)"
-
-ABICHECK=`which abi-compliance-checker 2>/dev/null`
-if [ $? -ne 0 ]
-then
-	log "INFO" "Can't find abi-compliance-checker utility"
-	cleanup_and_exit 1
-fi
-
-ABIDUMP=`which abi-dumper 2>/dev/null`
-if [ $? -ne 0 ]
-then
-	log "INFO" "Can't find abi-dumper utility"
-	cleanup_and_exit 1
-fi
+tag1="$tag1 ($hash1)"
+tag2="$tag2 ($hash2)"
 
-log "INFO" "We're going to check and make sure that applications built"
-log "INFO" "against DPDK DSOs from version $TAG1 will still run when executed"
-log "INFO" "against DPDK DSOs built from version $TAG2."
-log "INFO" ""
-
-# Check to make sure we have a clean tree
-git status | grep -q clean
-if [ $? -ne 0 ]
-then
-	log "WARN" "Working directory not clean, aborting"
-	cleanup_and_exit 1
+if [ "$hash1" = "$hash2" ]; then
+	log "ERROR" "$tag1 and $tag2 are the same revisions"
+	exit 1
 fi
 
-# Move to the root of the git tree
-cd $(dirname $0)/..
+cmd mkdir -p ${dst}
 
-log "INFO" "Checking out version $TAG1 of the dpdk"
-# Move to the old version of the tree
-git checkout $HASH1
+# dump abi for each revision
+gen_abi ${hash1}
+gen_abi ${hash2}
 
-fixup_config
+# compare the abi dumps
+cmd cd ${dst}
+ret=0
+list=""
+for i in ${hash2}/*.dump; do
+	name=`basename $i`
+	libname=${name%.dump}
 
-# Checking abi compliance relies on using the dwarf information in
-# The shared objects.  Thats only included in the DSO's if we build
-# with -g
-export EXTRA_CFLAGS="$EXTRA_CFLAGS -g -O0"
-export EXTRA_LDFLAGS="$EXTRA_LDFLAGS -g"
-
-# Now configure the build
-log "INFO" "Configuring DPDK $TAG1"
-make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
-
-log "INFO" "Building DPDK $TAG1. This might take a moment"
-make -j$DPDK_MAKE_JOBS O=$TARGET > $VERBOSE 2>&1
-
-if [ $? -ne 0 ]
-then
-	log "INFO" "THE BUILD FAILED.  ABORTING"
-	cleanup_and_exit 1
-fi
+	if [ ! -f ${hash1}/$name ]; then
+		log "INFO" "$NAME does not exist in $tag1. skipping..."
+		continue
+	fi
 
-# Move to the lib directory
-cd $TARGET/lib
-log "INFO" "COLLECTING ABI INFORMATION FOR $TAG1"
-for i in `ls *.so`
-do
-	$ABIDUMP $i -o $ABI_DIR/$i-ABI-0.dump -lver $HASH1
+	local_ret=0
+	cmd $abicheck -l $libname \
+	    -old ${hash1}/$name -new ${hash2}/$name || local_ret=$?
+	if [ $local_ret != 0 ]; then
+		log "NOTICE" "$abicheck returned $local_ret"
+		ret=$local_ret
+		list="$list $libname"
+	fi
 done
-cd ../..
-
-# Now clean the tree, checkout the second tag, and rebuild
-git clean -f -d
-git reset --hard
-# Move to the new version of the tree
-log "INFO" "Checking out version $TAG2 of the dpdk"
-git checkout $HASH2
-
-fixup_config
-
-# Now configure the build
-log "INFO" "Configuring DPDK $TAG2"
-make config T=$TARGET O=$TARGET > $VERBOSE 2>&1
-
-log "INFO" "Building DPDK $TAG2. This might take a moment"
-make -j$DPDK_MAKE_JOBS O=$TARGET > $VERBOSE 2>&1
 
-if [ $? -ne 0 ]
-then
-	log "INFO" "THE BUILD FAILED.  ABORTING"
-	cleanup_and_exit 1
+if [ $ret != 0 ]; then
+	log "NOTICE" "ABI may be incompatible, check reports/logs for details."
+	log "NOTICE" "Incompatible list: $list"
+else
+	log "NOTICE" "No error detected, ABI is compatible."
 fi
 
-cd $TARGET/lib
-log "INFO" "COLLECTING ABI INFORMATION FOR $TAG2"
-for i in `ls *.so`
-do
-	$ABIDUMP $i -o $ABI_DIR/$i-ABI-1.dump -lver $HASH2
-done
-cd ../..
-
-# Start comparison of ABI dumps
-for i in `ls $ABI_DIR/*-1.dump`
-do
-	NEWNAME=`basename $i`
-	OLDNAME=`basename $i | sed -e"s/1.dump/0.dump/"`
-	LIBNAME=`basename $i | sed -e"s/-ABI-1.dump//"`
-
-	if [ ! -f $ABI_DIR/$OLDNAME ]
-	then
-		log "INFO" "$OLDNAME DOES NOT EXIST IN $TAG1. SKIPPING..."
-	fi
-
-	#compare the abi dumps
-	$ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME
-done
+log "INFO" "Logs are in ${dst}/abi-check.log"
+log "INFO" "HTML reports are in ${dst}/compat_reports directory"
 
-git reset --hard
-log "INFO" "ABI CHECK COMPLETE.  REPORTS ARE IN compat_report directory"
-cleanup_and_exit 0
+exit $ret