Message-ID: <6dffbb7f-cbf5-7bb5-7eba-643bf772e64b@quicinc.com>
Date:   Wed, 22 Feb 2023 13:06:14 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh
Content-Language: en-US
To:     Masahiro Yamada <masahiroy@kernel.org>
CC:     Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>,
        <linux-kbuild@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>,
        "Trilok Soni" <quic_tsoni@quicinc.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Bjorn Andersson <andersson@kernel.org>,
        "Todd Kjos" <tkjos@google.com>,
        Matthias Maennich <maennich@google.com>,
        "Giuliano Procida" <gprocida@google.com>,
        <kernel-team@android.com>, Jordan Crouse <jorcrous@amazon.com>
References: <20230217202234.32260-1-quic_johmoo@quicinc.com>
 <20230217202234.32260-2-quic_johmoo@quicinc.com>
 <CAK7LNAQMgsNiizzKuZD+3VvJ=hPygcJ8PwNE+Q6pnxzBmQezCA@mail.gmail.com>
From:   John Moon <quic_johmoo@quicinc.com>
In-Reply-To: <CAK7LNAQMgsNiizzKuZD+3VvJ=hPygcJ8PwNE+Q6pnxzBmQezCA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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