Messages in this thread | | | Date | Wed, 22 Feb 2023 13:06:14 -0800 | Subject | Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh | From | 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
| |