[dpdk-dev] [PATCH] add ABI checks
Thomas Monjalon
thomas at monjalon.net
Fri Dec 20 22:00:51 CET 2019
20/12/2019 17:20, Kinsella, Ray:
> > -----Original Message-----
> > From: Richardson, Bruce <bruce.richardson at intel.com>
> > Sent: Friday 20 December 2019 15:32
> > To: David Marchand <david.marchand at redhat.com>; dev at dpdk.org
> > Cc: thomas at monjalon.net; Laatz, Kevin <kevin.laatz at intel.com>;
> > aconole at redhat.com; nhorman at tuxdriver.com; Michael Santana
> > <maicolgabriel at hotmail.com>; Mcnamara, John <john.mcnamara at intel.com>;
> > Kovacevic, Marko <marko.kovacevic at intel.com>; Kinsella, Ray
> > <ray.kinsella at intel.com>
> > Subject: RE: [PATCH] add ABI checks
> >
> >
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand at redhat.com>
> > > Sent: Friday, December 20, 2019 3:21 PM
> > > To: dev at dpdk.org
> > > Cc: thomas at monjalon.net; Richardson, Bruce
> > > <bruce.richardson at intel.com>; Laatz, Kevin <kevin.laatz at intel.com>;
> > > aconole at redhat.com; nhorman at tuxdriver.com; Michael Santana
> > > <maicolgabriel at hotmail.com>; Mcnamara, John
> > <john.mcnamara at intel.com>;
> > > Kovacevic, Marko <marko.kovacevic at intel.com>
> > > Subject: [PATCH] add ABI checks
> > >
> > > Starting from Kevin and Bruce idea of using libabigail, here is an
> > > alternate approach to implement ABI checks.
> > >
> > > By default, those checks are disabled and enabling them requires a
> > > manual step that generates the ABI dumps on a reference version for a
> > > set of configurations.
> > >
> > > Those checks are enabled in the CI by default for the default meson
> > > options on x86 and aarch64 so that proposed patches are validated.
> > > A cache of the ABI is stored in travis jobs.
> > > Checks can be only informational by setting ABI_CHECKS_WARN_ONLY when
> > > breaking the ABI in a future release.
> > >
> > > For advanced developers and maintainers, the contributing guide
> > > details the higher level scripts that are quite close to the existing
> > > devtools scripts.
> > >
> > > Signed-off-by: David Marchand <david.marchand at redhat.com>
> > > ---
> > > .ci/linux-build.sh | 43 +++++++++++++++++++++++++++
> > > .travis.yml | 20 +++++++++++--
> > > devtools/check-abi-dump.sh | 46
> > +++++++++++++++++++++++++++++
> > > devtools/check-abi-reference.sh | 27 +++++++++++++++++
> > > devtools/dpdk.abignore | 2 ++
> > > devtools/gen-abi-dump.sh | 29 ++++++++++++++++++
> > > devtools/gen-abi-reference.sh | 24 +++++++++++++++
> > > devtools/test-build.sh | 13 ++++++--
> > > devtools/test-meson-builds.sh | 6 ++++
> > > doc/guides/contributing/patches.rst | 25 ++++++++++++++++
> > > 10 files changed, 230 insertions(+), 5 deletions(-) create mode
> > > 100755 devtools/check-abi-dump.sh create mode 100755
> > > devtools/check-abi- reference.sh create mode 100644
> > > devtools/dpdk.abignore create mode
> > > 100755 devtools/gen-abi-dump.sh create mode 100755 devtools/gen-abi-
> > > reference.sh
> > >
> > > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh index
> > > ccc3a7ccd..345dba264 100755
> > > --- a/.ci/linux-build.sh
> > > +++ b/.ci/linux-build.sh
> > > @@ -30,8 +30,51 @@ fi
> > >
> > > OPTS="$OPTS --default-library=$DEF_LIB"
> > > meson build --werror -Dexamples=all $OPTS
> > > +
> > > +if [ "$ABI_CHECKS" = "1" ]; then
> > > + git remote add ref ${REF_GIT_REPO:-https://dpdk.org/git/dpdk}
> > > + git fetch --tags ref ${REF_GIT_BRANCH:-master}
> > > +
> > > + head=$(git describe --all)
> > > + tag=$(git describe --abbrev=0)
> > > +
> > > + if [ "$(cat reference/VERSION 2>/dev/null)" != "$tag" ]; then
> > > + rm -rf reference
> > > + fi
> > > +
> > > + if [ ! -d reference ]; then
> > > + gen_abi_dump=$(mktemp -t gen-abi-dump-XXX.sh)
> > > + cp -a devtools/gen-abi-dump.sh $gen_abi_dump
> > > +
> > > + git checkout -qf $tag
> > > + ninja -C build
> > > + $gen_abi_dump build reference
> > > +
> > > + if [ "$AARCH64" != "1" ]; then
> > > + mkdir -p reference/app
> > > + cp -a build/app/dpdk-testpmd reference/app/
> > > +
> > > + export
> > LD_LIBRARY_PATH=$(pwd)/build/lib:$(pwd)/build/drivers
> > > + devtools/test-null.sh reference/app/dpdk-testpmd
> > > + unset LD_LIBRARY_PATH
> > > + fi
> > > + echo $tag > reference/VERSION
> > > +
> > > + git checkout -qf $head
> > > + fi
> > > +fi
> > > +
> > > ninja -C build
> > >
> > > +if [ "$ABI_CHECKS" = "1" ]; then
> > > + devtools/check-abi-dump.sh build reference
> > ${ABI_CHECKS_WARN_ONLY:-}
> > > + if [ "$AARCH64" != "1" ]; then
> > > + export LD_LIBRARY_PATH=$(pwd)/build/lib:$(pwd)/build/drivers
> > > + devtools/test-null.sh reference/app/dpdk-testpmd
> > > + unset LD_LIBRARY_PATH
> > > + fi
> > > +fi
> > > +
> > > if [ "$AARCH64" != "1" ]; then
> > > devtools/test-null.sh
> > > fi
> > > diff --git a/.travis.yml b/.travis.yml index 8f90d06f2..bbb060fa2
> > > 100644
> > > --- a/.travis.yml
> > > +++ b/.travis.yml
> > > @@ -1,5 +1,8 @@
> > > language: c
> > > -cache: ccache
> > > +cache:
> > > + ccache: true
> > > + directories:
> > > + - reference
> > > compiler:
> > > - gcc
> > > - clang
> > > @@ -21,7 +24,7 @@ aarch64_packages: &aarch64_packages
> > >
> > > extra_packages: &extra_packages
> > > - *required_packages
> > > - - [libbsd-dev, libpcap-dev, libcrypto++-dev, libjansson4]
> > > + - [libbsd-dev, libpcap-dev, libcrypto++-dev, libjansson4,
> > > + abigail-tools]
> > >
> > > build_32b_packages: &build_32b_packages
> > > - *required_packages
> > > @@ -59,6 +62,13 @@ matrix:
> > > apt:
> > > packages:
> > > - *aarch64_packages
> > > + - env: DEF_LIB="shared" EXTRA_PACKAGES=1 ABI_CHECKS=1 AARCH64=1
> > > + compiler: gcc
> > > + addons:
> > > + apt:
> > > + packages:
> > > + - *aarch64_packages
> > > + - *extra_packages
> > > - env: DEF_LIB="static" EXTRA_PACKAGES=1
> > > compiler: gcc
> > > addons:
> > > @@ -72,6 +82,12 @@ matrix:
> > > packages:
> > > - *extra_packages
> > > - *doc_packages
> > > + - env: DEF_LIB="shared" EXTRA_PACKAGES=1 ABI_CHECKS=1
> > > + compiler: gcc
> > > + addons:
> > > + apt:
> > > + packages:
> > > + - *extra_packages
> > > - env: DEF_LIB="static" OPTS="-Denable_kmods=false"
> > EXTRA_PACKAGES=1
> > > compiler: gcc
> > > addons:
> > > diff --git a/devtools/check-abi-dump.sh b/devtools/check-abi-dump.sh
> > > new file mode 100755 index 000000000..f48a2ae7e
> > > --- /dev/null
> > > +++ b/devtools/check-abi-dump.sh
> > > @@ -0,0 +1,46 @@
> > > +#!/bin/sh -e
> > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2019 Red
> > Hat,
> > > +Inc.
> > > +
> > > +if [ $# != 2 ] && [ $# != 3 ]; then
> > > + echo "Usage: $0 builddir dumpdir [warnonly]"
> > > + exit 1
> > > +fi
> > > +
> > > +builddir=$1
> > > +dumpdir=$2
> > > +warnonly=${3:-}
> > > +if [ ! -d $builddir ]; then
> > > + echo "Error: build directory '$builddir' does not exist."
> > > + exit 1
> > > +fi
> > > +if [ ! -d $dumpdir ]; then
> > > + echo "Error: dump directory '$dumpdir' does not exist."
> > > + exit 1
> > > +fi
> > > +
> > > +ABIDIFF_OPTIONS="--suppr $(dirname $0)/dpdk.abignore"
> > > +for dump in $(find $dumpdir -name "*.dump"); do
> > > + libname=$(basename $dump)
> > > + libname=${libname%.dump}
> > > + result=
> > > + for f in $(find $builddir -name "$libname.so.*"); do
> > > + if test -L $f || [ "$f" != "${f%%.symbols}" ]; then
> > > + continue
> > > + fi
> > > + result=found
> > > +
> > > + if ! abidiff $ABIDIFF_OPTIONS $dump $f; then
> > > + if [ -z "$warnonly" ]; then
> > > + echo "Error: ABI issue reported for $dump, $f"
> > > + exit 1
> > > + fi
> > > + echo "Warning: ABI issue reported for $dump, $f"
> > > + fi
> > > + break
> > > + done
> > > + if [ "$result" != "found" ]; then
> > > + echo "Error: can't find a library for dump file $dump"
> > > + exit 1
> > > + fi
> > > +done
> > > diff --git a/devtools/check-abi-reference.sh b/devtools/check-abi-
> > > reference.sh new file mode 100755 index 000000000..7addb094e
> > > --- /dev/null
> > > +++ b/devtools/check-abi-reference.sh
> > > @@ -0,0 +1,27 @@
> > > +#!/bin/sh -e
> > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2019 Red
> > Hat,
> > > +Inc.
> > > +
> > > +devtools_dir=$(dirname $(readlink -f $0)) .
> > > +$devtools_dir/load-devel-config
> > > +
> > > +abi_ref_build_dir=${DPDK_ABI_REF_BUILD_DIR:-reference}
> > > +builds_dir=${DPDK_BUILD_TEST_DIR:-.}
> > > +
> > > +for dir in $abi_ref_build_dir/*; do
> > > + if [ "$dir" = "$abi_ref_build_dir" ]; then
> > > + exit 1
> > > + fi
> > > + if [ ! -d $dir/dump ]; then
> > > + echo "Skipping $dir"
> > > + continue
> > > + fi
> > > + target=$(basename $dir)
> > > + if [ -d $builds_dir/$target/install ]; then
> > > + libdir=$builds_dir/$target/install
> > > + else
> > > + libdir=$builds_dir/$target
> > > + fi
> > > + echo "Checking ABI between $libdir and $dir/dump"
> > > + $devtools_dir/check-abi-dump.sh $libdir $dir/dump done
> > > diff --git a/devtools/dpdk.abignore b/devtools/dpdk.abignore new file
> > > mode
> > > 100644 index 000000000..b866b7f26
> > > --- /dev/null
> > > +++ b/devtools/dpdk.abignore
> > > @@ -0,0 +1,2 @@
> > > +[suppress_function]
> > > + symbol_version = EXPERIMENTAL
> > > diff --git a/devtools/gen-abi-dump.sh b/devtools/gen-abi-dump.sh new
> > > file mode 100755 index 000000000..4e38d751f
> > > --- /dev/null
> > > +++ b/devtools/gen-abi-dump.sh
> > > @@ -0,0 +1,29 @@
> > > +#!/bin/sh -e
> > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2019 Red
> > Hat,
> > > +Inc.
> > > +
> > > +if [ $# != 2 ]; then
> > > + echo "Usage: $0 builddir dumpdir"
> > > + exit 1
> > > +fi
> > > +
> > > +builddir=$1
> > > +dumpdir=$2
> > > +if [ ! -d $builddir ]; then
> > > + echo "Error: build directory '$builddir' does not exist."
> > > + exit 1
> > > +fi
> > > +if [ -d $dumpdir ]; then
> > > + echo "Error: dump directory '$dumpdir' already exists."
> > > + exit 1
> > > +fi
> > > +
> > > +mkdir -p $dumpdir
> > > +for f in $(find $builddir -name "*.so.*"); do
> > > + if test -L $f || [ "$f" != "${f%%.symbols}" ]; then
> > > + continue
> > > + fi
> > > +
> > > + libname=$(basename $f)
> > > + abidw --out-file $dumpdir/${libname%.so.*}.dump $f done
> > > diff --git a/devtools/gen-abi-reference.sh
> > > b/devtools/gen-abi-reference.sh new file mode 100755 index
> > > 000000000..f41d7fadc
> > > --- /dev/null
> > > +++ b/devtools/gen-abi-reference.sh
> > > @@ -0,0 +1,24 @@
> > > +#!/bin/sh -e
> > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2019 Red
> > Hat,
> > > +Inc.
> > > +
> > > +devtools_dir=$(dirname $(readlink -f $0)) .
> > > +$devtools_dir/load-devel-config
> > > +
> > > +abi_ref_build_dir=${DPDK_ABI_REF_BUILD_DIR:-reference}
> > > +for dir in $abi_ref_build_dir/*; do
> > > + if [ "$dir" = "$abi_ref_build_dir" ]; then
> > > + exit 1
> > > + fi
> > > + if [ -d $dir/dump ]; then
> > > + echo "Skipping $dir"
> > > + continue
> > > + fi
> > > + if [ -d $dir/install ]; then
> > > + libdir=$dir/install
> > > + else
> > > + libdir=$dir
> > > + fi
> > > + echo "Dumping libraries from $libdir in $dir/dump"
> > > + $devtools_dir/gen-abi-dump.sh $libdir $dir/dump done
> > > diff --git a/devtools/test-build.sh b/devtools/test-build.sh index
> > > 52305fbb8..8cb5b56fb 100755
> > > --- a/devtools/test-build.sh
> > > +++ b/devtools/test-build.sh
> > > @@ -30,7 +30,8 @@ default_path=$PATH
> > > # - LIBSSO_SNOW3G_PATH
> > > # - LIBSSO_KASUMI_PATH
> > > # - LIBSSO_ZUC_PATH
> > > -. $(dirname $(readlink -f $0))/load-devel-config
> > > +devtools_dir=$(dirname $(readlink -f $0)) .
> > > +$devtools_dir/load-devel-config
> > >
> > > print_usage () {
> > > echo "usage: $(basename $0) [-h] [-jX] [-s] [config1 [config2]
> > > ...]]"
> > > @@ -64,6 +65,7 @@ print_help () {
> > > [ -z $MAKE ] && echo "Cannot find make or gmake" && exit 1
> > >
> > > J=$DPDK_MAKE_JOBS
> > > +abi_ref_build_dir=${DPDK_ABI_REF_BUILD_DIR:-reference}
> > > builds_dir=${DPDK_BUILD_TEST_DIR:-.}
> > > short=false
> > > unset verbose
> > > @@ -97,7 +99,7 @@ trap "signal=INT ; trap - INT ; kill -INT $$" INT
> > #
> > > notify result on exit trap on_exit EXIT
> > >
> > > -cd $(dirname $(readlink -f $0))/..
> > > +cd $devtools_dir/..
> > >
> > > reset_env ()
> > > {
> > > @@ -233,7 +235,7 @@ for conf in $configs ; do
> > > # reload config with DPDK_TARGET set
> > > DPDK_TARGET=$target
> > > reset_env
> > > - . $(dirname $(readlink -f $0))/load-devel-config
> > > + . $devtools_dir/load-devel-config
> > >
> > > options=$(echo $conf | sed 's,[^~+]*,,')
> > > dir=$builds_dir/$conf
> > > @@ -246,6 +248,11 @@ for conf in $configs ; do
> > > export RTE_TARGET=$target
> > > rm -rf $dir/install
> > > ${MAKE} install O=$dir DESTDIR=$dir/install prefix=
> > > + if [ -d $abi_ref_build_dir/$conf/dump ]; then
> > > + echo "================== Check ABI $conf"
> > > + $devtools_dir/check-abi-dump.sh $dir/install \
> > > + $abi_ref_build_dir/$conf/dump
> > > + fi
> > > echo "================== Build examples for $conf"
> > > export RTE_SDK=$(readlink -f $dir)/install/share/dpdk
> > > ln -sTf $(pwd)/lib $RTE_SDK/lib # workaround for vm_power_manager
> > > diff --git a/devtools/test-meson-builds.sh
> > > b/devtools/test-meson-builds.sh index 688567714..aaefa38a2 100755
> > > --- a/devtools/test-meson-builds.sh
> > > +++ b/devtools/test-meson-builds.sh
> > > @@ -16,6 +16,7 @@ srcdir=$(dirname $(readlink -f $0))/..
> > >
> > > MESON=${MESON:-meson}
> > > use_shared="--default-library=shared"
> > > +abi_ref_build_dir=${DPDK_ABI_REF_BUILD_DIR:-reference}
> > > builds_dir=${DPDK_BUILD_TEST_DIR:-.}
> > >
> > > if command -v gmake >/dev/null 2>&1 ; then @@ -88,6 +89,11 @@ build
> > > () # <directory> <target compiler> <meson options>
> > > echo "$ninja_cmd -C $builddir"
> > > $ninja_cmd -C $builddir
> > > fi
> > > +
> > > + if [ -d $abi_ref_build_dir/$1/dump ]; then
> > > + $srcdir/devtools/check-abi-dump.sh $builddir
> > > + $abi_ref_build_dir/$1/dump
> > > + fi
> > > }
> > >
> > > if [ "$1" = "-vv" ] ; then
> > > diff --git a/doc/guides/contributing/patches.rst
> > > b/doc/guides/contributing/patches.rst
> > > index 0686450e4..de3dff145 100644
> > > --- a/doc/guides/contributing/patches.rst
> > > +++ b/doc/guides/contributing/patches.rst
> > > @@ -513,6 +513,31 @@ in a single subfolder called "__builds" created
> > > in the current directory.
> > > Setting ``DPDK_BUILD_TEST_DIR`` to an absolute directory path e.g.
> > > ``/tmp`` is also supported.
> > >
> > >
> > > +Checking ABI compatibility
> > > +--------------------------
> > > +
> > > +The first thing is to build reference binaries for the latest
> > release
> > > +your patches are built on top of.
> > > +
> > > +Either you are in a git tree and an easy way to identify this is to
> > run::
> > > +
> > > + git checkout $(git describe --abbrev=0)
> > > +
> > > +Or you use a tarball and you extract the sources in a director of
> > > +your
> > > choice.
> > > +
> > > +Next is building those sources, refer to the previous paragraph.
> > > +You can set ``DPDK_BUILD_TEST_DIR=reference``, so that the builds
> > > +occur in this directory.
> > > +
> > > +Finally, the ABI dump files are generated with the
> > > +``devtools/gen-abi-reference.sh`` script. This script will look for
> > > +builds in the current sub directory ``reference``. But you can set
> > > +the environment variable ``DPDK_ABI_REF_BUILD_DIR`` to a different
> > location.
> > > +
> > > +Once done, you can check your current binaries ABI with this
> > > +reference with the ``devtools/check-abi-reference.sh`` script.
> > > +
> > > +
> > > Sending Patches
> > > ---------------
> > >
> > > --
> > > 2.23.0
> >
> > I still very much dislike forcing the user to generate his own
> > reference version to compare the ABI against. These should be archived
> > and the user should just be able to pull them down via git or http or
> > otherwise. Two reasons for this:
> >
> > 1. Less error prone, since there is no chance of the user having an
> > incorrect build for whatever reason.
> >
> > 2. Less effort for the user than asking them to do extra builds. The
> > more steps the user has to follow, the less likely they are to attempt
> > the process.
> >
> > Regards,
> > /Bruce
> >
>
> +1 ... 100% agree with this.
>
> Many people won't know or understand what the reference is,
> or why they to generate it.
Many people won't run the test at all and will rely on the automatic CI tests.
More information about the dev
mailing list