[PATCH RESEND 0/1] Validating UAPI backwards compatibility

[PATCH RESEND 0/1] Validating UAPI backwards compatibility

[John Moon]

+ linux-arm-kernel and Greg KH

Hi all,

The kernel community has rigorously enforced a policy of backwards
compatibility in its UAPI headers for a long time. This has allowed user
applications to enjoy stability across kernel upgrades without
recompiling.

In the vendor driver community (out-of-tree modules), there's been a
lack of discipline when it comes to maintaining UAPI backwards
compatibility. This has been a maintenance burden and limits our options
for long-term support of older devices.

Our goal is to add tooling for vendor driver developers because the
upstream model of expert maintainer code review can be difficult to
replicate in-house. Tools may help developers catch simple UAPI
incompatibilities that could be easily overlooked by in-house review.

We see in the kernel documentation:
"Kernel headers are backwards compatible, but not forwards compatible.
This means that a program built against a C library using older kernel
headers should run on a newer kernel (although it may not have access
to new features), but a program built against newer kernel headers may
not work on an older kernel."[1]

How does the kernel enforce this guarantee? We would be interested to
learn about any tools or methods used by kernel developers to make sure
the above statement remains true.

Could the documentation on UAPI maintenance (from a developer's point of
view) be expanded? Internally, we have a set of guidelines for our kernel
developers regarding UAPI compatibility techniques. If there's interest
in supplying a document on this topic with the kernel, we'd be happy to
submit a draft detailing what we have so far as a jumping off point.

Additionally, I've attached a shell script we've been using internally
to validate changes to our UAPI headers are backwards compatible. The
script uses libabigail's[2] tool abidiff[3] to compare a modified
header's ABI before and after a patch is applied. If an existing UAPI is
modified, the script exits non-zero. We use this script in our CI system
to block changes that fail the check.

Currently, the script works with gcc. It generates output like this when
a backwards-incompatible change is made to a UAPI header:

 !!! ABI differences detected in include/uapi/linux/acct.h (compared to
 file at HEAD^1) !!!

     [C] 'struct acct' changed:
       type size changed from 512 to 544 (in bits)
       1 data member insertion:
         '__u32 new_val', at offset 512 (in bits) at acct.h:71:1

 0/1 UAPI header file changes are backwards compatible
 UAPI header ABI check failed

However, we have not had success with clang. It seems clang is more
aggressive in optimizing dead code away (no matter which options we
pass). Therefore, no ABI differences are found.

We wanted to share with the community to receive feedback and any advice
when it comes to tooling/policy surrounding this issue. Our hope is that
the script will help all kernel UAPI authors (even those that haven't
upstreamed yet) maintain good discipline and avoid breaking userspace.

[1] Documentation/kbuild/headers_install.rst
[2] https://sourceware.org/libabigail/manual/libabigail-overview.html
[3] https://sourceware.org/libabigail/manual/abidiff.html

P.S. While at Qualcomm, Jordan Crouse <jorcrous@amazon.com> authored the
original version of the UAPI checker script. Thanks Jordan!

John Moon (1):
  check-uapi: Introduce check-uapi.sh

 scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 245 insertions(+)
 create mode 100755 scripts/check-uapi.sh


base-commit: 033c40a89f55525139fd5b6342281b09b97d05bf
--
2.17.1

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

