[PATCH v2 0/2] Validating UAPI backwards compatibility

[PATCH v2 0/2] Validating UAPI backwards compatibility

[John Moon]

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.

In v2, we've expanded the functionality quite a bit. We've added a
document for the tool which includes some examples and explanations of
possible false positives. We've also made it easier to examine changes
across a large range of commits (e.g. v6.0 -> v6.1). This provided a
great testbed of a wide variety of changes to examine.

It would be very helpful for the expert maintainers to look over the
tool's output and describe why or why not certain changes are being
incorrectly flagged. This could lead the way towards another document
that describes the kernel's UAPI policy more formally.

As our tooling improves, we may be able to effectively filter out the
false positives so the tool fits the kernel's UAPI policy neatly.

Thanks for the help on v1 and thanks in advance for the help on v2!

[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 (2):
  check-uapi: Introduce check-uapi.sh
  docs: dev-tools: Add UAPI checker documentation

 Documentation/dev-tools/checkuapi.rst | 258 +++++++++++++++
 Documentation/dev-tools/index.rst     |   1 +
 scripts/check-uapi.sh                 | 452 ++++++++++++++++++++++++++
 3 files changed, 711 insertions(+)
 create mode 100644 Documentation/dev-tools/checkuapi.rst
 create mode 100755 scripts/check-uapi.sh


base-commit: e492250d5252635b6c97d52eddf2792ec26f1ec1
--
2.17.1

[PATCH v2 1/2] 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 commit 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 commit 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."

The script also includes the ability to check the compatibilty of
all UAPI headers across commits. This allows developers to inspect
the stability of the UAPIs over time.

Signed-off-by: John Moon <quic_johmoo@quicinc.com>
---
   - Fixed issue where system UAPI headers were used instead of kernel
     source headers. Now, script will include all UAPI headers from
     source instead of just target ones.
   - Added logic to install all requires sanitized headers to the tmp
     directory up front (including across git commits)
   - Added filter for changes which only add types (which should be
     allowed for UAPI headers)
   - Added -b flag to specify "base commit" to compare against.
   - Modified logic to check for any changed UAPI files between the
     base commit and reference commit.
   - Added -a flag to check compatibility of all UAPI files.
   - Added threaded execution and -j flag to control number of jobs.
   - Added error log and -l option to control output path.
   - Added support for checking dirty git workspaces.
   - Added summary file output with header diffs
   - Added "arch/*/include/uapi" to list of files to check.
   - Addressed misc code review findings.

 scripts/check-uapi.sh | 451 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 451 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 000000000000..022cc7f8a2a9
--- /dev/null
+++ b/scripts/check-uapi.sh
@@ -0,0 +1,451 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+# Script to check commits for UAPI backwards compatibility
+
+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 (or current
+dirty changes) did not introduce ABI changes when compared to HEAD^1. You can
+check against additional commit ranges with the -b and -r options.
+
+To force the script to check compatibility of all UAPI headers, use the -a
+option.
+
+Usage: $name [-b BASE_REF] [-r COMP_REF] [-a] [-j N] [-l ERROR_LOG]
+
+Options:
+    -b BASE_REF    Base git reference to use for comparison. If unspecified or empty,
+                   will use any dirty changes in tree to UAPI files. If there are no
+                   dirty changes, HEAD will be used.
+    -r COMP_REF    Compare BASE_REF to COMP_REF (e.g. -r v6.1). If unspecified or empty,
+                   will use BASE_REF^1.
+    -a             Check all UAPI headers for backwards compatibility.
+    -j JOBS        Number of checks to run in parallel (default: number of CPU cores)
+    -l ERROR_LOG   Write error log to file (default: "\$KERNEL_SOURCE/abi_error_log.txt")
+
+Environmental args:
+    ABIDIFF  Custom path to abidiff binary
+    CC       C compiler (default is "gcc")
+EOF
+}
+
+# Some UAPI headers require an architecture-specific compiler to build properly.
+ARCH_SPECIFIC_CC_NEEDED=(
+	"arch/hexagon/include/uapi/asm/sigcontext.h"
+	"arch/ia64/include/uapi/asm/intel_intrin.h"
+	"arch/ia64/include/uapi/asm/setup.h"
+	"arch/ia64/include/uapi/asm/sigcontext.h"
+	"arch/mips/include/uapi/asm/bitfield.h"
+	"arch/mips/include/uapi/asm/byteorder.h"
+	"arch/mips/include/uapi/asm/inst.h"
+	"arch/sparc/include/uapi/asm/fbio.h"
+	"arch/sparc/include/uapi/asm/uctx.h"
+	"arch/xtensa/include/uapi/asm/byteorder.h"
+	"arch/xtensa/include/uapi/asm/msgbuf.h"
+	"arch/xtensa/include/uapi/asm/sembuf.h"
+)
+
+# Print to stderr
+eprintf() {
+	# shellcheck disable=SC2059
+	printf "$@" >&2
+}
+
+# Print list of UAPI files to operate on
+get_uapi_files() {
+	local -r check_all="$1"
+	local -r base_ref="$2"
+	local -r ref="$3"
+	local -r args="--name-status --no-renames --format= --diff-filter=a -- ${UAPI_DIRS[*]}"
+
+	if [ "$check_all" = "true" ]; then
+		# Use find to print all of the UAPI files as if git had detected they were modified
+		# Ignore the headers that need an arch-specific compiler because we can't build all
+		# of those in one run (as only one CC can be passed).
+		# shellcheck disable=SC2046,SC2048,SC2086
+		find "${UAPI_DIRS[@]}" -type f -name '*.h' -printf 'M\t%p\n' \
+			| grep -v $(get_no_header_list | xargs -- printf '-e %s ') \
+				  ${ARCH_SPECIFIC_CC_NEEDED[*]/#/-e }
+	else
+		if [ -z "$base_ref" ] || [ -z "$ref" ]; then
+			# shellcheck disable=SC2086
+			git diff $args
+		else
+			# shellcheck disable=SC2086
+			git diff "$ref" "$base_ref" $args
+		fi
+	fi
+}
+
+# Compile the simple test app
+do_compile() {
+	local -r inc_dir="$1"
+	local -r header="$2"
+	local -r out="$3"
+	printf "int main(void) { return 0; }\n" | \
+		"${CC:-gcc}" -c \
+		  -o "$out" \
+		  -x c \
+		  -O0 \
+		  -std=c90 \
+		  -fno-eliminate-unused-debug-types \
+		  -g \
+		  "-I${inc_dir}" \
+		  -include "$header" \
+		  -
+}
+
+# Print the list of incompatible headers from the usr/include Makefile
+get_no_header_list() {
+	{
+		# shellcheck disable=SC2016
+		printf 'all: ; @echo $(no-header-test)\n'
+		cat "usr/include/Makefile"
+	} | make -f - | tr " " "\n" | grep -v "asm-generic"
+
+	# One additional header file is not building correctly
+	# with this method.
+	# TODO: why can't we build this one?
+	printf "asm-generic/ucontext.h\n"
+}
+
+# Save the current git tree state, stashing if needed
+save_tree_state() {
+	printf "Saving current tree state... "
+	current_ref="$(git rev-parse HEAD)"
+	readonly current_ref
+	if ! git diff-index --quiet HEAD; then
+		unstash="true"
+		git stash push --quiet
+	fi
+	printf "OK\n"
+}
+
+# Restore the git tree state, unstashing if needed
+restore_tree_state() {
+	printf "Restoring current tree state... "
+	git checkout --quiet "$current_ref"
+	if [ "$unstash" = "true" ]; then
+		git stash pop --quiet
+		unstash="false"
+	fi
+	printf "OK\n"
+}
+
+# Install headers for every arch and ref we need
+install_headers() {
+	local -r check_all="$1"
+	local -r base_ref="$2"
+	local -r ref="$3"
+
+	local arch_list=()
+	while read -r status file; do
+		if arch="$(printf "%s" "$file" | grep -o 'arch/.*/uapi' | cut -d '/' -f 2)"; then
+			# shellcheck disable=SC2076
+			if ! [[ " ${arch_list[*]} " =~ " $arch " ]]; then
+				arch_list+=("$arch")
+			fi
+		fi
+	done < <(get_uapi_files "$check_all" "$base_ref" "$ref")
+
+	deviated_from_current_tree="false"
+	for inst_ref in "$base_ref" "$ref"; do
+		if [ -n "$inst_ref" ]; then
+			if [ "$deviated_from_current_tree" = "false" ]; then
+				save_tree_state
+				trap 'rm -rf "$tmp_dir"; restore_tree_state;' EXIT
+				deviated_from_current_tree="true"
+			fi
+			git checkout --quiet "$(git rev-parse "$inst_ref")"
+		fi
+
+		printf "Installing sanitized UAPI headers from %s... " "${inst_ref:-dirty tree}"
+		make INSTALL_HDR_PATH="${tmp_dir}/${inst_ref}/usr" headers_install > /dev/null 2>&1
+		for arch in "${arch_list[@]}"; do
+			make ARCH="$arch" INSTALL_HDR_PATH="${tmp_dir}/${inst_ref}/${arch}/usr" \
+				headers_install > /dev/null 2>&1
+		done
+		printf "OK\n"
+	done
+
+	restore_tree_state
+	trap 'rm -rf "$tmp_dir"' EXIT
+}
+
+# Check file list for UAPI compatibility
+check_uapi_files() {
+	local -r check_all="$1"
+	local -r base_ref="$2"
+	local -r ref="$3"
+
+	install_headers "$check_all" "$base_ref" "$ref"
+
+	local passed=0;
+	local failed=0;
+	local -a threads=()
+
+	printf "Checking changes to UAPI headers starting from %s\n" "${base_ref:-dirty tree}"
+	while read -r status file; do
+		if [ "${#threads[@]}" -ge "$MAX_THREADS" ]; then
+			if wait "${threads[0]}"; then
+				passed=$((passed + 1))
+			else
+				failed=$((failed + 1))
+			fi
+			threads=("${threads[@]:1}")
+		fi
+
+		check_individual_file "$base_ref" "$ref" "$status" "$file" &
+		threads+=("$!")
+	done < <(get_uapi_files "$check_all" "$base_ref" "$ref")
+
+	for t in "${threads[@]}"; do
+		if wait "$t"; then
+			passed=$((passed + 1))
+		else
+			failed=$((failed + 1))
+		fi
+	done
+
+	total="$((passed + failed))"
+	if [ "$failed" -gt 0 ]; then
+		eprintf "error - %d/%d UAPI headers modified between %s and %s are not backwards compatible\n" \
+			"$failed" "$total" "$ref" "${base_ref:-dirty tree}"
+	else
+		printf "All %d UAPI headers modified between %s and %s are backwards compatible!\n" \
+			"$total" "$ref" "${base_ref:-dirty tree}"
+	fi
+
+	return "$failed"
+}
+
+# Print the path to a give header in the tmp_dir
+get_header() {
+	local -r ref="$1"
+	local -r arch="$2"
+	local -r base="$3"
+
+	if [ -z "$arch" ]; then
+		printf "%s" "${tmp_dir}/${ref}/usr/${base}"
+	else
+		printf "%s" "${tmp_dir}/${ref}/${arch}/usr/$(printf "%s" "$base" | cut -d '/' -f 3-)"
+	fi
+}
+
+# Check an individual file for UAPI compatibility
+check_individual_file() {
+	local -r base_ref="$1"
+	local -r ref="$2"
+	local -r status="$3"
+	local -r file="$4"
+
+	if [ "$status" = "D" ]; then
+		eprintf "error - UAPI header %s was incorrectly removed\n" "$file"
+		return 1
+	fi
+
+	local -r base=${file/uapi\//}
+	local -r uapi_arch="$(printf "%s" "$file" | grep -o 'arch/.*/uapi' | cut -d '/' -f 2)"
+	local -r base_header=$(get_header "$base_ref" "$uapi_arch" "$base")
+	local -r ref_header=$(get_header "$ref" "$uapi_arch" "$base")
+	local -r installed_base="$(printf "%s" "$base_header" | grep -o "usr/include/.*" | cut -d '/' -f 3-)"
+
+	# shellcheck disable=SC2076
+	if [[ " $(get_no_header_list | xargs) " =~ " $installed_base " ]]; then
+		eprintf "%s cannot be tested by this script (see usr/include/Makefile)\n" "$file"
+		return 1
+	fi
+
+	for h in "$base_header" "$ref_header"; do
+		if [ ! -f "$h" ]; then
+			eprintf "%s does not exist - cannot compare ABI\n" "$h"
+			return 1
+		fi
+	done
+
+	compare_abi "$file" "$base_header" "$ref_header" "$base_ref" "$ref" "$uapi_arch"
+}
+
+# Perform the A/B compilation and compare output ABI
+compare_abi() {
+	local -r file="$1"
+	local -r base_header="$2"
+	local -r ref_header="$3"
+	local -r base_ref="$4"
+	local -r ref="$5"
+	local -r uapi_arch="$6"
+	local -r log="${tmp_dir}/log/$(basename "$file").log"
+
+	mkdir -p "$(dirname "$log")"
+
+	if ! do_compile "${tmp_dir}/${base_ref}/${uapi_arch}/usr/include" "$base_header" "${base_header}.bin" 2> "$log"; then
+		eprintf "error - couldn't compile current version of UAPI header %s\n" "$file"
+		# shellcheck disable=SC2076
+		if [[ " ${ARCH_SPECIFIC_CC_NEEDED[*]} " =~ " $file " ]]; then
+			eprintf "warning - this file needs to be built with a %s compiler. Are you using one?\n" "$uapi_arch"
+		fi
+		cat "$log" >&2
+		exit 1
+	fi
+
+	if ! do_compile "${tmp_dir}/${ref}/${uapi_arch}/usr/include" "$ref_header" "${ref_header}.bin" 2> "$log"; then
+		eprintf "error - couldn't compile version of UAPI header %s at %s\n" "$file" "$ref"
+		# shellcheck disable=SC2076
+		if [[ " ${ARCH_SPECIFIC_CC_NEEDED[*]} " =~ " $file " ]]; then
+			eprintf "warning - this file needs to be built with a %s compiler. Are you using one?\n" "$uapi_arch"
+		fi
+
+		cat "$log" >&2
+		exit 1
+	fi
+
+	if "$ABIDIFF" --non-reachable-types "${ref_header}.bin" "${base_header}.bin" > "$log"; then
+		printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$ref" "${base_ref:-dirty tree}"
+	else
+		# If the only changes were additions (not modifications to existing APIs), then
+		# there's no problem. Ignore these diffs.
+		if grep "Unreachable types summary" "$log" | grep -q "0 removed" &&
+		   grep "Unreachable types summary" "$log" | grep -q "0 changed"; then
+			return 0
+		fi
+		{
+			printf "!!! ABI differences detected in %s from %s -> %s !!!\n\n" "$file" "$ref" "${base_ref:-dirty tree}"
+			sed  -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/  /g' "$log"
+			printf "\nHeader file diff (after headers_install):\n"
+			diff -Naur "$ref_header" "$base_header" \
+				| sed -e "s|${ref_header}|${ref}/${file}|g" \
+				      -e "s|${base_header}|${base_ref:-dirty}/${file}|g"
+			printf "\n"
+		} | tee "${base_header}.error" >&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
+		eprintf "error - abidiff not found!\n"
+		eprintf "Please install abigail-tools (version 1.7 or greater)\n"
+		eprintf "See: https://sourceware.org/libabigail/manual/libabigail-overview.html\n"
+		exit 1
+	fi
+
+	read -r abidiff_maj abidiff_min _unused < <("$ABIDIFF" --version | cut -d ' ' -f 2 | tr '.' ' ')
+	if [ "$abidiff_maj" -lt 1 ] || { [ "$abidiff_maj" -eq 1 ] && [ "$abidiff_min" -lt 7 ]; }; then
+		eprintf "error - abidiff version too old: %s\n" "$("$ABIDIFF" --version)"
+		eprintf "Please install abigail-tools (version 1.7 or greater)\n"
+		eprintf "See: https://sourceware.org/libabigail/manual/libabigail-overview.html\n"
+		exit 1
+	fi
+
+	if [ ! -x "scripts/unifdef" ]; then
+		if ! make -f /dev/null scripts/unifdef; then
+			eprintf 'error - failed to build required dependency "scripts/unifdef"\n'
+			exit 1
+		fi
+	fi
+}
+
+main() {
+	MAX_THREADS=$(nproc)
+
+	base_ref=""
+	check_all="false"
+	while getopts "hb:r:aj:l:" opt; do
+		case $opt in
+		h)
+			print_usage
+			exit 0
+			;;
+		b)
+			base_ref="$OPTARG"
+			;;
+		r)
+			ref_to_check="$OPTARG"
+			;;
+		a)
+			check_all="true"
+			;;
+		j)
+			MAX_THREADS="$OPTARG"
+			;;
+		l)
+			abi_error_log="$OPTARG"
+			;;
+		*)
+			eprintf "error - invalid option %s\n" "$opt"
+			exit 1
+		esac
+	done
+
+	if [ -z "$KERNEL_SRC" ]; then
+		KERNEL_SRC="$(realpath "$(dirname "$0")"/..)"
+	fi
+
+	cd "$KERNEL_SRC"
+
+	abi_error_log="${abi_error_log:-${KERNEL_SRC}/abi_error_log.txt}"
+
+	check_deps
+
+	tmp_dir=$(mktemp -d)
+	trap 'rm -rf "$tmp_dir"' EXIT
+
+	# Set of UAPI directories to check by default
+	UAPI_DIRS=(include/uapi arch/*/include/uapi)
+
+	if ! git rev-parse --is-inside-work-tree > /dev/null 2>&1; then
+		eprintf "error - this script requires the kernel tree to be initialized with Git\n"
+		exit 1
+	fi
+
+	# If there are no dirty UAPI files, use HEAD as base_ref
+	if [ -z "$base_ref" ] && [ "$(get_uapi_files "" "" | wc -l)" -eq 0 ]; then
+		base_ref="HEAD"
+	fi
+
+	if [ -z "$ref_to_check" ]; then
+		if [ -n "$base_ref" ]; then
+			ref_to_check="${base_ref}^1"
+		else
+			ref_to_check="HEAD"
+		fi
+	fi
+
+	if [ -n "$ref_to_check" ] && ! git rev-parse --verify "$ref_to_check" > /dev/null 2>&1; then
+		printf 'error - invalid git reference "%s"\n' "$ref_to_check"
+		exit 1
+	fi
+
+	if [ -n "$base_ref" ]; then
+		if ! git merge-base --is-ancestor "$ref_to_check" "$base_ref" > /dev/null 2>&1; then
+			printf 'error - "%s" is not an ancestor of base ref "%s"\n' "$ref_to_check" "$base_ref"
+			exit 1
+		fi
+	fi
+
+	if [ "$check_all" != "true" ] && [ "$(get_uapi_files "$check_all" "$base_ref" "$ref_to_check" | wc -l)" -eq 0 ]; then
+		printf "No changes to UAPI headers were applied between %s and %s\n" "$ref_to_check" "$base_ref"
+		exit 0
+	fi
+
+	if ! check_uapi_files "$check_all" "$base_ref" "$ref_to_check"; then
+		eprintf "error - UAPI header ABI check failed\n"
+		{
+			printf 'Generated by "%s %s" from git ref %s\n\n' "$0" "$*" "$(git rev-parse "HEAD")"
+			find "$tmp_dir" -type f -name '*.error' -exec cat {} \;
+		} > "$abi_error_log"
+		eprintf "Failure summary saved to %s\n" "$abi_error_log"
+		exit 1
+	fi
+}
+
+main "$@"
--
2.17.1

[PATCH v2 2/2] docs: dev-tools: Add UAPI checker documentation

[John Moon]

Add detailed documentation for scripts/check-uapi.sh.
---
 Documentation/dev-tools/checkuapi.rst | 258 ++++++++++++++++++++++++++
 Documentation/dev-tools/index.rst     |   1 +
 2 files changed, 259 insertions(+)
 create mode 100644 Documentation/dev-tools/checkuapi.rst

diff --git a/Documentation/dev-tools/checkuapi.rst b/Documentation/dev-tools/checkuapi.rst
new file mode 100644
index 000000000000..2255066658e3
--- /dev/null
+++ b/Documentation/dev-tools/checkuapi.rst
@@ -0,0 +1,258 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+============
+UAPI Checker
+============
+
+The UAPI checker (scripts/check-uapi.sh) is a shell script which checks UAPI
+header files for userspace backwards-compatibility across the git tree.
+
+The script can produce false positives in some cases, so developers are
+encouraged to use their best judgement when interpreting the results. Please
+refer to kernel documentation on the topic of IOCTL stability for more
+information (Documentation/process/botching-up-ioctls.rst).
+
+Options
+=======
+
+This section will describe the options check-uapi.sh can be run with.
+
+Usage::
+
+    ./scripts/check-uapi.sh [-b BASE_REF] [-r COMP_REF] [-a] [-j N] [-l ERROR_LOG]
+
+Available options::
+
+    -b BASE_REF    Base git reference to use for comparison. If unspecified or empty,
+                   will use any dirty changes in tree to UAPI files. If there are no
+                   dirty changes, HEAD will be used.
+    -r COMP_REF    Compare BASE_REF to COMP_REF (e.g. -r v6.1). If unspecified or empty,
+                   will use BASE_REF^1.
+    -a             Check all UAPI headers for backwards compatibility.
+    -j JOBS        Number of checks to run in parallel (default: number of CPU cores)
+    -l ERROR_LOG   Write error log to file (default: "$KERNEL_SOURCE/abi_error_log.txt")
+
+Environmental args::
+
+    ABIDIFF  Custom path to abidiff binary
+    CC       C compiler (default is "gcc")
+
+Examples
+========
+
+First, let's try making a change to a UAPI header file that obviously won't
+break userspace::
+
+    cat << 'EOF' | patch -l -p1
+    --- a/include/uapi/linux/acct.h
+    +++ b/include/uapi/linux/acct.h
+    @@ -21,7 +21,9 @@
+     #include <asm/param.h>
+     #include <asm/byteorder.h>
+
+    -/*
+    +#define FOO
+    +
+    +/*
+      *  comp_t is a 16-bit "floating" point number with a 3-bit base 8
+      *  exponent and a 13-bit fraction.
+      *  comp2_t is 24-bit with 5-bit base 2 exponent and 20 bit fraction
+    diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
+    EOF
+
+Now, let's use the script to validate::
+
+    % ./scripts/check-uapi.sh
+    Installing sanitized UAPI headers from dirty tree... OK
+    Saving current tree state... OK
+    Installing sanitized UAPI headers from HEAD... OK
+    Restoring current tree state... OK
+    Checking changes to UAPI headers starting from dirty tree
+    No ABI differences detected in include/uapi/linux/acct.h from HEAD -> dirty tree
+    All 1 UAPI headers modified between HEAD and dirty tree are backwards compatible!
+
+Let's add another change that *would* break userspace::
+
+    cat << 'EOF' | patch -l -p1
+    --- a/include/uapi/linux/bpf.h  2023-02-28 13:32:36.505591077 -0800
+    +++ b/include/uapi/linux/bpf.h  2023-02-28 13:32:57.033494020 -0800
+    @@ -73,7 +73,7 @@
+            __u8    dst_reg:4;      /* dest register */
+            __u8    src_reg:4;      /* source register */
+            __s16   off;            /* signed offset */
+    -       __s32   imm;            /* signed immediate constant */
+    +       __u32   imm;            /* unsigned immediate constant */
+     };
+
+     /* Key of an a BPF_MAP_TYPE_LPM_TRIE entry */
+    EOF
+
+The script should catch this incompatibility::
+
+    % ./scripts/check-uapi.sh
+    Installing sanitized UAPI headers from dirty tree... OK
+    Saving current tree state... OK
+    Installing sanitized UAPI headers from HEAD... OK
+    Restoring current tree state... OK
+    Checking changes to UAPI headers starting from dirty tree
+    No ABI differences detected in include/uapi/linux/acct.h from HEAD -> dirty tree
+    !!! ABI differences detected in include/uapi/linux/bpf.h from HEAD -> dirty tree !!!
+
+        [C] 'struct bpf_insn' changed:
+          type size hasn't changed
+          1 data member change:
+            type of '__s32 imm' changed:
+              typedef name changed from __s32 to __u32 at int-ll64.h:27:1
+              underlying type 'int' changed:
+                type name changed from 'int' to 'unsigned int'
+                type size hasn't changed
+
+    Header file diff (after headers_install):
+    --- HEAD/include/uapi/linux/bpf.h       2023-02-28 22:24:44.068898545 -0800
+    +++ dirty/include/uapi/linux/bpf.h      2023-02-28 22:24:42.132909241 -0800
+    @@ -73,7 +73,7 @@
+            __u8    dst_reg:4;      /* dest register */
+            __u8    src_reg:4;      /* source register */
+            __s16   off;            /* signed offset */
+    -       __s32   imm;            /* signed immediate constant */
+    +       __u32   imm;            /* unsigned immediate constant */
+     };
+
+     /* Key of an a BPF_MAP_TYPE_LPM_TRIE entry */
+
+    error - 1/2 UAPI headers modified between HEAD and dirty tree are not backwards compatible
+    error - UAPI header ABI check failed
+
+Note: the script is operating on UAPI header files which have changed in the
+dirty git tree. If there were no changes in the tree, it would look for UAPI
+changes introduced by the latest HEAD commit.
+
+Let's commit the breaking change, then commit the good change::
+
+    % git commit -m 'Breaking UAPI change' include/uapi/linux/bpf.h
+    [detached HEAD f758e574663a] Breaking UAPI change
+     1 file changed, 1 insertion(+), 1 deletion(-)
+    % git commit -m 'Innocuous UAPI change' include/uapi/linux/acct.h
+    [detached HEAD 2e87df769081] Innocuous UAPI change
+     1 file changed, 3 insertions(+), 1 deletion(-)
+
+Now, what happens if we run the script again with no arguments?::
+
+    % ./scripts/check-uapi.sh
+    Saving current tree state... OK
+    Installing sanitized UAPI headers from HEAD... OK
+    Installing sanitized UAPI headers from HEAD^1... OK
+    Restoring current tree state... OK
+    Checking changes to UAPI headers starting from HEAD
+    No ABI differences detected in include/uapi/linux/acct.h from HEAD^1 -> HEAD
+    All 1 UAPI headers modified between HEAD^1 and HEAD are backwards compatible!
+
+It doesn't catch any breaking change because by default, it only compares HEAD
+to HEAD^1. If we wanted the search scope to go back further, we'd have to use
+the "-r" option to pass other references to compare to. In this case, let's
+pass "-r HEAD~2" to the script so it checks UAPI changes between HEAD~2 and
+HEAD::
+
+    % ./scripts/check-uapi.sh -r "HEAD~2"
+    Saving current tree state... OK
+    Installing sanitized UAPI headers from HEAD... OK
+    Installing sanitized UAPI headers from HEAD~2... OK
+    Restoring current tree state... OK
+    Checking changes to UAPI headers starting from HEAD
+    No ABI differences detected in include/uapi/linux/acct.h from HEAD~2 -> HEAD
+    !!! ABI differences detected in include/uapi/linux/bpf.h from HEAD~2 -> HEAD !!!
+
+        [C] 'struct bpf_insn' changed:
+          type size hasn't changed
+          1 data member change:
+            type of '__s32 imm' changed:
+              typedef name changed from __s32 to __u32 at int-ll64.h:27:1
+              underlying type 'int' changed:
+                type name changed from 'int' to 'unsigned int'
+                type size hasn't changed
+
+    Header file diff (after headers_install):
+    --- HEAD~2/include/uapi/linux/bpf.h     2023-02-28 22:27:40.311925004 -0800
+    +++ HEAD/include/uapi/linux/bpf.h       2023-02-28 22:27:39.743928142 -0800
+    @@ -73,7 +73,7 @@
+            __u8    dst_reg:4;      /* dest register */
+            __u8    src_reg:4;      /* source register */
+            __s16   off;            /* signed offset */
+    -       __s32   imm;            /* signed immediate constant */
+    +       __u32   imm;            /* unsigned immediate constant */
+     };
+
+     /* Key of an a BPF_MAP_TYPE_LPM_TRIE entry */
+
+    error - 1/2 UAPI headers modified between HEAD~2 and HEAD are not backwards compatible
+    error - UAPI header ABI check failed
+
+If you want to check different chunks of the tree, you can also change your
+base commit with the "-b" option. For example, to check all changed UAPI header
+files between v6.0 and v6.1, you'd run::
+
+    % ./scripts/check-uapi.sh -b v6.1 -r v6.0
+    Saving current tree state... OK
+    Installing sanitized UAPI headers from v6.1... OK
+    Installing sanitized UAPI headers from v6.0... OK
+    Restoring current tree state... OK
+    Checking changes to UAPI headers starting from v6.1
+    -- snip --
+    error - 42/106 UAPI headers modified between v6.0 and v6.1 are not backwards compatible
+    error - UAPI header ABI check failed
+
+Aren't kernel UAPIs stable forever? Why is the script reporting errors?
+
+False Positives
+===============
+
+The UAPI checker is very aggressive in detecting ABI changes, so some false
+positives may appear. For example, if you check all UAPI headers between v6.0
+and v6.1, many breakages will be flagged. Run the following::
+
+    ./scripts/check-uapi.sh -b v6.1 -r v6.0
+
+The errors will be logged to "abi_error_log.txt". Here, we'll find examples of
+several types of false positives.
+
+Enum Expansion::
+
+    !!! ABI differences detected in include/uapi/linux/openvswitch.h from v6.0 -> v6.1 !!!
+
+        [C] 'enum ovs_datapath_attr' changed:
+          type size hasn't changed
+          1 enumerator insertion:
+            'ovs_datapath_attr::OVS_DP_ATTR_IFINDEX' value '9'
+          1 enumerator change:
+            'ovs_datapath_attr::__OVS_DP_ATTR_MAX' from value '9' to '10' at openvswitch.h.current:85:1
+
+In this case, an enum was expanded. Consequently, the "MAX" value was
+incremented. This is not considered a breaking change because it's assumed
+userspace programs are using the MAX value in a sane fashion.
+
+Expanding into reserved/padding fields::
+
+    !!! ABI differences detected in include/uapi/linux/perf_event.h from v6.0 -> v6.1 !!!
+
+        [C] 'struct perf_branch_entry' changed:
+          type size hasn't changed
+          3 data member insertions:
+            '__u64 spec', at offset 152 (in bits) at perf_event.h.current:1420:1
+            '__u64 new_type', at offset 154 (in bits) at perf_event.h.current:1421:1
+            '__u64 priv', at offset 158 (in bits) at perf_event.h.current:1422:1
+          1 data member change:
+            '__u64 reserved' offset changed from 152 to 161 (in bits) (by +9 bits)
+
+In this case, a reserved field was expanded into. Previously, the reserved
+field occupied 40 bits in the struct. After the change, three new members
+were added that took up 9 bits, so the size of the reserved field was
+reduced to 31.
+
+As the size of the struct did not change and none of the fields a userspace
+program could have been using were removed/changed/relocated, this change is
+not considered breaking.
+
+In the future, as tooling improves, we may be able to filter out more of these
+false positives. There may also be additional examples of false positives not
+listed here. Use your best judgement, and ideally a unit test in userspace, to
+test your UAPI changes!
diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst
index 6b0663075dc0..0876f5a2cf55 100644
--- a/Documentation/dev-tools/index.rst
+++ b/Documentation/dev-tools/index.rst
@@ -34,6 +34,7 @@ Documentation/dev-tools/testing-overview.rst
    kselftest
    kunit/index
    ktap
