Re: [dpdk-dev] [PATCH v2] ABI: Add abi checking utility

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> 2015-02-02 13:18, Neil Horman:
> > There was a request for an abi validation utiltyfor the ongoing ABI stability
> > work.  As it turns out there is a abi compliance checker in development that
> > seems to be under active development and provides fairly detailed ABI compliance
> > reports.  Its not yet intellegent enough to understand symbol versioning, but it
> > does provide the ability to identify symbols which have changed between
> > releases, along with details of the change, and offers develoeprs the
> > opportunity to identify which symbols then need versioning and validation for a
> > given update via manaul testing.
> 
> There's a lot of typos in this text. Please check.
> 
Three.  Theres 3 typos.  But sure, I'll fix them.

><snip>
> > +
> > +usage() {
> > +	echo "$0 <TAG1> <TAG2> <TARGET>"
> > +}
> > +
> > +log() {
> > +	local level=$1
> 
> level is not used later?
> 
Not yet, but you'll note all the log calls start with a log level to add
filtering.  I'd rather leave this here as it doesn't hurt anything and
effectively documents the paramter.

><snip>
> > +	shift
> > +	echo "$*"
> > +}
> > +
> > +validate_tags() {
> > +	git tag -l | grep -q "$TAG1"
> > +	if [ $? -ne 0 ]
> > +	then
> > +		echo "$TAG1 is invalid"
> > +		return
> > +	fi
> > +	git tag -l | grep -q "$TAG2"
> > +	if [ $? -ne 0 ]
> > +	then
> > +		echo "$TAG2 is invalid"
> > +		return
> > +	fi
> > +}
> > +
> > +validate_args() {
> > +	if [ -z "$TAG1" ]
> > +	then
> > +		echo "Must Specify TAG1"
> > +		return
> > +	fi
> > +	if [ -z "$TAG2" ]
> > +	then
> > +		echo "Must Specify TAG2"
> > +		return
> > +	fi
> > +	if [ -z "$TARGET" ]
> > +	then
> > +		echo "Must Specify a build target"
> > +	fi
> > +}
> > +
> > +
> > +cleanup_and_exit() {
> > +	rm -rf $ABI_DIR
> > +	exit $1
> > +}
> 
> This function could be automatically invoked with trap.
> 
Yes, I can add that.

> > +###########################################
> > +#START
> > +############################################
> > +
> > +#Save the current branch
> > +CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
> 
> Will it work when not on any branch?
> 
No it won't, and I honestly wasn't that worried about it, as people
don't/shouldn't make changes in detached head state.  I can add a check to
ensure you're on a branch though.

> > +
> > +if [ -n "$VERBOSE" ]
> > +then
> > +	export VERBOSE=/dev/stdout
> > +else
> > +	export VERBOSE=/dev/null
> > +fi
> > +
> > +# Validate that we have all the arguments we need
> > +res=$(validate_args)
> > +if [ -n "$res" ]
> > +then
> > +	echo $res
> 
> Should be redirected to stderr >&2
> 
Why? this is eactly what I intended.  All the other messages from log are
directed to stdout, so should this be.

> > +	usage
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +# Make sure our tags exist
> > +res=$(validate_tags)
> > +if [ -n "$res" ]
> > +then
> > +	echo $res
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +ABICHECK=`which abi-compliance-checker 2>/dev/null`
> 
> Why not using the $() form like above?
> 
I don't honestly recall, but I do remember fighting trying to get output from
that format for some reason, and just left this as it was, as it wasn't
particularly relevant.

> I guess this is the tool:
> 	http://ispras.linuxbase.org/index.php/ABI_compliance_checker
> 
Correct.

> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "Cant find abi-compliance-checker utility"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +ABIDUMP=`which abi-dumper 2>/dev/null`
> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "Cant find abi-dumper utility"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +log "INFO" "We're going to check and make sure that applications built"
> > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> > +log "INFO" "against DPDK DSOs built from tag $TAG2."
> > +log "INFO" ""
> > +
> > +# Check to make sure we have a clean tree
> > +git status | grep -q clean
> > +if [ $? -ne 0 ]
> > +then
> 
> You may compact in one line:
> if git status | grep -q clean ; then
> 
I explicitly do execution and error checking on separate lines as I think its
more clear.  You'll find this style consistent in the script.