[John Moon]

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)
+    CC             C compiler (default is "gcc")
+    CROSS_COMPILE  Cross-compiling toochain prefix
+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
+			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"
+	)
+}
+
+# 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; }" | \
+		"$compiler" -c \
+		  -o "$out" \
+		  -x c \
+		  -O0 \
+		  -std=c90 \
+		  -fno-eliminate-unused-debug-types \
+		  -g \
+		  "-I$inc_dir" \
+		  -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-
+}
+
+# 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\//}
+
+		# Get the current version of the file and put it in the install dir
+		get_header "HEAD" "$file" "${tmp_dir}/usr/${base}"
+
+		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/)
+
+	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 [ "$status" = "R" ]; then
+		errlog "UAPI header $file was incorrectly renamed"
+		return 1
+	fi
+
+	# Get the "previous" verison of the API header and put it in the install dir
+	get_header "$ref" "$file" "${tmp_dir}/usr/${base}.pre"
+
+	compare_abi "${CROSS_COMPILE}${CC:-gcc}" "$file" "$base" "$ref"
+}
+
+# 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
+
+	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
+
+	if ! check_changed_files "${refs_to_check[@]}"; then
+		errlog "UAPI header ABI check failed"
+		exit 1
+	fi
+}
+
+main "$@"
--
2.17.1

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

[Greg Kroah-Hartman]

On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon 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

Ok, this is very cool, thank you so much for doing this.

I know Randy Dunlap was also looking into this previously, so I've cc:ed
him and bounced him the original.

I tried this out, and at first glance, this felt like it was just "too
fast" in that nothing actually was being tested.  So I manually added a
field to a structure I know would break the abi, and:

	$ ./scripts/check-uapi.sh
	!!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!!

	    [C] 'struct usb_ctrlrequest' changed:
	      type size changed from 64 to 72 (in bits)
	      1 data member insertion:
		'__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1
	      3 data member changes:
		'__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits)
		'__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits)
		'__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits)

	0/1 UAPI header file changes are backwards compatible
	UAPI header ABI check failed

So it worked!

There is a mismatch of different bash coding styles in the document, which
isn't a big deal, and one warning produced by the `shellcheck` tool, but that
can all be fixed up later.  I'll go queue this up now, as a starting point for
people to play with, thanks!

Also, it would be nice to be able to check if the current tree with changes in
it (not checked in, just modified) breaks the abi, without having to go and
check the change in.  But again, future fixups for people to do!

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Sat, Feb 18, 2023 at 09:17:12AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon 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
> 
> Ok, this is very cool, thank you so much for doing this.
> 
> I know Randy Dunlap was also looking into this previously, so I've cc:ed
> him and bounced him the original.
> 
> I tried this out, and at first glance, this felt like it was just "too
> fast" in that nothing actually was being tested.  So I manually added a
> field to a structure I know would break the abi, and:
> 
> 	$ ./scripts/check-uapi.sh
> 	!!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!!
> 
> 	    [C] 'struct usb_ctrlrequest' changed:
> 	      type size changed from 64 to 72 (in bits)
> 	      1 data member insertion:
> 		'__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1
> 	      3 data member changes:
> 		'__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits)
> 		'__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits)
> 		'__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits)
> 
> 	0/1 UAPI header file changes are backwards compatible
> 	UAPI header ABI check failed
> 
> So it worked!

Ok, I take it back, it doesn't seem to work :(

It only "catches" a change from the last commit, but if you have an
intermediate commit (i.e change something in HEAD^ but not HEAD), it
does not detect it at all.

And if you give it an old version, it doesn't check that either (hint,
try passing in a very old kernel version, that returns instantly and
doesn't actually build anything).

So it's a good first cut as an example, but as it doesn't really work
correctly yet, we can't take it.  Care to fix it up to work so that it
can be usable?

thanks,

greg k-h

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

[Trilok Soni]

