Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh

[John Moon]

On 2/19/2023 1:36 AM, Masahiro Yamada wrote:
> On Sat, Feb 18, 2023 at 5:23 AM John Moon <quic_johmoo@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> >   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