> > +	log "WARN" "Working directory not clean, aborting"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +if [ ! -d ./.git ]
> > +then
> > +	log "WARN" "You must be in the root of the dpdk git tree"
> > +	log "WARN" "You are in $PWD"
> > +	cleanup_and_exit 1
> > +fi
> 
> Why not cd $(dirname $0)/.. instead of returning an error?
> 
Why would that help in finding the base of the git tree.  Theres no guarantee
that you are in a subdirectory of a git tree.  I suppose we can try it
recursively until we hit /, but it seems just as easy and clear to tell the user
whats needed.

> > +log "INFO" "Checking out version $TAG1 of the dpdk"
> > +# Move to the old version of the tree
> > +git checkout $TAG1
> > +
> > +# Make sure we configure SHARED libraries
> > +# Also turn off IGB and KNI as those require kernel headers to build
> > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" 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
> 
> Why not tuning configuration after make config in .config file?
> 
Because this way we save a reconfig (from a developer viewpoint), you should run
make config again after changing configs, and so this way you save doing that.

> > +export EXTRA_CFLAGS=-g
> > +export EXTRA_LDFLAGS=-g
> 
> A comment is required (needed for abi-dumper?)
> 
Sure.

> > +# 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 O=$TARGET > $VERBOSE 2>&1
> 
> It would more efficient with a customizable -j option
> 
I'm sure it would, I'll look at that in future enhancement. 

> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "THE BUILD FAILED.  ABORTING"
> > +	cleanup_and_exit 1
> > +fi
> > +
> > +# Move to the lib directory
> > +cd $TARGET/lib
> > +log "INFO" "COLLECTING ABI INFORMATION FOR $TAG1"
> > +for i in `ls *.so`
> 
> I think ls is useless.
> 
Um, I don't?  Not sure what you're getting at here.

> > +do
> > +	$ABIDUMP $i -o $ABI_DIR/$i-ABI-0.dump -lver $TAG1
> > +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 $TAG2
> > +
> > +export EXTRA_CFLAGS=-g
> > +export EXTRA_LDFLAGS=-g
> > +
> > +# Make sure we configure SHARED libraries
> > +# Also turn off IGB and KNI as those require kernel headers to build
> > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" 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
> > +
> > +# 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 O=$TARGET > $VERBOSE 2>&1
> > +
> > +if [ $? -ne 0 ]
> > +then
> > +	log "INFO" "THE BUILD FAILED.  ABORTING"
> > +	cleanup_and_exit 1
> > +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 $TAG2
> > +done
> > +cd ../..
> > +
> > +# Start comparison of ABI dumps
> > +for i in `ls $ABI_DIR/*-1.dump`
> 
> Why ls?
> 
Because it preforms the needed action for what I want to do here. Not sure what
you're proposing

> > +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
> 
> Do we need to do a visual check? I didn't try yet.
> 
Yes, it generates an html report of all the symbols exported in a build and
compares them with the alternate version.  That needs manual review.

> > +done
> > +
> > +git reset --hard
> > +git checkout $CURRENT_BRANCH
> > +log "INFO" "ABI CHECK COMPLETE.  REPORTS ARE IN compat_report directory"
> > +cleanup_and_exit 0
> 
> So you compare the ABI dumps.
> Do we also need to run an app from TAG2 with libs from TAG1?
> 
I started down that path, but its not really that helpful, as all it will do is
refuse to run if there is a symbol missing from a later version.  While that
might be helpful, its no where near as through as the full report from the
compliance checker.

The bottom line is that real ABI compliance requires a developer to be aware of
the changes going into the code and how they affect binary layout. A simple
"does it still work" test isn't sufficient.

Neil
> Thanks Neil
>