+   checkuapi


 .. only::  subproject and html
--
2.17.1

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[Nick Desaulniers]

On Tue, Feb 28, 2023 at 11:54 PM John Moon <quic_johmoo@quicinc.com> wrote:
>
> 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.

Hi John,
Do you have the list of bugs you've filed upstream against clang wrt.
information missing when using `-fno-eliminate-unused-debug-types`?

https://github.com/llvm/llvm-project/issues is the issue tracker.

Seeing a strong participant in both the Android and LLVM ecosystems
supply scripts that lack clang support...raises eyebrows.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[John Moon]

On 3/1/2023 9:50 AM, Nick Desaulniers wrote:
> On Tue, Feb 28, 2023 at 11:54 PM John Moon <quic_johmoo@quicinc.com> wrote:
>>
>> 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.
> 
> Hi John,
> Do you have the list of bugs you've filed upstream against clang wrt.
> information missing when using `-fno-eliminate-unused-debug-types`?
> 
> https://github.com/llvm/llvm-project/issues is the issue tracker.
> 
> Seeing a strong participant in both the Android and LLVM ecosystems
> supply scripts that lack clang support...raises eyebrows.

We have not filed a bug with upstream clang since we're not sure it's 
*not* and issue on our end. Giuliano Procida (CC'd) has tested the 
script with clang 13 and 14 and GCC 10, 11 and 12 and got the expected 
diff. If it's convenient for anyone testing this script to give it a 
whirl with clang and report back, it could help us determine if there's 
a real issue with clang support. :)

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[John Moon]