On 2/18/2023 12:31 AM, Greg Kroah-Hartman wrote:
> On Sat, Feb 18, 2023 at 09:17:12AM +0100, Greg Kroah-Hartman wrote:
>> On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon 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
>>
>> Ok, this is very cool, thank you so much for doing this.
>>
>> I know Randy Dunlap was also looking into this previously, so I've cc:ed
>> him and bounced him the original.
>>
>> I tried this out, and at first glance, this felt like it was just "too
>> fast" in that nothing actually was being tested.  So I manually added a
>> field to a structure I know would break the abi, and:
>>
>> 	$ ./scripts/check-uapi.sh
>> 	!!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!!
>>
>> 	    [C] 'struct usb_ctrlrequest' changed:
>> 	      type size changed from 64 to 72 (in bits)
>> 	      1 data member insertion:
>> 		'__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1
>> 	      3 data member changes:
>> 		'__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits)
>> 		'__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits)
>> 		'__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits)
>>
>> 	0/1 UAPI header file changes are backwards compatible
>> 	UAPI header ABI check failed
>>
>> So it worked!
> 
> Ok, I take it back, it doesn't seem to work :(
> 
> It only "catches" a change from the last commit, but if you have an
> intermediate commit (i.e change something in HEAD^ but not HEAD), it
> does not detect it at all.
> 
> And if you give it an old version, it doesn't check that either (hint,
> try passing in a very old kernel version, that returns instantly and
> doesn't actually build anything).
> 
> So it's a good first cut as an example, but as it doesn't really work
> correctly yet, we can't take it.  Care to fix it up to work so that it
> can be usable?

These first patches were to introduce the tool w/ the one scenario only, 
and thanks for trying it out. We can fix it and add your suggestions.

---Trilok Soni

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

[Masahiro Yamada]

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>




BTW, the patch is prefixed with 'RESEND', but it contains
several diff lines against the previous submission [1].

People would submit this as v2 with changes mentioned under '---'.


[1]: https://patchwork.kernel.org/project/linux-kbuild/patch/20230217202234.32260-2-quic_johmoo@quicinc.com/






diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh
index 209c6793a4d0..b9cd3a2d7805 100755
--- a/scripts/check-uapi.sh
+++ b/scripts/check-uapi.sh
@@ -3,6 +3,7 @@

 # Script to check a patch for UAPI stability
 set -o errexit
