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.
> + 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
