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