On 3/1/2023 10:03 AM, John Moon wrote:
> On 3/1/2023 9:50 AM, Nick Desaulniers wrote:
>> On Tue, Feb 28, 2023 at 11:54 PM John Moon <quic_johmoo@quicinc.com> 
>> wrote:
>>>
>>> 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.
>>
>> Hi John,
>> Do you have the list of bugs you've filed upstream against clang wrt.
>> information missing when using `-fno-eliminate-unused-debug-types`?
>>
>> https://github.com/llvm/llvm-project/issues is the issue tracker.
>>
>> Seeing a strong participant in both the Android and LLVM ecosystems
>> supply scripts that lack clang support...raises eyebrows.
> 
> We have not filed a bug with upstream clang since we're not sure it's 
> *not* and issue on our end. Giuliano Procida (CC'd) has tested the 
> script with clang 13 and 14 and GCC 10, 11 and 12 and got the expected 
> diff. If it's convenient for anyone testing this script to give it a 
> whirl with clang and report back, it could help us determine if there's 
> a real issue with clang support. :)

With some additional internal testing, we've found that clang does not 
work with this script on Ubuntu 18.04, but does work on Ubuntu 20.04. 
This is controlling for the clang version and different installation 
sources. The same clang-15 binary run on an 18.04 host fails while 
working on 20.04.

