lkml.org 
[lkml]   [2023]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh
From
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

\
 
 \ /
  Last update: 2023-03-27 00:33    [W:0.308 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site