[dpdk-dev] [PATCH] add ABI checks
Burakov, Anatoly
anatoly.burakov at intel.com
Wed Jan 15 14:07:27 CET 2020
On 06-Jan-20 1:17 PM, Aaron Conole wrote:
> Thomas Monjalon <thomas at monjalon.net> writes:
>
>> 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.
>
> It's more error prone I think. There are issues with archived versions
> of ABI dumps, or bad content delivery networks. Working from the git
> repository is *guaranteed* to generate a correct ABI dump. Anything
> else allows your CDN / other to break.
I can't speak for everyone else, but normally when i'm working on a DPDK
codebase, there's a whole bunch of stuff that isn't built by default,
and i normally never bother. So, while working from git repo is
guaranteed to generate a correct ABI dump, it is *not* guaranteed to
cover everything, unless the developer takes steps to do otherwise.
As far as i understand, the plan is to make ABI checks part of the
automation - so the effort of *having* these reference ABI dumps is
already spent anyway, might as well make them accessible to whoever
develops DPDK too. And if CDN/connectivity fails - well, so be it,
either fall back to manual process, or let the automation handle it.
>
>>>> 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.
>
> It's important to remember that (as far as I'm aware) the vast majority
> of DPDK users don't make many code changes or even compile it. And
> those who do typically don't even run the unit tests. I think it's good
> to document a process for people to follow, but such process shouldn't
> assume that developers can't run basic git commands or execute scripts.
>
>>>> 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.
>
> +1 to this. It's good for the robot's travis build to validate the ABI for
> patches, since it will alert us anyway.
>
>
--
Thanks,
Anatoly
More information about the dev
mailing list