+set -o pipefail

 print_usage() {
        name=$(basename "$0")
@@ -34,7 +35,7 @@ get_header() {

        if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then
                if ! make -C "${KERNEL_SRC}/scripts" unifdef; then
-                       echo 'error - failed to build required
dependency "scripts/unifdef"'
+                       errlog 'error - failed to build required
dependency "scripts/unifdef"'
                        exit 1
                fi
        fi
@@ -111,7 +112,7 @@ check_changed_files() {

        total=$((passed + failed))
        if [ "$total" -eq 0 ]; then
-               errlog "No changes to UAPI headers detected"
+               errlog "No changes to UAPI headers detected in most
recent commit"
        else
                errlog "${passed}/${total} UAPI header file changes
are backwards compatible"
        fi
@@ -190,16 +191,15 @@ check_deps() {
                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 2
+               exit 1
        fi

-       local -r abidiff_maj=$("$ABIDIFF" --version | cut -d ' ' -f 2
| cut -d '.' -f 1)
-       local -r abidiff_min=$("$ABIDIFF" --version | cut -d ' ' -f 2
| cut -d '.' -f 1)
+       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 - installed abidiff version too old:
$("$ABIDIFF" --version)"
+               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 2
+               exit 1
        fi
 }

@@ -220,13 +220,18 @@ main() {
        check_deps

        tmp_dir=$(mktemp -d)
-       #trap 'rm -rf $tmp_dir' EXIT
+       trap 'rm -rf $tmp_dir' EXIT

        if [ -z "$KERNEL_SRC" ]; then
                KERNEL_SRC="$(realpath "$(dirname "$0")"/..)"
        fi
        export KERNEL_SRC

+       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






-- 
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

[-- Attachment #1: Type: text/plain, Size: 5056 bytes --]

On Sat, Feb 18, 2023 at 5:31 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Feb 18, 2023 at 09:17:12AM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon 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
> >
> > Ok, this is very cool, thank you so much for doing this.
> >
> > I know Randy Dunlap was also looking into this previously, so I've cc:ed
> > him and bounced him the original.
> >
> > I tried this out, and at first glance, this felt like it was just "too
> > fast" in that nothing actually was being tested.  So I manually added a
> > field to a structure I know would break the abi, and:
> >
> >       $ ./scripts/check-uapi.sh
> >       !!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!!
> >
> >           [C] 'struct usb_ctrlrequest' changed:
> >             type size changed from 64 to 72 (in bits)
> >             1 data member insertion:
> >               '__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1
> >             3 data member changes:
> >               '__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits)
> >               '__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits)
> >               '__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits)
> >
> >       0/1 UAPI header file changes are backwards compatible
> >       UAPI header ABI check failed
> >
> > So it worked!
>
> Ok, I take it back, it doesn't seem to work :(
>
> It only "catches" a change from the last commit, but if you have an
> intermediate commit (i.e change something in HEAD^ but not HEAD), it
> does not detect it at all.
>
> And if you give it an old version, it doesn't check that either (hint,
> try passing in a very old kernel version, that returns instantly and
> doesn't actually build anything).
>
> So it's a good first cut as an example, but as it doesn't really work
> correctly yet, we can't take it.  Care to fix it up to work so that it
> can be usable?
>
> thanks,
>
> greg k-h



This tool does not even work with changes in HEAD.

I attached two test-case patches.

This patch does not mention any requirement for
the build host, but neither of them works for me,
on Ubuntu 22.04.

I guess people will find more bad cases
if they use older distros as the build host.
(I know why it does not work, though)




For patch 0001:

$ ./scripts/check-uapi.sh
Couldn't compile current version of UAPI header
include/uapi/linux/cec-funcs.h...
In file included from <command-line>:
/tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre: In function
‘cec_msg_set_audio_volume_level’:
/tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre:1575:23: error:
‘CEC_MSG_SET_AUDIO_VOLUME_LEVEL’ undeclared (first use in this
function)
 1575 |         msg->msg[1] = CEC_MSG_SET_AUDIO_VOLUME_LEVEL;
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre:1575:23: note:
each undeclared identifier is reported only once for each function it
appears in
0/1 UAPI header file changes are backwards compatible
UAPI header ABI check failed



For patch 0002:

$ ./scripts/check-uapi.sh
Couldn't compile current version of UAPI header include/uapi/linux/signal.h...
In file included from /tmp/tmp.wm5RykUr3y/usr/include/linux/signal.h.pre:5,
                 from <command-line>:
/usr/include/x86_64-linux-gnu/asm/signal.h:103:9: error: unknown type
name ‘size_t’
  103 |         size_t ss_size;
      |         ^~~~~~
0/1 UAPI header file changes are backwards compatible
UAPI header ABI check failed






BTW, I recommend you to not pick up a patch before having
any reviewer read the code.










--
Best Regards
Masahiro Yamada

[-- Attachment #2: 0001-uapi-linux-cec-funcs.h-add-harmless-change-for-user-.patch --]
[-- Type: application/x-patch, Size: 1976 bytes --]

[-- Attachment #3: 0002-uapi-linux-signal.h-add-harmless-change-for-user-spa.patch --]
[-- Type: application/x-patch, Size: 1628 bytes --]

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

[Masahiro Yamada]

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.





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

Re: [PATCH RESEND 0/1] Validating UAPI backwards compatibility

[Masahiro Yamada]

On Sat, Feb 18, 2023 at 5:23 AM John Moon <quic_johmoo@quicinc.com> wrote:
>
> + linux-arm-kernel and Greg KH
>
> Hi all,
>
> The kernel community has rigorously enforced a policy of backwards
> compatibility in its UAPI headers for a long time. This has allowed user
> applications to enjoy stability across kernel upgrades without
> recompiling.
>
> In the vendor driver community (out-of-tree modules), there's been a
> lack of discipline when it comes to maintaining UAPI backwards
> compatibility. This has been a maintenance burden and limits our options
> for long-term support of older devices.
>
> Our goal is to add tooling for vendor driver developers because the
> upstream model of expert maintainer code review can be difficult to
> replicate in-house. Tools may help developers catch simple UAPI
> incompatibilities that could be easily overlooked by in-house review.
>
> We see in the kernel documentation:
> "Kernel headers are backwards compatible, but not forwards compatible.
> This means that a program built against a C library using older kernel
> headers should run on a newer kernel (although it may not have access
> to new features), but a program built against newer kernel headers may
> not work on an older kernel."[1]
>
> How does the kernel enforce this guarantee? We would be interested to
> learn about any tools or methods used by kernel developers to make sure
> the above statement remains true.
>
> Could the documentation on UAPI maintenance (from a developer's point of
> view) be expanded? Internally, we have a set of guidelines for our kernel
> developers regarding UAPI compatibility techniques. If there's interest
> in supplying a document on this topic with the kernel, we'd be happy to
> submit a draft detailing what we have so far as a jumping off point.
>
> Additionally, I've attached a shell script we've been using internally
> to validate changes to our UAPI headers are backwards compatible. The
> script uses libabigail's[2] tool abidiff[3] to compare a modified
> header's ABI before and after a patch is applied. If an existing UAPI is
> modified, the script exits non-zero. We use this script in our CI system
> to block changes that fail the check.
>
> Currently, the script works with gcc. It generates output like this when
> a backwards-incompatible change is made to a UAPI header:
>
>  !!! ABI differences detected in include/uapi/linux/acct.h (compared to
>  file at HEAD^1) !!!
>
>      [C] 'struct acct' changed:
>        type size changed from 512 to 544 (in bits)
>        1 data member insertion:
>          '__u32 new_val', at offset 512 (in bits) at acct.h:71:1
>
>  0/1 UAPI header file changes are backwards compatible
>  UAPI header ABI check failed
>
> However, we have not had success with clang. It seems clang is more
> aggressive in optimizing dead code away (no matter which options we
> pass). Therefore, no ABI differences are found.
>
> We wanted to share with the community to receive feedback and any advice
> when it comes to tooling/policy surrounding this issue. Our hope is that
> the script will help all kernel UAPI authors (even those that haven't
> upstreamed yet) maintain good discipline and avoid breaking userspace.
>
> [1] Documentation/kbuild/headers_install.rst
> [2] https://sourceware.org/libabigail/manual/libabigail-overview.html
> [3] https://sourceware.org/libabigail/manual/abidiff.html
>
> P.S. While at Qualcomm, Jordan Crouse <jorcrous@amazon.com> authored the
> original version of the UAPI checker script. Thanks Jordan!


If you want to express the authorship of the original,
it is possible to add the "Co-developed-by" tag,
which is mentioned in
Documentation/translations/sp_SP/process/submitting-patches.rst


It depends on how much code you rewrote, though.





>
> John Moon (1):
>   check-uapi: Introduce check-uapi.sh
>
>  scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 245 insertions(+)
>  create mode 100755 scripts/check-uapi.sh
>
>
> base-commit: 033c40a89f55525139fd5b6342281b09b97d05bf
> --
> 2.17.1
>


--
Best Regards
Masahiro Yamada

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

[John Moon]

On 2/18/2023 12:17 AM, Greg Kroah-Hartman wrote:
> On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon 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
> 
> Ok, this is very cool, thank you so much for doing this.
> 
> I know Randy Dunlap was also looking into this previously, so I've cc:ed
> him and bounced him the original.
> 

Okay, will keep him in the loop!

> I tried this out, and at first glance, this felt like it was just "too
> fast" in that nothing actually was being tested.  So I manually added a
> field to a structure I know would break the abi, and:
> 
> 	$ ./scripts/check-uapi.sh
> 	!!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!!
> 
> 	    [C] 'struct usb_ctrlrequest' changed:
> 	      type size changed from 64 to 72 (in bits)
> 	      1 data member insertion:
> 		'__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1
> 	      3 data member changes:
> 		'__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits)
> 		'__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits)
> 		'__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits)
> 
> 	0/1 UAPI header file changes are backwards compatible
> 	UAPI header ABI check failed
> 
> So it worked!
> 
> There is a mismatch of different bash coding styles in the document, which
> isn't a big deal, and one warning produced by the `shellcheck` tool, but that
> can all be fixed up later.  I'll go queue this up now, as a starting point for
> people to play with, thanks!
> 

Will resolve all shellcheck warnings in next rev. Not sure about the 
coding style differences you're referring to - is there a shell script 
coding style document?

> Also, it would be nice to be able to check if the current tree with changes in
> it (not checked in, just modified) breaks the abi, without having to go and
> check the change in.  But again, future fixups for people to do!
> 

Should be straightforward to add. Since I'm making another rev, I'll 
take care of this as well.

> thanks,
> 
> greg k-h

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

[John Moon]

On 2/19/2023 12:52 AM, Masahiro Yamada wrote:
> On Sat, Feb 18, 2023 at 5:31 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Sat, Feb 18, 2023 at 09:17:12AM +0100, Greg Kroah-Hartman wrote:
>>> On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon 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
>>>
>>> Ok, this is very cool, thank you so much for doing this.
>>>
>>> I know Randy Dunlap was also looking into this previously, so I've cc:ed
>>> him and bounced him the original.
>>>
>>> I tried this out, and at first glance, this felt like it was just "too
>>> fast" in that nothing actually was being tested.  So I manually added a
>>> field to a structure I know would break the abi, and:
>>>
>>>        $ ./scripts/check-uapi.sh
>>>        !!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!!
>>>
>>>            [C] 'struct usb_ctrlrequest' changed:
>>>              type size changed from 64 to 72 (in bits)
>>>              1 data member insertion:
>>>                '__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1
>>>              3 data member changes:
>>>                '__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits)
>>>                '__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits)
>>>                '__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits)
>>>
>>>        0/1 UAPI header file changes are backwards compatible
>>>        UAPI header ABI check failed
>>>
>>> So it worked!
>>
>> Ok, I take it back, it doesn't seem to work :(
>>
>> It only "catches" a change from the last commit, but if you have an
>> intermediate commit (i.e change something in HEAD^ but not HEAD), it
>> does not detect it at all.
>>
>> And if you give it an old version, it doesn't check that either (hint,
>> try passing in a very old kernel version, that returns instantly and
>> doesn't actually build anything).

I'll make an update to improve this behavior. Should be able to specify 
the commit in which a change is made and which past commits to check 
against.

>>
>> So it's a good first cut as an example, but as it doesn't really work
>> correctly yet, we can't take it.  Care to fix it up to work so that it
>> can be usable?
>>
>> thanks,
>>
>> greg k-h
> 
> 
> 
> This tool does not even work with changes in HEAD.
> 
> I attached two test-case patches.
> 
> This patch does not mention any requirement for
> the build host, but neither of them works for me,
> on Ubuntu 22.04.
> 
> I guess people will find more bad cases
> if they use older distros as the build host.
> (I know why it does not work, though)
> 
> 
> 
> 
> For patch 0001:
> 
> $ ./scripts/check-uapi.sh
> Couldn't compile current version of UAPI header
> include/uapi/linux/cec-funcs.h...
> In file included from <command-line>:
> /tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre: In function
> ‘cec_msg_set_audio_volume_level’:
> /tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre:1575:23: error:
> ‘CEC_MSG_SET_AUDIO_VOLUME_LEVEL’ undeclared (first use in this
> function)
>   1575 |         msg->msg[1] = CEC_MSG_SET_AUDIO_VOLUME_LEVEL;
>        |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre:1575:23: note:
> each undeclared identifier is reported only once for each function it
> appears in
> 0/1 UAPI header file changes are backwards compatible
> UAPI header ABI check failed
> 
> 
> 
> For patch 0002:
> 
> $ ./scripts/check-uapi.sh
> Couldn't compile current version of UAPI header include/uapi/linux/signal.h...
> In file included from /tmp/tmp.wm5RykUr3y/usr/include/linux/signal.h.pre:5,
>                   from <command-line>:
> /usr/include/x86_64-linux-gnu/asm/signal.h:103:9: error: unknown type
> name ‘size_t’
>    103 |         size_t ss_size;
>        |         ^~~~~~
> 0/1 UAPI header file changes are backwards compatible
> UAPI header ABI check failed
> 

Thanks for the example patches! We understand the issue and I think 
there's a simple solution. We can install all of the current UAPI 
headers into the tmp directory and include those. That way, the user's 
system headers shouldn't enter into the equation.

> 
> 
> 
> 
> 
> BTW, I recommend you to not pick up a patch before having
> any reviewer read the code.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> --
> Best Regards
> Masahiro Yamada

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

[John Moon]

On 2/18/2023 9:25 PM, 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>
> 
> 
> 
> 
> BTW, the patch is prefixed with 'RESEND', but it contains
> several diff lines against the previous submission [1].
> 
> People would submit this as v2 with changes mentioned under '---'.
> 

Sorry about that, I'm still learning the etiquette. :)

I will mark the patch rev as v2 after addressing your comments.

> 
> [1]: https://patchwork.kernel.org/project/linux-kbuild/patch/20230217202234.32260-2-quic_johmoo@quicinc.com/
> 
> 
> 
> 
> 
> 
> diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh
> index 209c6793a4d0..b9cd3a2d7805 100755
> --- a/scripts/check-uapi.sh
> +++ b/scripts/check-uapi.sh
> @@ -3,6 +3,7 @@
> 
>   # Script to check a patch for UAPI stability
>   set -o errexit
> +set -o pipefail
> 
>   print_usage() {
>          name=$(basename "$0")
> @@ -34,7 +35,7 @@ get_header() {
> 
>          if [ ! -x "${KERNEL_SRC}/scripts/unifdef" ]; then
>                  if ! make -C "${KERNEL_SRC}/scripts" unifdef; then
> -                       echo 'error - failed to build required
> dependency "scripts/unifdef"'
> +                       errlog 'error - failed to build required
> dependency "scripts/unifdef"'
>                          exit 1
>                  fi
>          fi
> @@ -111,7 +112,7 @@ check_changed_files() {
> 
>          total=$((passed + failed))
>          if [ "$total" -eq 0 ]; then
> -               errlog "No changes to UAPI headers detected"
> +               errlog "No changes to UAPI headers detected in most
> recent commit"
>          else
>                  errlog "${passed}/${total} UAPI header file changes
> are backwards compatible"
>          fi
> @@ -190,16 +191,15 @@ check_deps() {
>                  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 2
> +               exit 1
>          fi
> 
> -       local -r abidiff_maj=$("$ABIDIFF" --version | cut -d ' ' -f 2
> | cut -d '.' -f 1)
> -       local -r abidiff_min=$("$ABIDIFF" --version | cut -d ' ' -f 2
> | cut -d '.' -f 1)
> +       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 - installed abidiff version too old:
> $("$ABIDIFF" --version)"
> +               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 2
> +               exit 1
>          fi
>   }
> 
> @@ -220,13 +220,18 @@ main() {
>          check_deps
> 
>          tmp_dir=$(mktemp -d)
> -       #trap 'rm -rf $tmp_dir' EXIT
> +       trap 'rm -rf $tmp_dir' EXIT
> 
>          if [ -z "$KERNEL_SRC" ]; then
>                  KERNEL_SRC="$(realpath "$(dirname "$0")"/..)"
>          fi
>          export KERNEL_SRC
> 
> +       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
> 
> 
> 
> 
> 
> 

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@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