From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 91AEBA04B3; Fri, 20 Dec 2019 22:00:59 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A6C261F1C; Fri, 20 Dec 2019 22:00:58 +0100 (CET) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by dpdk.org (Postfix) with ESMTP id CC7971252 for ; Fri, 20 Dec 2019 22:00:57 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 23887222E6; Fri, 20 Dec 2019 16:00:56 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Fri, 20 Dec 2019 16:00:56 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=fIPBKPb0JCNU3pmAoRLFUXfg7smF8xi4pn7ueJ74MsI=; b=iK5QYodeA2/W sYjbNqyylI5CxV+PzoG9kHe0pNcbCvkPGuBgDBZFmBwIsTCoOQ6ZQw5REsp9zYIF 43J/TovfsMrhjIe1qL5GWNvYmL2M7XVHSvx74mGJJKfzFwHJEDD1SR9Mea8+vDv2 Ys5h+F1Gxzo4Pi752Lc0kSyYIVLTG0g= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=fIPBKPb0JCNU3pmAoRLFUXfg7smF8xi4pn7ueJ74M sI=; b=q6PGC7juf3HR1uBMZ8aMmLGf9zgxNG/WDEvph/BTRfNn5o0bcsj28sfic bJCIPdp8LnAwW8i0+gwRkDgtVyab+loJvc9wPEAweM6RQDBkbFVJS25InSCX3Rz3 dwjHKmgZ9Iq3/E3cQk3+HB5uXVbgWFMd2+2cFEgHXACt0/jyea7PArn7HN5SHQuF b9N7xXxy+zpmtvwnld+EVGhbvBkTgHLYmogeqTUXiHaoyPbmPRzH6pWdHSEmP4D1 6/fuL+Rl3+5GHRn7xMB309q6b6wgBMEmJ60Sp8iMzWDkMkAYMh6ifrh6NHhORXah Zc0Jzh5iHb0NP8Ta3w89gXNz5wMYA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrvddufedgudegudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ffohhmrghinhepshhhsgdrtghipdguphgukhdrohhrghenucfkphepjeejrddufeegrddv tdefrddukeegnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrg hlohhnrdhnvghtnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id BCCAD80066; Fri, 20 Dec 2019 16:00:53 -0500 (EST) From: Thomas Monjalon To: "Kinsella, Ray" Cc: "Richardson, Bruce" , David Marchand , "dev@dpdk.org" , "Laatz, Kevin" , "aconole@redhat.com" , "nhorman@tuxdriver.com" , Michael Santana , "Mcnamara, John" , "Kovacevic, Marko" Date: Fri, 20 Dec 2019 22:00:51 +0100 Message-ID: <3489488.buKiBKRxXp@xps> In-Reply-To: References: <20191220152058.10739-1-david.marchand@redhat.com> <59AF69C657FD0841A61C55336867B5B07EEAB0AE@IRSMSX103.ger.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH] add ABI checks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 20/12/2019 17:20, Kinsella, Ray: > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Friday 20 December 2019 15:32 > > To: David Marchand ; dev@dpdk.org > > Cc: thomas@monjalon.net; Laatz, Kevin ; > > aconole@redhat.com; nhorman@tuxdriver.com; Michael Santana > > ; Mcnamara, John ; > > Kovacevic, Marko ; Kinsella, Ray > > > > Subject: RE: [PATCH] add ABI checks > > > > > > > > > -----Original Message----- > > > From: David Marchand > > > Sent: Friday, December 20, 2019 3:21 PM > > > To: dev@dpdk.org > > > Cc: thomas@monjalon.net; Richardson, Bruce > > > ; Laatz, Kevin ; > > > aconole@redhat.com; nhorman@tuxdriver.com; Michael Santana > > > ; Mcnamara, John > > ; > > > Kovacevic, Marko > > > 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 > > > --- > > > .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 > > > () # > > > 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.