We'll investigate some more internally and potentially file a bug with 
upstream clang.

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[John Moon]

On 3/1/2023 11:24 AM, John Moon wrote:
> On 3/1/2023 10:03 AM, John Moon wrote:
>> On 3/1/2023 9:50 AM, Nick Desaulniers wrote:
>>> On Tue, Feb 28, 2023 at 11:54 PM John Moon <quic_johmoo@quicinc.com> 
>>> wrote:
>>>>
>>>> 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.
>>>
>>> Hi John,
>>> Do you have the list of bugs you've filed upstream against clang wrt.
>>> information missing when using `-fno-eliminate-unused-debug-types`?
>>>
>>> https://github.com/llvm/llvm-project/issues is the issue tracker.
>>>
>>> Seeing a strong participant in both the Android and LLVM ecosystems
>>> supply scripts that lack clang support...raises eyebrows.
>>
>> We have not filed a bug with upstream clang since we're not sure it's 
>> *not* and issue on our end. Giuliano Procida (CC'd) has tested the 
>> script with clang 13 and 14 and GCC 10, 11 and 12 and got the expected 
>> diff. If it's convenient for anyone testing this script to give it a 
>> whirl with clang and report back, it could help us determine if 
>> there's a real issue with clang support. :)
> 
> With some additional internal testing, we've found that clang does not 
> work with this script on Ubuntu 18.04, but does work on Ubuntu 20.04. 
> This is controlling for the clang version and different installation 
> sources. The same clang-15 binary run on an 18.04 host fails while 
> working on 20.04.
> 
> We'll investigate some more internally and potentially file a bug with 
> upstream clang.

With some additional help from Nick offline, we determined that the 
issue isn't with clang, but with libdw (from elfutils). You need at 
least libdw version 0.171 for the abidiff tool to work correctly with 
clang (in this particular case). Ubuntu 18.04 ships with version 0.170.

If there's any interest, it'd be fairly easy to add a check for this 
condition under the check_deps() function in the script.

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[Nick Desaulniers]

On Wed, Mar 1, 2023 at 2:33 PM John Moon <quic_johmoo@quicinc.com> wrote:
>
> With some additional help from Nick offline, we determined that the
> issue isn't with clang, but with libdw (from elfutils). You need at
> least libdw version 0.171 for the abidiff tool to work correctly with
> clang (in this particular case). Ubuntu 18.04 ships with version 0.170.
>
> If there's any interest, it'd be fairly easy to add a check for this
> condition under the check_deps() function in the script.

Good job John; mind sending a v3 with that addition, and the commit
message updated?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[Trilok Soni]

On 3/1/2023 3:33 PM, Nick Desaulniers wrote:
> On Wed, Mar 1, 2023 at 2:33 PM John Moon <quic_johmoo@quicinc.com> wrote:
>>
>> With some additional help from Nick offline, we determined that the
>> issue isn't with clang, but with libdw (from elfutils). You need at
>> least libdw version 0.171 for the abidiff tool to work correctly with
>> clang (in this particular case). Ubuntu 18.04 ships with version 0.170.
>>
>> If there's any interest, it'd be fairly easy to add a check for this
>> condition under the check_deps() function in the script.
> 
> Good job John; mind sending a v3 with that addition, and the commit
> message updated?

I would prefer to wait to get more reviews on v2 from Greg/Masahiro and 
then combine above checks in the v3. If there are no comments by next 
Monday then we can send v3, sounds good?

---Trilok Soni

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[Mark Wielaard]

Hi John,

On Wed, Mar 01, 2023 at 02:33:00PM -0800, John Moon via Libabigail wrote:
> With some additional help from Nick offline, we determined that the
> issue isn't with clang, but with libdw (from elfutils). You need at
> least libdw version 0.171 for the abidiff tool to work correctly
> with clang (in this particular case). Ubuntu 18.04 ships with
> version 0.170.

I don't remember any specific fixes for clang in libdw for elfutils
0.171. But elfutils 0.171 was the first release that supported most of
DWARF5 (including GNU DebugFission and split dwarf).

> If there's any interest, it'd be fairly easy to add a check for this
> condition under the check_deps() function in the script.

Please do add this check. elfutils 0.170 is almost 6 years old now,
there have been many, many bug fixes since then (current release is
0.188 from Nov 2022).

Thanks,

Mark

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[John Moon]

On 3/1/2023 3:52 PM, Mark Wielaard wrote:
> Hi John,
> 
> On Wed, Mar 01, 2023 at 02:33:00PM -0800, John Moon via Libabigail wrote:
>> With some additional help from Nick offline, we determined that the
>> issue isn't with clang, but with libdw (from elfutils). You need at
>> least libdw version 0.171 for the abidiff tool to work correctly
>> with clang (in this particular case). Ubuntu 18.04 ships with
>> version 0.170.
> 
> I don't remember any specific fixes for clang in libdw for elfutils
> 0.171. But elfutils 0.171 was the first release that supported most of
> DWARF5 (including GNU DebugFission and split dwarf).
> 
>> If there's any interest, it'd be fairly easy to add a check for this
>> condition under the check_deps() function in the script.
> 
> Please do add this check. elfutils 0.170 is almost 6 years old now,
> there have been many, many bug fixes since then (current release is
> 0.188 from Nov 2022).
> 
> Thanks,
> 
> Mark

Okay, I'll add the check in v3 after giving a chance for some other 
reviews. Thanks!

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

[Dodji Seketeli]

Hello John,

John Moon via Libabigail <libabigail@sourceware.org> a écrit:

> 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 commit 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 commit 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."
>
> The script also includes the ability to check the compatibilty of
> all UAPI headers across commits. This allows developers to inspect
> the stability of the UAPIs over time.

Thank you for working on this.

The libabigail bits look good to me, for what it's worth.  I just have
some general considerations to discuss.

[...]

> +# Perform the A/B compilation and compare output ABI
> +compare_abi() {

[...]

> +	if "$ABIDIFF" --non-reachable-types "${ref_header}.bin" "${base_header}.bin" > "$log"; then
> +		printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$ref" "${base_ref:-dirty tree}"
> +	else
> +		# If the only changes were additions (not modifications to existing APIs), then
> +		# there's no problem. Ignore these diffs.
> +		if grep "Unreachable types summary" "$log" | grep -q "0 removed" &&
> +		   grep "Unreachable types summary" "$log" | grep -q "0 changed"; then
> +			return 0

There is no problem in parsing the output of the tool like this.
However, the return code of the tool has been designed as a bit field that
could be analysed to know more about the kind of changes that were
reported: https://sourceware.org/libabigail/manual/abidiff.html#return-values.

Right now, there is no bit assigned to detect new types (or interface)
addition, but do you think that it would be a helpful new feature to add
to abidiff for this use case?  We can discuss this in a separate thread
if you prefer, so that we don't pollute others with this minutiae.

> +		fi
> +		{
> +			printf "!!! ABI differences detected in %s from %s -> %s !!!\n\n" "$file" "$ref" "${base_ref:-dirty tree}"
> +			sed  -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/  /g' "$log"

Here again, if you'd like to have a particular output format emitted by
the tool, we'd be glad to discuss how to improve the plasticity of the
tool enough to emit the right output for you.  For instance, we could
add a new --no-summary that would let the tool display the change
directly without the summary header that you are strimming out with this
sed script.

[...]

Thanks again for this tool that I think might be very useful.

Cheers,

-- 
		Dodji

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

[Masahiro Yamada]

On Wed, Mar 1, 2023 at 4:54 PM 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 commit 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 commit 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."
>
> The script also includes the ability to check the compatibilty of
> all UAPI headers across commits. This allows developers to inspect
> the stability of the UAPIs over time.


Let's see more test cases.


[Case 1]

I think d759be8953febb6e5b5376c7d9bbf568864c6e2d
is a trivial/good cleanup.
Apparently, it still exports equivalent headers,
but this tool reports "incorrectly removed".



$ ./scripts/check-uapi.sh -b d759be8953
Saving current tree state... OK
Installing sanitized UAPI headers from d759be8953... OK
Installing sanitized UAPI headers from d759be8953^1... OK
Restoring current tree state... OK
Checking changes to UAPI headers starting from d759be8953
error - UAPI header arch/alpha/include/uapi/asm/poll.h was incorrectly removed
error - UAPI header arch/ia64/include/uapi/asm/poll.h was incorrectly removed
error - UAPI header arch/x86/include/uapi/asm/poll.h was incorrectly removed
/tmp/tmp.ixUIBlntUP/d759be8953/x86/usr/include/asm/Kbuild does not
exist - cannot compare ABI
/tmp/tmp.ixUIBlntUP/d759be8953/alpha/usr/include/asm/Kbuild does not
exist - cannot compare ABI
/tmp/tmp.ixUIBlntUP/d759be8953/ia64/usr/include/asm/Kbuild does not
exist - cannot compare ABI
error - 6/6 UAPI headers modified between d759be8953^1 and d759be8953
are not backwards compatible
error - UAPI header ABI check failed
Failure summary saved to /home/masahiro/ref/linux/abi_error_log.txt



[Case 2]

This tool compiles only changed headers.
Does it detect ABI change?

I believe the users of the headers must be compiled.



Think about this case.


$ cat foo-typedef.h
typedef int foo_cap_type;


$ cat foo.h
#include "foo-typedef.h"

struct foo {
       foo_cap_type capability;
};



Then, change the first header to
  typedef long long foo_cap_type;

abidiff will never notice the ABI change
until it compiles "foo.h" instead of "foo-typedef.h"



For testing, I applied the following patch.


 --- a/include/uapi/linux/types.h
 +++ b/include/uapi/linux/types.h
 @@ -52,7 +52,7 @@ typedef __u32 __bitwise __wsum;
  #define __aligned_be64 __be64 __attribute__((aligned(8)))
  #define __aligned_le64 __le64 __attribute__((aligned(8)))

 -typedef unsigned __bitwise __poll_t;
 +typedef unsigned short __bitwise __poll_t;

  #endif /*  __ASSEMBLY__ */
  #endif /* _UAPI_LINUX_TYPES_H */




I believe this is an ABI change because this will change
'struct epoll_event' in the include/uapi/linux/eventpoll.h
but the tool happily reports it is backwards compatible.


$ ./scripts/check-uapi.sh
Saving current tree state... OK
Installing sanitized UAPI headers from HEAD... OK
Installing sanitized UAPI headers from HEAD^1... OK
Restoring current tree state... OK
Checking changes to UAPI headers starting from HEAD
No ABI differences detected in include/uapi/linux/types.h from HEAD^1 -> HEAD
All 1 UAPI headers modified between HEAD^1 and HEAD are backwards compatible!





I would not use such a tool that contains both false positives
and false negatives, but you may notice this is more difficult
than you had expected.

I do not know if further review is worthwhile since this does not work
but I added some more in-line comments.






> +
> +# Some UAPI headers require an architecture-specific compiler to build properly.
> +ARCH_SPECIFIC_CC_NEEDED=(
> +       "arch/hexagon/include/uapi/asm/sigcontext.h"
> +       "arch/ia64/include/uapi/asm/intel_intrin.h"
> +       "arch/ia64/include/uapi/asm/setup.h"
> +       "arch/ia64/include/uapi/asm/sigcontext.h"
> +       "arch/mips/include/uapi/asm/bitfield.h"
> +       "arch/mips/include/uapi/asm/byteorder.h"
> +       "arch/mips/include/uapi/asm/inst.h"
> +       "arch/sparc/include/uapi/asm/fbio.h"
> +       "arch/sparc/include/uapi/asm/uctx.h"
> +       "arch/xtensa/include/uapi/asm/byteorder.h"
> +       "arch/xtensa/include/uapi/asm/msgbuf.h"
> +       "arch/xtensa/include/uapi/asm/sembuf.h"
> +)


Yes, arch/*/include/ must be compiled by the target compiler.
If you compile them by the host compiler, it is unpredictable (i.e. wrong).

BTW, was this blacklist detected on a x86 host?

If you do this on an ARM/ARM64 host, some headers
under arch/x86/include/uapi/ might be blacklisted?



> +# Compile the simple test app
> +do_compile() {
> +       local -r inc_dir="$1"
> +       local -r header="$2"
> +       local -r out="$3"
> +       printf "int main(void) { return 0; }\n" | \
> +               "${CC:-gcc}" -c \
> +                 -o "$out" \
> +                 -x c \
> +                 -O0 \
> +                 -std=c90 \
> +                 -fno-eliminate-unused-debug-types \
> +                 -g \
> +                 "-I${inc_dir}" \
> +                 -include "$header" \
> +                 -
> +}
> +
> +# Print the list of incompatible headers from the usr/include Makefile
> +get_no_header_list() {
> +       {
> +               # shellcheck disable=SC2016
> +               printf 'all: ; @echo $(no-header-test)\n'
> +               cat "usr/include/Makefile"

You must pass SRCARCH=$arch.

Otherwise,

ifeq ($(SRCARCH),...)
  ...
endif

are all skipped.





> +       } | make -f - | tr " " "\n" | grep -v "asm-generic"
> +
> +       # One additional header file is not building correctly
> +       # with this method.
> +       # TODO: why can't we build this one?
> +       printf "asm-generic/ucontext.h\n"


Answer - it is not intended for standalone compiling in the first place.

<asm-generic/*.h> should be included from <asm/*.h>.

Userspace never ever includes <asm-generic/*.h> directly.
(If it does, it is a bug in the userspace program)

I am afraid you read user/include/Makefile wrongly.




> +
> +# Install headers for every arch and ref we need
> +install_headers() {
> +       local -r check_all="$1"
> +       local -r base_ref="$2"
> +       local -r ref="$3"
> +
> +       local arch_list=()
> +       while read -r status file; do
> +               if arch="$(printf "%s" "$file" | grep -o 'arch/.*/uapi' | cut -d '/' -f 2)"; then
> +                       # shellcheck disable=SC2076
> +                       if ! [[ " ${arch_list[*]} " =~ " $arch " ]]; then
> +                               arch_list+=("$arch")
> +                       fi
> +               fi
> +       done < <(get_uapi_files "$check_all" "$base_ref" "$ref")
> +
> +       deviated_from_current_tree="false"
> +       for inst_ref in "$base_ref" "$ref"; do
> +               if [ -n "$inst_ref" ]; then
> +                       if [ "$deviated_from_current_tree" = "false" ]; then
> +                               save_tree_state
> +                               trap 'rm -rf "$tmp_dir"; restore_tree_state;' EXIT
> +                               deviated_from_current_tree="true"
> +                       fi
> +                       git checkout --quiet "$(git rev-parse "$inst_ref")"


I might be wrong, but I was worried when I looked at this line
because git-checkout may change the running code
if check-uapi.sh is changed between ref and base_ref.

If bash always loads all code into memory before running
it is safe but I do not know how it works.


If this is safe, some comments might be worthwhile:

    # 'git checkout' may update this script itself while running,
    # but it is OK because ...





> +
> +# Make sure we have the tools we need
> +check_deps() {
> +       export ABIDIFF="${ABIDIFF:-abidiff}"
> +
> +       if ! command -v "$ABIDIFF" > /dev/null 2>&1; then
> +               eprintf "error - abidiff not found!\n"
> +               eprintf "Please install abigail-tools (version 1.7 or greater)\n"
> +               eprintf "See: https://sourceware.org/libabigail/manual/libabigail-overview.html\n"
> +               exit 1
> +       fi
> +
> +       read -r abidiff_maj abidiff_min _unused < <("$ABIDIFF" --version | cut -d ' ' -f 2 | tr '.' ' ')
> +       if [ "$abidiff_maj" -lt 1 ] || { [ "$abidiff_maj" -eq 1 ] && [ "$abidiff_min" -lt 7 ]; }; then


This is up to you, but I think "sort -V" would be cleaner.
(see Documentation/devicetree/bindings/Makefile for example)




> +       fi
> +
> +       if [ ! -x "scripts/unifdef" ]; then
> +               if ! make -f /dev/null scripts/unifdef; then

Previously, I wanted to point out that using Make is meaningless,
and using gcc directly is better.


But, is this still necessary?

V2 uses 'make headers_install' to install all headers.
scripts/unifdef is not used anywhere in this script.






> +
> +       abi_error_log="${abi_error_log:-${KERNEL_SRC}/abi_error_log.txt}"
> +
> +       check_deps
> +
> +       tmp_dir=$(mktemp -d)
> +       trap 'rm -rf "$tmp_dir"' EXIT
> +
> +       # Set of UAPI directories to check by default
> +       UAPI_DIRS=(include/uapi arch/*/include/uapi)
> +
> +       if ! git rev-parse --is-inside-work-tree > /dev/null 2>&1; then
> +               eprintf "error - this script requires the kernel tree to be initialized with Git\n"
> +               exit 1
> +       fi
> +
> +       # If there are no dirty UAPI files, use HEAD as base_ref
> +       if [ -z "$base_ref" ] && [ "$(get_uapi_files "" "" | wc -l)" -eq 0 ]; then
> +               base_ref="HEAD"
> +       fi
> +
> +       if [ -z "$ref_to_check" ]; then
> +               if [ -n "$base_ref" ]; then
> +                       ref_to_check="${base_ref}^1"
> +               else
> +                       ref_to_check="HEAD"
> +               fi
> +       fi


I think this is because I am not good at English, but
I was so confused between 'base_ref' vs 'ref_to_check'.
I do not get which one is the ancestor from the names.

I thought 'ref_a' and 'ref_b' would be less confusing,
but I hope somebody will come up with better naming
than that.




--
Best Regards





Masahiro Yamada

Re: [PATCH v2 2/2] docs: dev-tools: Add UAPI checker documentation

[Masahiro Yamada]

On Wed, Mar 1, 2023 at 4:54 PM John Moon <quic_johmoo@quicinc.com> wrote:
>
> Add detailed documentation for scripts/check-uapi.sh.
> ---
>  Documentation/dev-tools/checkuapi.rst | 258 ++++++++++++++++++++++++++
>  Documentation/dev-tools/index.rst     |   1 +
>  2 files changed, 259 insertions(+)
>  create mode 100644 Documentation/dev-tools/checkuapi.rst
>
> diff --git a/Documentation/dev-tools/checkuapi.rst b/Documentation/dev-tools/checkuapi.rst
> new file mode 100644
> index 000000000000..2255066658e3
> --- /dev/null
> +++ b/Documentation/dev-tools/checkuapi.rst
> @@ -0,0 +1,258 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +============
> +UAPI Checker
> +============
> +
> +The UAPI checker (scripts/check-uapi.sh) is a shell script which checks UAPI
> +header files for userspace backwards-compatibility across the git tree.
> +
> +The script can produce false positives in some cases, so developers are


and false negatives too.







-- 
Best Regards
Masahiro Yamada

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

[John Moon]

On 3/4/2023 8:22 PM, Masahiro Yamada wrote:
> On Wed, Mar 1, 2023 at 4:54 PM 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 commit 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 commit 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."
>>
>> The script also includes the ability to check the compatibilty of
>> all UAPI headers across commits. This allows developers to inspect
>> the stability of the UAPIs over time.
> 
> 
> Let's see more test cases.
> 
> 
> [Case 1]
> 
> I think d759be8953febb6e5b5376c7d9bbf568864c6e2d
> is a trivial/good cleanup.
> Apparently, it still exports equivalent headers,
> but this tool reports "incorrectly removed".
> 
> 
> 
> $ ./scripts/check-uapi.sh -b d759be8953
> Saving current tree state... OK
> Installing sanitized UAPI headers from d759be8953... OK
> Installing sanitized UAPI headers from d759be8953^1... OK
> Restoring current tree state... OK
> Checking changes to UAPI headers starting from d759be8953
> error - UAPI header arch/alpha/include/uapi/asm/poll.h was incorrectly removed
> error - UAPI header arch/ia64/include/uapi/asm/poll.h was incorrectly removed
> error - UAPI header arch/x86/include/uapi/asm/poll.h was incorrectly removed
> /tmp/tmp.ixUIBlntUP/d759be8953/x86/usr/include/asm/Kbuild does not
> exist - cannot compare ABI
> /tmp/tmp.ixUIBlntUP/d759be8953/alpha/usr/include/asm/Kbuild does not
> exist - cannot compare ABI
> /tmp/tmp.ixUIBlntUP/d759be8953/ia64/usr/include/asm/Kbuild does not
> exist - cannot compare ABI
> error - 6/6 UAPI headers modified between d759be8953^1 and d759be8953
> are not backwards compatible
> error - UAPI header ABI check failed
> Failure summary saved to /home/masahiro/ref/linux/abi_error_log.txt
> 
> 

This is an interesting test case. Thanks for bringing it up. I don't 
know if there's a way for the script to filter out these kinds of 
changes, so it may just need to be noted under possible false positives 
in the document.

It also reveals that the script isn't filtering out non-headers from the 
git diffs... I'll fix that in v3.

> 
> [Case 2]
> 
> This tool compiles only changed headers.
> Does it detect ABI change?
> 
> I believe the users of the headers must be compiled.
> 
> 
> 
> Think about this case.
> 
> 
> $ cat foo-typedef.h
> typedef int foo_cap_type;
> 
> 
> $ cat foo.h
> #include "foo-typedef.h"
> 
> struct foo {
>         foo_cap_type capability;
> };
> 
> 
> 
> Then, change the first header to
>    typedef long long foo_cap_type;
> 
> abidiff will never notice the ABI change
> until it compiles "foo.h" instead of "foo-typedef.h"
>  >
> 
> For testing, I applied the following patch.
> 
> 
>   --- a/include/uapi/linux/types.h
>   +++ b/include/uapi/linux/types.h
>   @@ -52,7 +52,7 @@ typedef __u32 __bitwise __wsum;
>    #define __aligned_be64 __be64 __attribute__((aligned(8)))
>    #define __aligned_le64 __le64 __attribute__((aligned(8)))
> 
>   -typedef unsigned __bitwise __poll_t;
>   +typedef unsigned short __bitwise __poll_t;
> 
>    #endif /*  __ASSEMBLY__ */
>    #endif /* _UAPI_LINUX_TYPES_H */
> 
> 
> 
> 
> I believe this is an ABI change because this will change
> 'struct epoll_event' in the include/uapi/linux/eventpoll.h
> but the tool happily reports it is backwards compatible.
> 
> 
> $ ./scripts/check-uapi.sh
> Saving current tree state... OK
> Installing sanitized UAPI headers from HEAD... OK
> Installing sanitized UAPI headers from HEAD^1... OK
> Restoring current tree state... OK
> Checking changes to UAPI headers starting from HEAD
> No ABI differences detected in include/uapi/linux/types.h from HEAD^1 -> HEAD
> All 1 UAPI headers modified between HEAD^1 and HEAD are backwards compatible!
> 
> 

You're correct, this case is missed when only checking modified headers. 
With the same patch, if I pass "-a" to the script, it does catch the change:

% ./scripts/check-uapi.sh -a
--- snip ---
error - 1/1328 UAPI headers modified between HEAD and dirty tree are not 
backwards compatible
error - UAPI header ABI check failed


% cat abi_error_log.txt
Generated by "./scripts/check-uapi.sh -a" from git ref 
94b1166f7954f1136f307dafbaad5f9d871b73bf

!!! ABI differences detected in include/uapi/linux/eventpoll.h from HEAD 
-> dirty tree !!!

     [C] 'struct epoll_event' changed:
       type size changed from 96 to 80 (in bits)
       2 data member changes:
         type of '__poll_t events' changed:
           underlying type 'unsigned int' changed:
             type name changed from 'unsigned int' to 'unsigned short int'
             type size changed from 32 to 16 (in bits)
         '__u64 data' offset changed from 32 to 16 (in bits) (by -16 bits)

Perhaps in the next revision we could add some way to detect these 
dependencies (e.g. if foo.h includes bar.h, and bar.h was modified, we 
should check foo.h). However, the time savings may not be worth the 
complicated and potentially fragile dependency detection.

For now, "-a" should catch this, and it only took about 1 minute to run 
through all the headers on my 8-core machine, so it should be a 
resonable test step for a CI system.

> 
> 
> 
> I would not use such a tool that contains both false positives
> and false negatives, but you may notice this is more difficult
> than you had expected.
> 

Right, it certainly has its shortcomings. I appreciate you helping us 
find and address them! Even in its current state, I believe the script 
has value for developers and reviewers. :)

> I do not know if further review is worthwhile since this does not work
> but I added some more in-line comments.
> 
> 
> 
> 
> 
> 
>> +
>> +# Some UAPI headers require an architecture-specific compiler to build properly.
>> +ARCH_SPECIFIC_CC_NEEDED=(
>> +       "arch/hexagon/include/uapi/asm/sigcontext.h"
>> +       "arch/ia64/include/uapi/asm/intel_intrin.h"
>> +       "arch/ia64/include/uapi/asm/setup.h"
>> +       "arch/ia64/include/uapi/asm/sigcontext.h"
>> +       "arch/mips/include/uapi/asm/bitfield.h"
>> +       "arch/mips/include/uapi/asm/byteorder.h"
>> +       "arch/mips/include/uapi/asm/inst.h"
>> +       "arch/sparc/include/uapi/asm/fbio.h"
>> +       "arch/sparc/include/uapi/asm/uctx.h"
>> +       "arch/xtensa/include/uapi/asm/byteorder.h"
>> +       "arch/xtensa/include/uapi/asm/msgbuf.h"
>> +       "arch/xtensa/include/uapi/asm/sembuf.h"
>> +)
> 
> 
> Yes, arch/*/include/ must be compiled by the target compiler.
> If you compile them by the host compiler, it is unpredictable (i.e. wrong).
> 
> BTW, was this blacklist detected on a x86 host?
> 

Yes.

> If you do this on an ARM/ARM64 host, some headers
> under arch/x86/include/uapi/ might be blacklisted?
> 

Good point - I missed those!

> 
> 
>> +# Compile the simple test app
>> +do_compile() {
>> +       local -r inc_dir="$1"
>> +       local -r header="$2"
>> +       local -r out="$3"
>> +       printf "int main(void) { return 0; }\n" | \
>> +               "${CC:-gcc}" -c \
>> +                 -o "$out" \
>> +                 -x c \
>> +                 -O0 \
>> +                 -std=c90 \
>> +                 -fno-eliminate-unused-debug-types \
>> +                 -g \
>> +                 "-I${inc_dir}" \
>> +                 -include "$header" \
>> +                 -
>> +}
>> +
>> +# Print the list of incompatible headers from the usr/include Makefile
>> +get_no_header_list() {
>> +       {
>> +               # shellcheck disable=SC2016
>> +               printf 'all: ; @echo $(no-header-test)\n'
>> +               cat "usr/include/Makefile"
> 
> You must pass SRCARCH=$arch.
> 
> Otherwise,
> 
> ifeq ($(SRCARCH),...)
>    ...
> endif
> 
> are all skipped.
> 
> 

Thanks for the tip, that explains it. Should be able to address this in v3.

> 
> 
> 
>> +       } | make -f - | tr " " "\n" | grep -v "asm-generic"
>> +
>> +       # One additional header file is not building correctly
>> +       # with this method.
>> +       # TODO: why can't we build this one?
>> +       printf "asm-generic/ucontext.h\n"
> 
> 
> Answer - it is not intended for standalone compiling in the first place.
> 
> <asm-generic/*.h> should be included from <asm/*.h>.
> 
> Userspace never ever includes <asm-generic/*.h> directly.
> (If it does, it is a bug in the userspace program)
> 
> I am afraid you read user/include/Makefile wrongly.
> 
> 

Understood. I think I had misinterpreted one of your comments on v1, but 
now I'm clear. Will address in v3.

> 
> 
>> +
>> +# Install headers for every arch and ref we need
>> +install_headers() {
>> +       local -r check_all="$1"
>> +       local -r base_ref="$2"
>> +       local -r ref="$3"
>> +
>> +       local arch_list=()
>> +       while read -r status file; do
>> +               if arch="$(printf "%s" "$file" | grep -o 'arch/.*/uapi' | cut -d '/' -f 2)"; then
>> +                       # shellcheck disable=SC2076
>> +                       if ! [[ " ${arch_list[*]} " =~ " $arch " ]]; then
>> +                               arch_list+=("$arch")
>> +                       fi
>> +               fi
>> +       done < <(get_uapi_files "$check_all" "$base_ref" "$ref")
>> +
>> +       deviated_from_current_tree="false"
>> +       for inst_ref in "$base_ref" "$ref"; do
>> +               if [ -n "$inst_ref" ]; then
>> +                       if [ "$deviated_from_current_tree" = "false" ]; then
>> +                               save_tree_state
>> +                               trap 'rm -rf "$tmp_dir"; restore_tree_state;' EXIT
>> +                               deviated_from_current_tree="true"
>> +                       fi
>> +                       git checkout --quiet "$(git rev-parse "$inst_ref")"
> 
> 
> I might be wrong, but I was worried when I looked at this line
> because git-checkout may change the running code
> if check-uapi.sh is changed between ref and base_ref.
> 
> If bash always loads all code into memory before running
> it is safe but I do not know how it works.
> 
> 
> If this is safe, some comments might be worthwhile:
> 
>      # 'git checkout' may update this script itself while running,
>      # but it is OK because ...
> 

Yes, my understanding is that since the script is all encapsulated in 
functions, the shell has loaded all of the functions before execution 
starts. My testing has shown this to be safe as well. Will add a comment 
in v3.

> 
> 
> 
> 
>> +
>> +# Make sure we have the tools we need
>> +check_deps() {
>> +       export ABIDIFF="${ABIDIFF:-abidiff}"
>> +
>> +       if ! command -v "$ABIDIFF" > /dev/null 2>&1; then
>> +               eprintf "error - abidiff not found!\n"
>> +               eprintf "Please install abigail-tools (version 1.7 or greater)\n"
>> +               eprintf "See: https://sourceware.org/libabigail/manual/libabigail-overview.html\n"
>> +               exit 1
>> +       fi
>> +
>> +       read -r abidiff_maj abidiff_min _unused < <("$ABIDIFF" --version | cut -d ' ' -f 2 | tr '.' ' ')
>> +       if [ "$abidiff_maj" -lt 1 ] || { [ "$abidiff_maj" -eq 1 ] && [ "$abidiff_min" -lt 7 ]; }; then
> 
> 
> This is up to you, but I think "sort -V" would be cleaner.
> (see Documentation/devicetree/bindings/Makefile for example)
> 
> 

Noted.

> 
> 
>> +       fi
>> +
>> +       if [ ! -x "scripts/unifdef" ]; then
>> +               if ! make -f /dev/null scripts/unifdef; then
> 
> Previously, I wanted to point out that using Make is meaningless,
> and using gcc directly is better.
> 
> 
> But, is this still necessary?
> 
> V2 uses 'make headers_install' to install all headers.
> scripts/unifdef is not used anywhere in this script.
> 
> 

Ah, you're right it is not necessary. Previously, we were calling 
headers_install.sh directly, so make wasn't there to supply the unifdef 
dependency. Will remove this in v3.

> 
> 
> 
> 
>> +
>> +       abi_error_log="${abi_error_log:-${KERNEL_SRC}/abi_error_log.txt}"
>> +
>> +       check_deps
>> +
>> +       tmp_dir=$(mktemp -d)
>> +       trap 'rm -rf "$tmp_dir"' EXIT
>> +
>> +       # Set of UAPI directories to check by default
>> +       UAPI_DIRS=(include/uapi arch/*/include/uapi)
>> +
>> +       if ! git rev-parse --is-inside-work-tree > /dev/null 2>&1; then
>> +               eprintf "error - this script requires the kernel tree to be initialized with Git\n"
>> +               exit 1
>> +       fi
>> +
>> +       # If there are no dirty UAPI files, use HEAD as base_ref
>> +       if [ -z "$base_ref" ] && [ "$(get_uapi_files "" "" | wc -l)" -eq 0 ]; then
>> +               base_ref="HEAD"
>> +       fi
>> +
>> +       if [ -z "$ref_to_check" ]; then
>> +               if [ -n "$base_ref" ]; then
>> +                       ref_to_check="${base_ref}^1"
>> +               else
>> +                       ref_to_check="HEAD"
>> +               fi
>> +       fi
> 
> 
> I think this is because I am not good at English, but
> I was so confused between 'base_ref' vs 'ref_to_check'.
> I do not get which one is the ancestor from the names.
> 
> I thought 'ref_a' and 'ref_b' would be less confusing,
> but I hope somebody will come up with better naming
> than that.
> 

Agreed, I think this is a confusing case for native English-speakers too. :)

I want to indicate that one ref has to come after the other in the Git 
tree. Maybe "base_ref" and "past_ref"? We'll think on it.

> 
> 
> 
> --
> Best Regards
> 
> 
> 
> 
> 
> Masahiro Yamada

Thank you again for the detailed review!

Re: [PATCH v2 2/2] docs: dev-tools: Add UAPI checker documentation

[John Moon]

On 3/4/2023 10:20 PM, Masahiro Yamada wrote:
> On Wed, Mar 1, 2023 at 4:54 PM John Moon <quic_johmoo@quicinc.com> wrote:
>>
>> Add detailed documentation for scripts/check-uapi.sh.
>> ---
>>   Documentation/dev-tools/checkuapi.rst | 258 ++++++++++++++++++++++++++
>>   Documentation/dev-tools/index.rst     |   1 +
>>   2 files changed, 259 insertions(+)
>>   create mode 100644 Documentation/dev-tools/checkuapi.rst
>>
>> diff --git a/Documentation/dev-tools/checkuapi.rst b/Documentation/dev-tools/checkuapi.rst
>> new file mode 100644
>> index 000000000000..2255066658e3
>> --- /dev/null
>> +++ b/Documentation/dev-tools/checkuapi.rst
>> @@ -0,0 +1,258 @@
>> +.. SPDX-License-Identifier: GPL-2.0-only
>> +
>> +============
>> +UAPI Checker
>> +============
>> +
>> +The UAPI checker (scripts/check-uapi.sh) is a shell script which checks UAPI
>> +header files for userspace backwards-compatibility across the git tree.
>> +
>> +The script can produce false positives in some cases, so developers are
> 
> 
> and false negatives too.
> 

Yes, I'll expand the notes to include examples of false negatives in v3.

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[Christoph Hellwig]

On Tue, Feb 28, 2023 at 11:54:00PM -0800, John Moon wrote:
> 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.

Why would this matter in any way for the kernel?  If you tool is useful
for in-kernel usage it should be added to the tree and documented as
such, but ouf of tree crap simply does not matter.

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[Trilok Soni]

On 3/10/2023 12:09 AM, Christoph Hellwig wrote:
> On Tue, Feb 28, 2023 at 11:54:00PM -0800, John Moon wrote:
>> 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.
> 
> Why would this matter in any way for the kernel?  If you tool is useful
> for in-kernel usage it should be added to the tree and documented as
> such, but ouf of tree crap simply does not matter.

This tool will be helpful for the kernel maintainers and reviewers as 
well if it can detect potential UAPI backward compatibilities. Even for 
the developers while changing UAPI interfaces at kernel.org before 
submission.

John is trying to highlight also that this tool can be useful for 
downstream users who want to keep the UAPI backward compatibility like 
we do at upstream. We can remove the above text, since we would like to 
mainline it at kernel.org.

---Trilok Soni

Re: [PATCH v2 0/2] Validating UAPI backwards compatibility

[Christoph Hellwig]

On Fri, Mar 10, 2023 at 10:20:14AM -0800, Trilok Soni wrote:
> This tool will be helpful for the kernel maintainers and reviewers as well
> if it can detect potential UAPI backward compatibilities. Even for the
> developers while changing UAPI interfaces at kernel.org before submission.

So document it as that.