| Message ID | 20230217202234.32260-2-quic_johmoo@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Validating UAPI backwards compatibility | expand |
On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon wrote: > While the kernel community has been good at maintaining backwards > compatibility with kernel UAPIs, it would be helpful to have a tool > to check if a patch introduces changes that break backwards > compatibility. > > To that end, introduce check-uapi.sh: a simple shell script that > checks for changes to UAPI headers using libabigail. > > libabigail is "a framework which aims at helping developers and > software distributors to spot some ABI-related issues like interface > incompatibility in ELF shared libraries by performing a static > analysis of the ELF binaries at hand." > > The script uses one of libabigail's tools, "abidiff", to compile the > changed header before and after the patch to detect any changes. > > abidiff "compares the ABI of two shared libraries in ELF format. It > emits a meaningful report describing the differences between the two > ABIs." > > Signed-off-by: John Moon <quic_johmoo@quicinc.com> > --- > scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 245 insertions(+) > create mode 100755 scripts/check-uapi.sh Ok, this is very cool, thank you so much for doing this. I know Randy Dunlap was also looking into this previously, so I've cc:ed him and bounced him the original. I tried this out, and at first glance, this felt like it was just "too fast" in that nothing actually was being tested. So I manually added a field to a structure I know would break the abi, and: $ ./scripts/check-uapi.sh !!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!! [C] 'struct usb_ctrlrequest' changed: type size changed from 64 to 72 (in bits) 1 data member insertion: '__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1 3 data member changes: '__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits) '__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits) '__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits) 0/1 UAPI header file changes are backwards compatible UAPI header ABI check failed So it worked! There is a mismatch of different bash coding styles in the document, which isn't a big deal, and one warning produced by the `shellcheck` tool, but that can all be fixed up later. I'll go queue this up now, as a starting point for people to play with, thanks! Also, it would be nice to be able to check if the current tree with changes in it (not checked in, just modified) breaks the abi, without having to go and check the change in. But again, future fixups for people to do! thanks, greg k-h
On Sat, Feb 18, 2023 at 5:23 AM John Moon <quic_johmoo@quicinc.com> wrote: > > While the kernel community has been good at maintaining backwards > compatibility with kernel UAPIs, it would be helpful to have a tool > to check if a patch introduces changes that break backwards > compatibility. > > To that end, introduce check-uapi.sh: a simple shell script that > checks for changes to UAPI headers using libabigail. > > libabigail is "a framework which aims at helping developers and > software distributors to spot some ABI-related issues like interface > incompatibility in ELF shared libraries by performing a static > analysis of the ELF binaries at hand." > > The script uses one of libabigail's tools, "abidiff", to compile the > changed header before and after the patch to detect any changes. > > abidiff "compares the ABI of two shared libraries in ELF format. It > emits a meaningful report describing the differences between the two > ABIs." > > Signed-off-by: John Moon <quic_johmoo@quicinc.com> BTW, the patch is prefixed with 'RESEND', but it contains several diff lines against the previous submission [1]. People would submit this as v2 with changes mentioned under '---'. [1]: https://patchwork.kernel.org/project/linux-kbuild/patch/20230217202234.32260-2-quic_johmoo@quicinc.com/ diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh index 209c6793a4d0..b9cd3a2d7805 100755 --- a/scripts/check-uapi.sh +++ b/scripts/check-uapi.sh @@ -3,6 +3,7 @@ # Script to check a patch for UAPI stability set -o errexit +set -o pipefail print_usage() { name=$(basename "$0") @@ -34,7 +35,7 @@ get_header() { if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then if ! make -C "${KERNEL_SRC}/scripts" unifdef; then - echo 'error - failed to build required dependency "scripts/unifdef"' + errlog 'error - failed to build required dependency "scripts/unifdef"' exit 1 fi fi @@ -111,7 +112,7 @@ check_changed_files() { total=$((passed + failed)) if [ "$total" -eq 0 ]; then - errlog "No changes to UAPI headers detected" + errlog "No changes to UAPI headers detected in most recent commit" else errlog "${passed}/${total} UAPI header file changes are backwards compatible" fi @@ -190,16 +191,15 @@ check_deps() { errlog "error - abidiff not found!" errlog "Please install abigail-tools (version 1.7 or greater)" errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" - exit 2 + exit 1 fi - local -r abidiff_maj=$("$ABIDIFF" --version | cut -d ' ' -f 2 | cut -d '.' -f 1) - local -r abidiff_min=$("$ABIDIFF" --version | cut -d ' ' -f 2 | cut -d '.' -f 1) + read -r abidiff_maj abidiff_min _ < <("$ABIDIFF" --version | cut -d ' ' -f 2 | tr '.' ' ') if [ "$abidiff_maj" -lt 1 ] || ([ "$abidiff_maj" -eq 1 ] && [ "$abidiff_min" -lt 7 ]); then - errlog "error - installed abidiff version too old: $("$ABIDIFF" --version)" + errlog "error - abidiff version too old: $("$ABIDIFF" --version)" errlog "Please install abigail-tools (version 1.7 or greater)" errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" - exit 2 + exit 1 fi } @@ -220,13 +220,18 @@ main() { check_deps tmp_dir=$(mktemp -d) - #trap 'rm -rf $tmp_dir' EXIT + trap 'rm -rf $tmp_dir' EXIT if [ -z "$KERNEL_SRC" ]; then KERNEL_SRC="$(realpath "$(dirname "$0")"/..)" fi export KERNEL_SRC + if ! (cd "$KERNEL_SRC" && git rev-parse --is-inside-work-tree > /dev/null 2>&1); then + errlog "error - this script requires the kernel tree to be initialized with Git" + exit 1 + fi + export ARCH export CC export CROSS_COMPILE
On Sat, Feb 18, 2023 at 5:23 AM John Moon <quic_johmoo@quicinc.com> wrote: > > While the kernel community has been good at maintaining backwards > compatibility with kernel UAPIs, it would be helpful to have a tool > to check if a patch introduces changes that break backwards > compatibility. > > To that end, introduce check-uapi.sh: a simple shell script that > checks for changes to UAPI headers using libabigail. > > libabigail is "a framework which aims at helping developers and > software distributors to spot some ABI-related issues like interface > incompatibility in ELF shared libraries by performing a static > analysis of the ELF binaries at hand." > > The script uses one of libabigail's tools, "abidiff", to compile the > changed header before and after the patch to detect any changes. > > abidiff "compares the ABI of two shared libraries in ELF format. It > emits a meaningful report describing the differences between the two > ABIs." > > Signed-off-by: John Moon <quic_johmoo@quicinc.com> > --- > scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 245 insertions(+) > create mode 100755 scripts/check-uapi.sh > > diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh > new file mode 100755 > index 000000000..b9cd3a2d7 > --- /dev/null > +++ b/scripts/check-uapi.sh > @@ -0,0 +1,245 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0-only > + > +# Script to check a patch for UAPI stability > +set -o errexit > +set -o pipefail > + > +print_usage() { > + name=$(basename "$0") > + cat << EOF > +$name - check for UAPI header stability across Git commits > + > +By default, the script will check to make sure the latest commit did > +not introduce ABI changes (HEAD^1). You can check against additional > +commits/tags with the -r option. > + > +Usage: $name [-r GIT_REF] > + > +Options: > + -r GIT_REF Compare current version of file to GIT_REF (e.g. -r v6.1) > + > +Environmental Args: > + ABIDIFF Custom path to abidiff binary > + ARCH Architecture to build with (e.g. ARCH=arm) ARCH is not used anywhere in this script. > + CC C compiler (default is "gcc") > + CROSS_COMPILE Cross-compiling toochain prefix CROSS_COMPILE is unneeded since the toolchain prefix is a part of CC > +EOF > +} > + > +# Get the file and sanitize it using the headers_install script > +get_header() { > + local -r ref="$1" > + local -r file="$2" > + local -r out="$3" > + > + if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then > + if ! make -C "${KERNEL_SRC}/scripts" unifdef; then I think if ! make -f /dev/null "${KERNEL_SRC}/scripts/unifdef"; then ... clarifies what you are doing here because you are using make's built-in rule, and nothing in scripts/Makefile. I do not understand the reason for using make if you do not use Makefile at all. You are just compiling scripts/unifdef.c directly. > + errlog 'error - failed to build required dependency "scripts/unifdef"' > + exit 1 > + fi > + fi > + > + mkdir -p "$(dirname "$out")" > + ( > + cd "$KERNEL_SRC" > + git show "${ref}:${file}" > "${out}.in" > + scripts/headers_install.sh "${out}.in" "$out" > + ) Unneeded sub-shell fork. git -C "$KERNEL_SRC" show "${ref}:${file}" > "${out}.in" scripts/headers_install.sh "${out}.in" "$out" > +} > + > +# Compile the simple test app > +do_compile() { > + local -r compiler="$1" > + local -r inc_dir="$2" > + local -r header="$3" > + local -r out="$4" > + echo "int main(int argc, char **argv) { return 0; }" | \ bikeshed: 'int main(void) { return 0; }' is enough. > + "$compiler" -c \ You can expand ${CC} here "${CC:-gcc}" -c \ I do not see anywhere else to use ${CC}. Remove the 'compiler' argument. > + -o "$out" \ > + -x c \ > + -O0 \ > + -std=c90 \ > + -fno-eliminate-unused-debug-types \ > + -g \ > + "-I$inc_dir" \ "-I$inc_dir" is meaningless for most cases, unless two UAPI headers are changed in HEAD. In some cases, you cannot even compile the header. Think about this case: include/uapi/linux/foo.h includes <linux/bar.h> linux/bar.h does not exist in this tmp directory. You assume <linux/bar.h> comes from the user's build environment, presumably located under /usr/include/. It does not necessarily new enough to compile include/uapi/linux/foo.h So, this does not work. I believe you need to re-consider the approach. > + -include "$header" \ > + - > +} > + > +# Print to stderr > +errlog() { > + echo "$@" >&2 > +} > + > +# Grab the list of incompatible headers from the usr/include Makefile > +get_no_header_list() { > + { > + cat "${KERNEL_SRC}/usr/include/Makefile" > + # shellcheck disable=SC2016 > + printf '\nall:\n\t@echo $(no-header-test)\n' > + } | make -C "${KERNEL_SRC}/usr/include" -f - --just-print \ > + | grep '^echo' \ > + | cut -d ' ' -f 2- > +} Redundant. get_no_header_list() { { echo 'all: ; @echo $(no-header-test)' cat "${KERNEL_SRC}/usr/include/Makefile" } | make -f - } should be equivalent, but you still cannot exclude include/uapi/asm-generic/*.h, though. > + > +# Check any changed files in this commit for UAPI compatibility > +check_changed_files() { > + refs_to_check=("$@") > + > + local passed=0; > + local failed=0; > + > + while read -r status file; do > + local -r base=${file/uapi\//} The -r option is wrong since 'base' is updated in the second iteration. If this while loop gets two or more input lines, I see the following in the second iteration. ./scripts/check-uapi.sh: line 94: local: base: readonly variable > + > + # Get the current version of the file and put it in the install dir > + get_header "HEAD" "$file" "${tmp_dir}/usr/${base}" Is '/usr' needed? > + > + for ref in "${refs_to_check[@]}"; do > + if ! git rev-parse --verify "$ref" > /dev/null 2>&1; then > + echo "error - invalid ref \"$ref\"" > + exit 1 > + fi > + > + if check_uapi_for_file "$status" "$file" "$ref" "$base"; then > + passed=$((passed + 1)) > + else > + failed=$((failed + 1)) > + fi > + done > + done < <(cd "$KERNEL_SRC" && git show HEAD --name-status --format="" --diff-filter=a -- include/uapi/) Redundant. done < <(git -C "$KERNEL_SRC" show HEAD --name-status --format="" --diff-filter=a -- include/uapi/) Why are you checking only include/uapi/ ? UAPI headers exist in arch/*/include/uapi/ > + > + total=$((passed + failed)) > + if [ "$total" -eq 0 ]; then > + errlog "No changes to UAPI headers detected in most recent commit" > + else > + errlog "${passed}/${total} UAPI header file changes are backwards compatible" > + fi > + > + return "$failed" > +} > + > +# Check UAPI compatibility for a given file > +check_uapi_for_file() { > + local -r status="$1" > + local -r file="$2" > + local -r ref="$3" > + local -r base="$4" > + > + # shellcheck disable=SC2076 > + if [[ " $(get_no_header_list) " =~ " ${base/include\//} " ]]; then > + errlog "$file cannot be tested by this script (see usr/include/Makefile)." > + return 1 > + fi > + > + if [ "$status" = "D" ]; then > + errlog "UAPI header $file was incorrectly removed" > + return 1 > + fi If you look at git history, we sometimes do this. e.g. 1e6b57d6421f0343dd11619612e5ff8930cddf38 > + > + if [ "$status" = "R" ]; then > + errlog "UAPI header $file was incorrectly renamed" > + return 1 > + fi I think this is unneeded if you add --no-renames to 'git show'. I do not see any sense to distinguish removal and rename since it is what git detects from the similarity. > + > + # Get the "previous" verison of the API header and put it in the install dir > + get_header "$ref" "$file" "${tmp_dir}/usr/${base}.pre" Is '/usr' needed? > + > + compare_abi "${CROSS_COMPILE}${CC:-gcc}" "$file" "$base" "$ref" CROSS_COMPILE is unneeded since it is included in ${CC}. > +} > + > +# Perform the A/B compilation and compare output ABI > +compare_abi() { > + local -r compiler="$1" > + local -r file="$2" > + local -r base="$3" > + local -r ref="$4" > + > + pre_bin="${tmp_dir}/pre.bin" > + post_bin="${tmp_dir}/post.bin" > + log="${tmp_dir}/log" > + > + if ! do_compile "$compiler" "${tmp_dir}/usr/include" "${tmp_dir}/usr/${base}.pre" "$pre_bin" 2> "$log"; then > + errlog "Couldn't compile current version of UAPI header $file..." > + cat "$log" >&2 > + return 1 > + fi > + > + if ! do_compile "$compiler" "${tmp_dir}/usr/include" "${tmp_dir}/usr/${base}" "$post_bin" 2> "$log"; then > + errlog "Couldn't compile new version of UAPI header $file..." > + cat "$log" >&2 > + return 1 > + fi > + > + if "$ABIDIFF" --non-reachable-types "$pre_bin" "$post_bin" > "$log"; then > + echo "No ABI differences detected in $file (compared to file at $ref)" > + else > + errlog "!!! ABI differences detected in $file (compared to file at $ref) !!!" > + echo >&2 > + sed -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/ /g' "$log" >&2 > + echo >&2 > + return 1 > + fi > +} > + > +# Make sure we have the tools we need > +check_deps() { > + export ABIDIFF="${ABIDIFF:-abidiff}" > + > + if ! command -v "$ABIDIFF" > /dev/null 2>&1; then > + errlog "error - abidiff not found!" > + errlog "Please install abigail-tools (version 1.7 or greater)" > + errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" > + exit 1 > + fi > + > + read -r abidiff_maj abidiff_min _ < <("$ABIDIFF" --version | cut -d ' ' -f 2 | tr '.' ' ') > + if [ "$abidiff_maj" -lt 1 ] || ([ "$abidiff_maj" -eq 1 ] && [ "$abidiff_min" -lt 7 ]); then > + errlog "error - abidiff version too old: $("$ABIDIFF" --version)" > + errlog "Please install abigail-tools (version 1.7 or greater)" > + errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" > + exit 1 > + fi > +} > + > +main() { > + refs_to_check=( "HEAD^1" ) > + while getopts "hr:" opt; do > + case $opt in > + h) > + print_usage > + exit 0 > + ;; > + r) > + refs_to_check+=( "$OPTARG" ) > + ;; > + esac > + done > + > + check_deps > + > + tmp_dir=$(mktemp -d) > + trap 'rm -rf $tmp_dir' EXIT > + > + if [ -z "$KERNEL_SRC" ]; then > + KERNEL_SRC="$(realpath "$(dirname "$0")"/..)" > + fi > + export KERNEL_SRC Who will use KERNEL_SRC except this script? > + > + if ! (cd "$KERNEL_SRC" && git rev-parse --is-inside-work-tree > /dev/null 2>&1); then > + errlog "error - this script requires the kernel tree to be initialized with Git" > + exit 1 > + fi > + > + export ARCH > + export CC > + export CROSS_COMPILE print_usage() says these three are taken from environment variables. So, they are already exported, aren't they? > + > + if ! check_changed_files "${refs_to_check[@]}"; then > + errlog "UAPI header ABI check failed" > + exit 1 > + fi > +} > + > +main "$@" > -- > 2.17.1 > -- Best Regards Masahiro Yamada
On 2/18/2023 12:17 AM, Greg Kroah-Hartman wrote: > On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon wrote: >> While the kernel community has been good at maintaining backwards >> compatibility with kernel UAPIs, it would be helpful to have a tool >> to check if a patch introduces changes that break backwards >> compatibility. >> >> To that end, introduce check-uapi.sh: a simple shell script that >> checks for changes to UAPI headers using libabigail. >> >> libabigail is "a framework which aims at helping developers and >> software distributors to spot some ABI-related issues like interface >> incompatibility in ELF shared libraries by performing a static >> analysis of the ELF binaries at hand." >> >> The script uses one of libabigail's tools, "abidiff", to compile the >> changed header before and after the patch to detect any changes. >> >> abidiff "compares the ABI of two shared libraries in ELF format. It >> emits a meaningful report describing the differences between the two >> ABIs." >> >> Signed-off-by: John Moon <quic_johmoo@quicinc.com> >> --- >> scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 245 insertions(+) >> create mode 100755 scripts/check-uapi.sh > > Ok, this is very cool, thank you so much for doing this. > > I know Randy Dunlap was also looking into this previously, so I've cc:ed > him and bounced him the original. > Okay, will keep him in the loop! > I tried this out, and at first glance, this felt like it was just "too > fast" in that nothing actually was being tested. So I manually added a > field to a structure I know would break the abi, and: > > $ ./scripts/check-uapi.sh > !!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!! > > [C] 'struct usb_ctrlrequest' changed: > type size changed from 64 to 72 (in bits) > 1 data member insertion: > '__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1 > 3 data member changes: > '__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits) > '__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits) > '__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits) > > 0/1 UAPI header file changes are backwards compatible > UAPI header ABI check failed > > So it worked! > > There is a mismatch of different bash coding styles in the document, which > isn't a big deal, and one warning produced by the `shellcheck` tool, but that > can all be fixed up later. I'll go queue this up now, as a starting point for > people to play with, thanks! > Will resolve all shellcheck warnings in next rev. Not sure about the coding style differences you're referring to - is there a shell script coding style document? > Also, it would be nice to be able to check if the current tree with changes in > it (not checked in, just modified) breaks the abi, without having to go and > check the change in. But again, future fixups for people to do! > Should be straightforward to add. Since I'm making another rev, I'll take care of this as well. > thanks, > > greg k-h
On 2/18/2023 9:25 PM, Masahiro Yamada wrote: > On Sat, Feb 18, 2023 at 5:23 AM John Moon <quic_johmoo@quicinc.com> wrote: >> >> While the kernel community has been good at maintaining backwards >> compatibility with kernel UAPIs, it would be helpful to have a tool >> to check if a patch introduces changes that break backwards >> compatibility. >> >> To that end, introduce check-uapi.sh: a simple shell script that >> checks for changes to UAPI headers using libabigail. >> >> libabigail is "a framework which aims at helping developers and >> software distributors to spot some ABI-related issues like interface >> incompatibility in ELF shared libraries by performing a static >> analysis of the ELF binaries at hand." >> >> The script uses one of libabigail's tools, "abidiff", to compile the >> changed header before and after the patch to detect any changes. >> >> abidiff "compares the ABI of two shared libraries in ELF format. It >> emits a meaningful report describing the differences between the two >> ABIs." >> >> Signed-off-by: John Moon <quic_johmoo@quicinc.com> > > > > > BTW, the patch is prefixed with 'RESEND', but it contains > several diff lines against the previous submission [1]. > > People would submit this as v2 with changes mentioned under '---'. > Sorry about that, I'm still learning the etiquette. :) I will mark the patch rev as v2 after addressing your comments. > > [1]: https://patchwork.kernel.org/project/linux-kbuild/patch/20230217202234.32260-2-quic_johmoo@quicinc.com/ > > > > > > > diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh > index 209c6793a4d0..b9cd3a2d7805 100755 > --- a/scripts/check-uapi.sh > +++ b/scripts/check-uapi.sh > @@ -3,6 +3,7 @@ > > # Script to check a patch for UAPI stability > set -o errexit > +set -o pipefail > > print_usage() { > name=$(basename "$0") > @@ -34,7 +35,7 @@ get_header() { > > if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then > if ! make -C "${KERNEL_SRC}/scripts" unifdef; then > - echo 'error - failed to build required > dependency "scripts/unifdef"' > + errlog 'error - failed to build required > dependency "scripts/unifdef"' > exit 1 > fi > fi > @@ -111,7 +112,7 @@ check_changed_files() { > > total=$((passed + failed)) > if [ "$total" -eq 0 ]; then > - errlog "No changes to UAPI headers detected" > + errlog "No changes to UAPI headers detected in most > recent commit" > else > errlog "${passed}/${total} UAPI header file changes > are backwards compatible" > fi > @@ -190,16 +191,15 @@ check_deps() { > errlog "error - abidiff not found!" > errlog "Please install abigail-tools (version 1.7 or greater)" > errlog "See: > https://sourceware.org/libabigail/manual/libabigail-overview.html" > - exit 2 > + exit 1 > fi > > - local -r abidiff_maj=$("$ABIDIFF" --version | cut -d ' ' -f 2 > | cut -d '.' -f 1) > - local -r abidiff_min=$("$ABIDIFF" --version | cut -d ' ' -f 2 > | cut -d '.' -f 1) > + read -r abidiff_maj abidiff_min _ < <("$ABIDIFF" --version | > cut -d ' ' -f 2 | tr '.' ' ') > if [ "$abidiff_maj" -lt 1 ] || ([ "$abidiff_maj" -eq 1 ] && [ > "$abidiff_min" -lt 7 ]); then > - errlog "error - installed abidiff version too old: > $("$ABIDIFF" --version)" > + errlog "error - abidiff version too old: $("$ABIDIFF" > --version)" > errlog "Please install abigail-tools (version 1.7 or greater)" > errlog "See: > https://sourceware.org/libabigail/manual/libabigail-overview.html" > - exit 2 > + exit 1 > fi > } > > @@ -220,13 +220,18 @@ main() { > check_deps > > tmp_dir=$(mktemp -d) > - #trap 'rm -rf $tmp_dir' EXIT > + trap 'rm -rf $tmp_dir' EXIT > > if [ -z "$KERNEL_SRC" ]; then > KERNEL_SRC="$(realpath "$(dirname "$0")"/..)" > fi > export KERNEL_SRC > > + if ! (cd "$KERNEL_SRC" && git rev-parse --is-inside-work-tree >> /dev/null 2>&1); then > + errlog "error - this script requires the kernel tree > to be initialized with Git" > + exit 1 > + fi > + > export ARCH > export CC > export CROSS_COMPILE > > > > > >
On 2/19/2023 1:36 AM, Masahiro Yamada wrote: > On Sat, Feb 18, 2023 at 5:23 AM John Moon <quic_johmoo@quicinc.com> wrote: >> >> While the kernel community has been good at maintaining backwards >> compatibility with kernel UAPIs, it would be helpful to have a tool >> to check if a patch introduces changes that break backwards >> compatibility. >> >> To that end, introduce check-uapi.sh: a simple shell script that >> checks for changes to UAPI headers using libabigail. >> >> libabigail is "a framework which aims at helping developers and >> software distributors to spot some ABI-related issues like interface >> incompatibility in ELF shared libraries by performing a static >> analysis of the ELF binaries at hand." >> >> The script uses one of libabigail's tools, "abidiff", to compile the >> changed header before and after the patch to detect any changes. >> >> abidiff "compares the ABI of two shared libraries in ELF format. It >> emits a meaningful report describing the differences between the two >> ABIs." >> >> Signed-off-by: John Moon <quic_johmoo@quicinc.com> >> --- >> scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 245 insertions(+) >> create mode 100755 scripts/check-uapi.sh >> >> diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh >> new file mode 100755 >> index 000000000..b9cd3a2d7 >> --- /dev/null >> +++ b/scripts/check-uapi.sh >> @@ -0,0 +1,245 @@ >> +#!/bin/bash >> +# SPDX-License-Identifier: GPL-2.0-only >> + >> +# Script to check a patch for UAPI stability >> +set -o errexit >> +set -o pipefail >> + >> +print_usage() { >> + name=$(basename "$0") >> + cat << EOF >> +$name - check for UAPI header stability across Git commits >> + >> +By default, the script will check to make sure the latest commit did >> +not introduce ABI changes (HEAD^1). You can check against additional >> +commits/tags with the -r option. >> + >> +Usage: $name [-r GIT_REF] >> + >> +Options: >> + -r GIT_REF Compare current version of file to GIT_REF (e.g. -r v6.1) >> + >> +Environmental Args: >> + ABIDIFF Custom path to abidiff binary >> + ARCH Architecture to build with (e.g. ARCH=arm) > > > ARCH is not used anywhere in this script. > > Right, yes. We'll end up using it in the next rev for installing arch-specific headers. > > > >> + CC C compiler (default is "gcc") >> + CROSS_COMPILE Cross-compiling toochain prefix > > CROSS_COMPILE is unneeded since the toolchain prefix > is a part of CC > Good point, will remove. > > >> +EOF >> +} >> + >> +# Get the file and sanitize it using the headers_install script >> +get_header() { >> + local -r ref="$1" >> + local -r file="$2" >> + local -r out="$3" >> + >> + if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then >> + if ! make -C "${KERNEL_SRC}/scripts" unifdef; then > > > > I think > > if ! make -f /dev/null "${KERNEL_SRC}/scripts/unifdef"; then > > ... clarifies what you are doing here > because you are using make's built-in rule, > and nothing in scripts/Makefile. > > I do not understand the reason for using make > if you do not use Makefile at all. > > You are just compiling scripts/unifdef.c directly. > > Agreed - will update. > > > > > >> + errlog 'error - failed to build required dependency "scripts/unifdef"' >> + exit 1 >> + fi >> + fi >> + >> + mkdir -p "$(dirname "$out")" >> + ( >> + cd "$KERNEL_SRC" >> + git show "${ref}:${file}" > "${out}.in" >> + scripts/headers_install.sh "${out}.in" "$out" >> + ) > > > Unneeded sub-shell fork. > > git -C "$KERNEL_SRC" show "${ref}:${file}" > "${out}.in" > scripts/headers_install.sh "${out}.in" "$out" > > Yes, will fix. > > > > > > > > > >> +} >> + >> +# Compile the simple test app >> +do_compile() { >> + local -r compiler="$1" >> + local -r inc_dir="$2" >> + local -r header="$3" >> + local -r out="$4" >> + echo "int main(int argc, char **argv) { return 0; }" | \ > > bikeshed: 'int main(void) { return 0; }' is enough. > Agreed. > >> + "$compiler" -c \ > > > You can expand ${CC} here > > "${CC:-gcc}" -c \ > > > I do not see anywhere else to use ${CC}. > Remove the 'compiler' argument. > > Yes, will do. > > >> + -o "$out" \ >> + -x c \ >> + -O0 \ >> + -std=c90 \ >> + -fno-eliminate-unused-debug-types \ >> + -g \ >> + "-I$inc_dir" \ > > > "-I$inc_dir" is meaningless for most cases, unless > two UAPI headers are changed in HEAD. > > > In some cases, you cannot even compile the header. > > Think about this case: > include/uapi/linux/foo.h includes <linux/bar.h> > > linux/bar.h does not exist in this tmp directory. > > You assume <linux/bar.h> comes from the user's build environment, > presumably located under /usr/include/. > > It does not necessarily new enough to compile > include/uapi/linux/foo.h > > So, this does not work. > I believe you need to re-consider the approach. > > This is a great point. I think a good approach here would be to install all the headers into $inc_dir so that cross-header dependencies like this use the correct versions from the current kernel. I'll try this approach in the next rev. > > > >> + -include "$header" \ >> + - >> +} >> + >> +# Print to stderr >> +errlog() { >> + echo "$@" >&2 >> +} >> + >> +# Grab the list of incompatible headers from the usr/include Makefile >> +get_no_header_list() { >> + { >> + cat "${KERNEL_SRC}/usr/include/Makefile" >> + # shellcheck disable=SC2016 >> + printf '\nall:\n\t@echo $(no-header-test)\n' >> + } | make -C "${KERNEL_SRC}/usr/include" -f - --just-print \ >> + | grep '^echo' \ >> + | cut -d ' ' -f 2- >> +} > > > Redundant. > > > > get_no_header_list() { > { > echo 'all: ; @echo $(no-header-test)' > cat "${KERNEL_SRC}/usr/include/Makefile" > } | make -f - > } > > > should be equivalent, but you still cannot exclude > include/uapi/asm-generic/*.h, though. > > Yes, that's a better implementation. I'll also make sure that the asm-generic headers get included in the next rev. > > > >> + >> +# Check any changed files in this commit for UAPI compatibility >> +check_changed_files() { >> + refs_to_check=("$@") >> + >> + local passed=0; >> + local failed=0; >> + >> + while read -r status file; do >> + local -r base=${file/uapi\//} > > > The -r option is wrong since 'base' is updated > in the second iteration. > > > If this while loop gets two or more input lines, > I see the following in the second iteration. > > > ./scripts/check-uapi.sh: line 94: local: base: readonly variable > > Nice catch, didn't find that in my testing. I will fix. > > > > >> + >> + # Get the current version of the file and put it in the install dir >> + get_header "HEAD" "$file" "${tmp_dir}/usr/${base}" > > > > Is '/usr' needed? > Not strictly. Was just using it for "sanitization" in the temp dir. Will reconsider. > > >> + >> + for ref in "${refs_to_check[@]}"; do >> + if ! git rev-parse --verify "$ref" > /dev/null 2>&1; then >> + echo "error - invalid ref \"$ref\"" >> + exit 1 >> + fi >> + >> + if check_uapi_for_file "$status" "$file" "$ref" "$base"; then >> + passed=$((passed + 1)) >> + else >> + failed=$((failed + 1)) >> + fi >> + done >> + done < <(cd "$KERNEL_SRC" && git show HEAD --name-status --format="" --diff-filter=a -- include/uapi/) > > Redundant. > > done < <(git -C "$KERNEL_SRC" show HEAD --name-status --format="" > --diff-filter=a -- include/uapi/) > > > > > Why are you checking only include/uapi/ ? > UAPI headers exist in arch/*/include/uapi/ > > I hadn't considered those arch-specific headers. Will include them in the next rev. > > > > > > > >> + >> + total=$((passed + failed)) >> + if [ "$total" -eq 0 ]; then >> + errlog "No changes to UAPI headers detected in most recent commit" >> + else >> + errlog "${passed}/${total} UAPI header file changes are backwards compatible" >> + fi >> + >> + return "$failed" >> +} >> + >> +# Check UAPI compatibility for a given file >> +check_uapi_for_file() { >> + local -r status="$1" >> + local -r file="$2" >> + local -r ref="$3" >> + local -r base="$4" >> + >> + # shellcheck disable=SC2076 >> + if [[ " $(get_no_header_list) " =~ " ${base/include\//} " ]]; then >> + errlog "$file cannot be tested by this script (see usr/include/Makefile)." >> + return 1 >> + fi >> + >> + if [ "$status" = "D" ]; then >> + errlog "UAPI header $file was incorrectly removed" >> + return 1 >> + fi > > If you look at git history, we sometimes do this. > > e.g. > > 1e6b57d6421f0343dd11619612e5ff8930cddf38 > > > I think the script should still fail in this case. If someone decides to ignore it and still remove the header file, that's fine. In general though, I'd expect a flag on my CI build if a supported UAPI header file was removed. > > > > >> + >> + if [ "$status" = "R" ]; then >> + errlog "UAPI header $file was incorrectly renamed" >> + return 1 >> + fi > > > > I think this is unneeded if you add --no-renames to 'git show'. > > I do not see any sense to distinguish removal and rename > since it is what git detects from the similarity. > > > Agreed, there's not really a distinction here. I'll add --no-renames and only check for removals. > > > > > > > >> + >> + # Get the "previous" verison of the API header and put it in the install dir >> + get_header "$ref" "$file" "${tmp_dir}/usr/${base}.pre" > > Is '/usr' needed? > > >> + >> + compare_abi "${CROSS_COMPILE}${CC:-gcc}" "$file" "$base" "$ref" > > CROSS_COMPILE is unneeded since it is included in ${CC}. > Agreed, will remove. > > > > > >> +} >> + >> +# Perform the A/B compilation and compare output ABI >> +compare_abi() { >> + local -r compiler="$1" >> + local -r file="$2" >> + local -r base="$3" >> + local -r ref="$4" >> + >> + pre_bin="${tmp_dir}/pre.bin" >> + post_bin="${tmp_dir}/post.bin" >> + log="${tmp_dir}/log" >> + >> + if ! do_compile "$compiler" "${tmp_dir}/usr/include" "${tmp_dir}/usr/${base}.pre" "$pre_bin" 2> "$log"; then >> + errlog "Couldn't compile current version of UAPI header $file..." >> + cat "$log" >&2 >> + return 1 >> + fi >> + >> + if ! do_compile "$compiler" "${tmp_dir}/usr/include" "${tmp_dir}/usr/${base}" "$post_bin" 2> "$log"; then >> + errlog "Couldn't compile new version of UAPI header $file..." >> + cat "$log" >&2 >> + return 1 >> + fi >> + >> + if "$ABIDIFF" --non-reachable-types "$pre_bin" "$post_bin" > "$log"; then >> + echo "No ABI differences detected in $file (compared to file at $ref)" >> + else >> + errlog "!!! ABI differences detected in $file (compared to file at $ref) !!!" >> + echo >&2 >> + sed -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/ /g' "$log" >&2 >> + echo >&2 >> + return 1 >> + fi >> +} >> + >> +# Make sure we have the tools we need >> +check_deps() { >> + export ABIDIFF="${ABIDIFF:-abidiff}" >> + >> + if ! command -v "$ABIDIFF" > /dev/null 2>&1; then >> + errlog "error - abidiff not found!" >> + errlog "Please install abigail-tools (version 1.7 or greater)" >> + errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" >> + exit 1 >> + fi >> + >> + read -r abidiff_maj abidiff_min _ < <("$ABIDIFF" --version | cut -d ' ' -f 2 | tr '.' ' ') >> + if [ "$abidiff_maj" -lt 1 ] || ([ "$abidiff_maj" -eq 1 ] && [ "$abidiff_min" -lt 7 ]); then >> + errlog "error - abidiff version too old: $("$ABIDIFF" --version)" >> + errlog "Please install abigail-tools (version 1.7 or greater)" >> + errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" >> + exit 1 >> + fi >> +} >> + >> +main() { >> + refs_to_check=( "HEAD^1" ) >> + while getopts "hr:" opt; do >> + case $opt in >> + h) >> + print_usage >> + exit 0 >> + ;; >> + r) >> + refs_to_check+=( "$OPTARG" ) >> + ;; >> + esac >> + done >> + >> + check_deps >> + >> + tmp_dir=$(mktemp -d) >> + trap 'rm -rf $tmp_dir' EXIT >> + >> + if [ -z "$KERNEL_SRC" ]; then >> + KERNEL_SRC="$(realpath "$(dirname "$0")"/..)" >> + fi >> + export KERNEL_SRC > > > Who will use KERNEL_SRC except this script? > > > > > > >> + >> + if ! (cd "$KERNEL_SRC" && git rev-parse --is-inside-work-tree > /dev/null 2>&1); then >> + errlog "error - this script requires the kernel tree to be initialized with Git" >> + exit 1 >> + fi >> + >> + export ARCH >> + export CC >> + export CROSS_COMPILE > > print_usage() says these three are taken from > environment variables. > > So, they are already exported, aren't they? > > > Right, don't need to re-export these. Will remove. > > > >> + >> + if ! check_changed_files "${refs_to_check[@]}"; then >> + errlog "UAPI header ABI check failed" >> + exit 1 >> + fi >> +} >> + >> +main "$@" >> -- >> 2.17.1 >> > -- > Best Regards > Masahiro Yamada Thanks so much for the thorough review. It is greatly appreciated! - John
diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh new file mode 100755 index 000000000..b9cd3a2d7 --- /dev/null +++ b/scripts/check-uapi.sh @@ -0,0 +1,245 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0-only + +# Script to check a patch for UAPI stability +set -o errexit +set -o pipefail + +print_usage() { + name=$(basename "$0") + cat << EOF +$name - check for UAPI header stability across Git commits + +By default, the script will check to make sure the latest commit did +not introduce ABI changes (HEAD^1). You can check against additional +commits/tags with the -r option. + +Usage: $name [-r GIT_REF] + +Options: + -r GIT_REF Compare current version of file to GIT_REF (e.g. -r v6.1) + +Environmental Args: + ABIDIFF Custom path to abidiff binary + ARCH Architecture to build with (e.g. ARCH=arm) + CC C compiler (default is "gcc") + CROSS_COMPILE Cross-compiling toochain prefix +EOF +} + +# Get the file and sanitize it using the headers_install script +get_header() { + local -r ref="$1" + local -r file="$2" + local -r out="$3" + + if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then + if ! make -C "${KERNEL_SRC}/scripts" unifdef; then + errlog 'error - failed to build required dependency "scripts/unifdef"' + exit 1 + fi + fi + + mkdir -p "$(dirname "$out")" + ( + cd "$KERNEL_SRC" + git show "${ref}:${file}" > "${out}.in" + scripts/headers_install.sh "${out}.in" "$out" + ) +} + +# Compile the simple test app +do_compile() { + local -r compiler="$1" + local -r inc_dir="$2" + local -r header="$3" + local -r out="$4" + echo "int main(int argc, char **argv) { return 0; }" | \ + "$compiler" -c \ + -o "$out" \ + -x c \ + -O0 \ + -std=c90 \ + -fno-eliminate-unused-debug-types \ + -g \ + "-I$inc_dir" \ + -include "$header" \ + - +} + +# Print to stderr +errlog() { + echo "$@" >&2 +} + +# Grab the list of incompatible headers from the usr/include Makefile +get_no_header_list() { + { + cat "${KERNEL_SRC}/usr/include/Makefile" + # shellcheck disable=SC2016 + printf '\nall:\n\t@echo $(no-header-test)\n' + } | make -C "${KERNEL_SRC}/usr/include" -f - --just-print \ + | grep '^echo' \ + | cut -d ' ' -f 2- +} + +# Check any changed files in this commit for UAPI compatibility +check_changed_files() { + refs_to_check=("$@") + + local passed=0; + local failed=0; + + while read -r status file; do + local -r base=${file/uapi\//} + + # Get the current version of the file and put it in the install dir + get_header "HEAD" "$file" "${tmp_dir}/usr/${base}" + + for ref in "${refs_to_check[@]}"; do + if ! git rev-parse --verify "$ref" > /dev/null 2>&1; then + echo "error - invalid ref \"$ref\"" + exit 1 + fi + + if check_uapi_for_file "$status" "$file" "$ref" "$base"; then + passed=$((passed + 1)) + else + failed=$((failed + 1)) + fi + done + done < <(cd "$KERNEL_SRC" && git show HEAD --name-status --format="" --diff-filter=a -- include/uapi/) + + total=$((passed + failed)) + if [ "$total" -eq 0 ]; then + errlog "No changes to UAPI headers detected in most recent commit" + else + errlog "${passed}/${total} UAPI header file changes are backwards compatible" + fi + + return "$failed" +} + +# Check UAPI compatibility for a given file +check_uapi_for_file() { + local -r status="$1" + local -r file="$2" + local -r ref="$3" + local -r base="$4" + + # shellcheck disable=SC2076 + if [[ " $(get_no_header_list) " =~ " ${base/include\//} " ]]; then + errlog "$file cannot be tested by this script (see usr/include/Makefile)." + return 1 + fi + + if [ "$status" = "D" ]; then + errlog "UAPI header $file was incorrectly removed" + return 1 + fi + + if [ "$status" = "R" ]; then + errlog "UAPI header $file was incorrectly renamed" + return 1 + fi + + # Get the "previous" verison of the API header and put it in the install dir + get_header "$ref" "$file" "${tmp_dir}/usr/${base}.pre" + + compare_abi "${CROSS_COMPILE}${CC:-gcc}" "$file" "$base" "$ref" +} + +# Perform the A/B compilation and compare output ABI +compare_abi() { + local -r compiler="$1" + local -r file="$2" + local -r base="$3" + local -r ref="$4" + + pre_bin="${tmp_dir}/pre.bin" + post_bin="${tmp_dir}/post.bin" + log="${tmp_dir}/log" + + if ! do_compile "$compiler" "${tmp_dir}/usr/include" "${tmp_dir}/usr/${base}.pre" "$pre_bin" 2> "$log"; then + errlog "Couldn't compile current version of UAPI header $file..." + cat "$log" >&2 + return 1 + fi + + if ! do_compile "$compiler" "${tmp_dir}/usr/include" "${tmp_dir}/usr/${base}" "$post_bin" 2> "$log"; then + errlog "Couldn't compile new version of UAPI header $file..." + cat "$log" >&2 + return 1 + fi + + if "$ABIDIFF" --non-reachable-types "$pre_bin" "$post_bin" > "$log"; then + echo "No ABI differences detected in $file (compared to file at $ref)" + else + errlog "!!! ABI differences detected in $file (compared to file at $ref) !!!" + echo >&2 + sed -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/ /g' "$log" >&2 + echo >&2 + return 1 + fi +} + +# Make sure we have the tools we need +check_deps() { + export ABIDIFF="${ABIDIFF:-abidiff}" + + if ! command -v "$ABIDIFF" > /dev/null 2>&1; then + errlog "error - abidiff not found!" + errlog "Please install abigail-tools (version 1.7 or greater)" + errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" + exit 1 + fi + + read -r abidiff_maj abidiff_min _ < <("$ABIDIFF" --version | cut -d ' ' -f 2 | tr '.' ' ') + if [ "$abidiff_maj" -lt 1 ] || ([ "$abidiff_maj" -eq 1 ] && [ "$abidiff_min" -lt 7 ]); then + errlog "error - abidiff version too old: $("$ABIDIFF" --version)" + errlog "Please install abigail-tools (version 1.7 or greater)" + errlog "See: https://sourceware.org/libabigail/manual/libabigail-overview.html" + exit 1 + fi +} + +main() { + refs_to_check=( "HEAD^1" ) + while getopts "hr:" opt; do + case $opt in + h) + print_usage + exit 0 + ;; + r) + refs_to_check+=( "$OPTARG" ) + ;; + esac + done + + check_deps + + tmp_dir=$(mktemp -d) + trap 'rm -rf $tmp_dir' EXIT + + if [ -z "$KERNEL_SRC" ]; then + KERNEL_SRC="$(realpath "$(dirname "$0")"/..)" + fi + export KERNEL_SRC + + if ! (cd "$KERNEL_SRC" && git rev-parse --is-inside-work-tree > /dev/null 2>&1); then + errlog "error - this script requires the kernel tree to be initialized with Git" + exit 1 + fi + + export ARCH + export CC + export CROSS_COMPILE + + if ! check_changed_files "${refs_to_check[@]}"; then + errlog "UAPI header ABI check failed" + exit 1 + fi +} + +main "$@"
While the kernel community has been good at maintaining backwards compatibility with kernel UAPIs, it would be helpful to have a tool to check if a patch introduces changes that break backwards compatibility. To that end, introduce check-uapi.sh: a simple shell script that checks for changes to UAPI headers using libabigail. libabigail is "a framework which aims at helping developers and software distributors to spot some ABI-related issues like interface incompatibility in ELF shared libraries by performing a static analysis of the ELF binaries at hand." The script uses one of libabigail's tools, "abidiff", to compile the changed header before and after the patch to detect any changes. abidiff "compares the ABI of two shared libraries in ELF format. It emits a meaningful report describing the differences between the two ABIs." Signed-off-by: John Moon <quic_johmoo@quicinc.com> --- scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 245 insertions(+) create mode 100755 scripts/check-uapi.sh -- 2.17.1