All of lore.kernel.org
 help / color / mirror / Atom feed
From: Giuliano Procida <gprocida@google.com>
To: quic_johmoo@quicinc.com
Cc: masahiroy@kernel.org, nathan@kernel.org, ndesaulniers@google.com,
	nicolas@fjasle.eu, linux-kbuild@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, gregkh@linuxfoundation.org,
	rdunlap@infradead.org, arnd@arndb.de, andersson@kernel.org,
	tkjos@google.com, maennich@google.com, gprocida@google.com,
	kernel-team@android.com, libabigail@sourceware.org,
	jorcrous@amazon.com, quic_tsoni@quicinc.com,
	quic_satyap@quicinc.com, quic_eberman@quicinc.com,
	quic_gurus@quicinc.com
Subject: [PATCH] scripts/check-uapi.sh: add stgdiff support
Date: Thu, 20 Jul 2023 17:10:53 +0100	[thread overview]
Message-ID: <20230720161053.1213680-1-gprocida@google.com> (raw)
In-Reply-To: <20230407203456.27141-2-quic_johmoo@quicinc.com>

Hi John.

I spent a few minutes adding stgdiff support to the script. It's
really just for illustration purposes.

As I think you know, STG doesn't yet exist as a project outside of
AOSP. Nevertheless, this may be useful to you as-is.

STG has quite a different philosophy to libabigil in terms of
filtering out certain kinds of differences. Some of the things (like
enum enumerator additions) are not considered harmless. The reasoning
behind this is basically...
https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)

However, it does have --ignore interface_addition (and the related
--ignore type_definition_addition) which can be used to detect whether
one ABI is a subset of another.

I am looking at adding support for macro definitions (gcc -g3) to STG
which will then let us cover significantly more of the UAPI surface.

Unfortunately, there are some headers which use anonymous enums to
define constants (e.g. and ironically BTF). ABI tracking these types
would require something a bit hacky. Or we could just name them.

Regards,
Giuliano.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 scripts/check-uapi.sh | 102 ++++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 33 deletions(-)

diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh
index 755187f27be5..982666b48f3b 100755
--- a/scripts/check-uapi.sh
+++ b/scripts/check-uapi.sh
@@ -32,6 +32,7 @@ Options:
     -v             Verbose operation (print more information about each header being checked).
 
 Environmental args:
+    STGDIFF  Custom path to stgdiff binary - use stgdiff instead of abidiff
     ABIDIFF  Custom path to abidiff binary
     CC       C compiler (default is "gcc")
     ARCH     Target architecture of C compiler (default is host arch)
@@ -270,43 +271,78 @@ compare_abi() {
 		exit "$FAIL_COMPILE"
 	fi
 
-	local ret=0
-	"$ABIDIFF" --non-reachable-types "${past_header}.bin" "${base_header}.bin" > "$log" || ret="$?"
-	if [ "$ret" -eq 0 ]; then
-		if [ "$VERBOSE" = "true" ]; then
-			printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+	if [ "$STGDIFF" ]; then
+		local ret=0
+		"$STGDIFF" --types --ignore interface_addition --elf "${past_header}.bin" "${base_header}.bin" --format small --output "$log" || ret="$?"
+		if [ "$ret" -eq 0 ]; then
+			if [ "$VERBOSE" = "true" ]; then
+				printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+			fi
+		else
+			# stgdiff's return code can be used to determine the type of error
+			if [ $((ret & 0x1)) -gt 0 ]; then
+				eprintf "error - stgdiff failed\n"
+				exit 1
+			fi
+
+			{
+				printf "!!! ABI differences detected in %s from %s -> %s !!!\n\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+				sed  -e 's/^/  /g' "$log"
+
+				if ! cmp "$past_header" "$base_header" > /dev/null 2>&1; then
+					printf "\nHeader file diff (after headers_install):\n"
+					diff -Naur "$past_header" "$base_header" \
+						| sed -e "s|${past_header}|${past_ref}/${file}|g" \
+						      -e "s|${base_header}|${base_ref:-dirty}/${file}|g"
+					printf "\n"
+				else
+					printf "\n%s did not change between %s and %s...\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+					printf "It's possible a change to one of the headers it includes caused this error:\n"
+					grep '^#include' "$base_header"
+					printf "\n"
+				fi
+			} | tee "${base_header}.error" >&2
+			return 1
 		fi
 	else
-		# Bits in abidiff's return code can be used to determine the type of error
-		if [ $((ret & 0x1)) -gt 0 ]; then
-			eprintf "error - abidiff did not run properly\n"
-			exit 1
-		fi
+		local ret=0
+		"$ABIDIFF" --non-reachable-types "${past_header}.bin" "${base_header}.bin" > "$log" || ret="$?"
+		if [ "$ret" -eq 0 ]; then
+			if [ "$VERBOSE" = "true" ]; then
+				printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+			fi
+		else
+			# Bits in abidiff's return code can be used to determine the type of error
+			if [ $((ret & 0x1)) -gt 0 ]; then
+				eprintf "error - abidiff did not run properly\n"
+				exit 1
+			fi
 
-		# 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" "$past_ref" "${base_ref:-dirty tree}"
-			sed  -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/  /g' "$log"
-
-			if ! cmp "$past_header" "$base_header" > /dev/null 2>&1; then
-				printf "\nHeader file diff (after headers_install):\n"
-				diff -Naur "$past_header" "$base_header" \
-					| sed -e "s|${past_header}|${past_ref}/${file}|g" \
-					      -e "s|${base_header}|${base_ref:-dirty}/${file}|g"
-				printf "\n"
-			else
-				printf "\n%s did not change between %s and %s...\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
-				printf "It's possible a change to one of the headers it includes caused this error:\n"
-				grep '^#include' "$base_header"
-				printf "\n"
+			# 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
-		} | tee "${base_header}.error" >&2
-		return 1
+			{
+				printf "!!! ABI differences detected in %s from %s -> %s !!!\n\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+				sed  -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/  /g' "$log"
+
+				if ! cmp "$past_header" "$base_header" > /dev/null 2>&1; then
+					printf "\nHeader file diff (after headers_install):\n"
+					diff -Naur "$past_header" "$base_header" \
+						| sed -e "s|${past_header}|${past_ref}/${file}|g" \
+						      -e "s|${base_header}|${base_ref:-dirty}/${file}|g"
+					printf "\n"
+				else
+					printf "\n%s did not change between %s and %s...\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+					printf "It's possible a change to one of the headers it includes caused this error:\n"
+					grep '^#include' "$base_header"
+					printf "\n"
+				fi
+			} | tee "${base_header}.error" >&2
+			return 1
+		fi
 	fi
 }
 
-- 
2.41.0.255.g8b1d071c50-goog


WARNING: multiple messages have this Message-ID (diff)
From: Giuliano Procida <gprocida@google.com>
To: quic_johmoo@quicinc.com
Cc: masahiroy@kernel.org, nathan@kernel.org, ndesaulniers@google.com,
	 nicolas@fjasle.eu, linux-kbuild@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	 linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org,  gregkh@linuxfoundation.org,
	rdunlap@infradead.org, arnd@arndb.de,  andersson@kernel.org,
	tkjos@google.com, maennich@google.com,  gprocida@google.com,
	kernel-team@android.com, libabigail@sourceware.org,
	 jorcrous@amazon.com, quic_tsoni@quicinc.com,
	quic_satyap@quicinc.com,  quic_eberman@quicinc.com,
	quic_gurus@quicinc.com
Subject: [PATCH] scripts/check-uapi.sh: add stgdiff support
Date: Thu, 20 Jul 2023 17:10:53 +0100	[thread overview]
Message-ID: <20230720161053.1213680-1-gprocida@google.com> (raw)
In-Reply-To: <20230407203456.27141-2-quic_johmoo@quicinc.com>

Hi John.

I spent a few minutes adding stgdiff support to the script. It's
really just for illustration purposes.

As I think you know, STG doesn't yet exist as a project outside of
AOSP. Nevertheless, this may be useful to you as-is.

STG has quite a different philosophy to libabigil in terms of
filtering out certain kinds of differences. Some of the things (like
enum enumerator additions) are not considered harmless. The reasoning
behind this is basically...
https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)

However, it does have --ignore interface_addition (and the related
--ignore type_definition_addition) which can be used to detect whether
one ABI is a subset of another.

I am looking at adding support for macro definitions (gcc -g3) to STG
which will then let us cover significantly more of the UAPI surface.

Unfortunately, there are some headers which use anonymous enums to
define constants (e.g. and ironically BTF). ABI tracking these types
would require something a bit hacky. Or we could just name them.

Regards,
Giuliano.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 scripts/check-uapi.sh | 102 ++++++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 33 deletions(-)

diff --git a/scripts/check-uapi.sh b/scripts/check-uapi.sh
index 755187f27be5..982666b48f3b 100755
--- a/scripts/check-uapi.sh
+++ b/scripts/check-uapi.sh
@@ -32,6 +32,7 @@ Options:
     -v             Verbose operation (print more information about each header being checked).
 
 Environmental args:
+    STGDIFF  Custom path to stgdiff binary - use stgdiff instead of abidiff
     ABIDIFF  Custom path to abidiff binary
     CC       C compiler (default is "gcc")
     ARCH     Target architecture of C compiler (default is host arch)
@@ -270,43 +271,78 @@ compare_abi() {
 		exit "$FAIL_COMPILE"
 	fi
 
-	local ret=0
-	"$ABIDIFF" --non-reachable-types "${past_header}.bin" "${base_header}.bin" > "$log" || ret="$?"
-	if [ "$ret" -eq 0 ]; then
-		if [ "$VERBOSE" = "true" ]; then
-			printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+	if [ "$STGDIFF" ]; then
+		local ret=0
+		"$STGDIFF" --types --ignore interface_addition --elf "${past_header}.bin" "${base_header}.bin" --format small --output "$log" || ret="$?"
+		if [ "$ret" -eq 0 ]; then
+			if [ "$VERBOSE" = "true" ]; then
+				printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+			fi
+		else
+			# stgdiff's return code can be used to determine the type of error
+			if [ $((ret & 0x1)) -gt 0 ]; then
+				eprintf "error - stgdiff failed\n"
+				exit 1
+			fi
+
+			{
+				printf "!!! ABI differences detected in %s from %s -> %s !!!\n\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+				sed  -e 's/^/  /g' "$log"
+
+				if ! cmp "$past_header" "$base_header" > /dev/null 2>&1; then
+					printf "\nHeader file diff (after headers_install):\n"
+					diff -Naur "$past_header" "$base_header" \
+						| sed -e "s|${past_header}|${past_ref}/${file}|g" \
+						      -e "s|${base_header}|${base_ref:-dirty}/${file}|g"
+					printf "\n"
+				else
+					printf "\n%s did not change between %s and %s...\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+					printf "It's possible a change to one of the headers it includes caused this error:\n"
+					grep '^#include' "$base_header"
+					printf "\n"
+				fi
+			} | tee "${base_header}.error" >&2
+			return 1
 		fi
 	else
-		# Bits in abidiff's return code can be used to determine the type of error
-		if [ $((ret & 0x1)) -gt 0 ]; then
-			eprintf "error - abidiff did not run properly\n"
-			exit 1
-		fi
+		local ret=0
+		"$ABIDIFF" --non-reachable-types "${past_header}.bin" "${base_header}.bin" > "$log" || ret="$?"
+		if [ "$ret" -eq 0 ]; then
+			if [ "$VERBOSE" = "true" ]; then
+				printf "No ABI differences detected in %s from %s -> %s\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+			fi
+		else
+			# Bits in abidiff's return code can be used to determine the type of error
+			if [ $((ret & 0x1)) -gt 0 ]; then
+				eprintf "error - abidiff did not run properly\n"
+				exit 1
+			fi
 
-		# 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" "$past_ref" "${base_ref:-dirty tree}"
-			sed  -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/  /g' "$log"
-
-			if ! cmp "$past_header" "$base_header" > /dev/null 2>&1; then
-				printf "\nHeader file diff (after headers_install):\n"
-				diff -Naur "$past_header" "$base_header" \
-					| sed -e "s|${past_header}|${past_ref}/${file}|g" \
-					      -e "s|${base_header}|${base_ref:-dirty}/${file}|g"
-				printf "\n"
-			else
-				printf "\n%s did not change between %s and %s...\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
-				printf "It's possible a change to one of the headers it includes caused this error:\n"
-				grep '^#include' "$base_header"
-				printf "\n"
+			# 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
-		} | tee "${base_header}.error" >&2
-		return 1
+			{
+				printf "!!! ABI differences detected in %s from %s -> %s !!!\n\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+				sed  -e '/summary:/d' -e '/changed type/d' -e '/^$/d' -e 's/^/  /g' "$log"
+
+				if ! cmp "$past_header" "$base_header" > /dev/null 2>&1; then
+					printf "\nHeader file diff (after headers_install):\n"
+					diff -Naur "$past_header" "$base_header" \
+						| sed -e "s|${past_header}|${past_ref}/${file}|g" \
+						      -e "s|${base_header}|${base_ref:-dirty}/${file}|g"
+					printf "\n"
+				else
+					printf "\n%s did not change between %s and %s...\n" "$file" "$past_ref" "${base_ref:-dirty tree}"
+					printf "It's possible a change to one of the headers it includes caused this error:\n"
+					grep '^#include' "$base_header"
+					printf "\n"
+				fi
+			} | tee "${base_header}.error" >&2
+			return 1
+		fi
 	fi
 }
 
-- 
2.41.0.255.g8b1d071c50-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2023-07-20 16:11 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-07 20:34 [PATCH v5 0/2] Validating UAPI backwards compatibility John Moon
2023-04-07 20:34 ` John Moon
2023-04-07 20:34 ` [PATCH v5 1/2] check-uapi: Introduce check-uapi.sh John Moon
2023-04-07 20:34   ` John Moon
2023-04-10 10:03   ` Masahiro Yamada
2023-04-10 10:03     ` Masahiro Yamada
2023-04-10 18:45     ` Greg Kroah-Hartman
2023-04-10 18:45       ` Greg Kroah-Hartman
2023-04-10 23:32       ` John Moon
2023-04-10 23:32         ` John Moon
2023-04-11  6:34         ` Greg Kroah-Hartman
2023-04-11  6:34           ` Greg Kroah-Hartman
2023-04-11 18:36           ` John Moon
2023-04-11 18:36             ` John Moon
2023-04-12  6:14             ` Greg Kroah-Hartman
2023-04-12  6:14               ` Greg Kroah-Hartman
2023-04-12 16:37               ` John Moon
2023-04-12 16:37                 ` John Moon
2023-04-12 16:43                 ` Greg Kroah-Hartman
2023-04-12 16:43                   ` Greg Kroah-Hartman
2023-04-13 17:07                   ` John Moon
2023-04-13 17:07                     ` John Moon
2023-04-13 18:22                     ` Greg Kroah-Hartman
2023-04-13 18:22                       ` Greg Kroah-Hartman
2023-04-13 14:37                 ` Mark Wielaard
2023-04-13 14:37                   ` Mark Wielaard
2023-04-13 17:12                   ` Giuliano Procida
2023-04-13 17:12                     ` Giuliano Procida
2023-04-13 17:15                   ` John Moon
2023-04-13 17:15                     ` John Moon
2023-04-13 17:03     ` Nicolas Schier
2023-04-13 17:03       ` Nicolas Schier
2023-04-13 17:33       ` John Moon
2023-04-13 17:33         ` John Moon
2023-07-20 16:10   ` Giuliano Procida [this message]
2023-07-20 16:10     ` [PATCH] scripts/check-uapi.sh: add stgdiff support Giuliano Procida
2023-07-22 19:40     ` Trilok Soni
2023-07-22 19:40       ` Trilok Soni
2023-04-07 20:34 ` [PATCH v5 2/2] docs: dev-tools: Add UAPI checker documentation John Moon
2023-04-07 20:34   ` John Moon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230720161053.1213680-1-gprocida@google.com \
    --to=gprocida@google.com \
    --cc=andersson@kernel.org \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=jorcrous@amazon.com \
    --cc=kernel-team@android.com \
    --cc=libabigail@sourceware.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maennich@google.com \
    --cc=masahiroy@kernel.org \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=nicolas@fjasle.eu \
    --cc=quic_eberman@quicinc.com \
    --cc=quic_gurus@quicinc.com \
    --cc=quic_johmoo@quicinc.com \
    --cc=quic_satyap@quicinc.com \
    --cc=quic_tsoni@quicinc.com \
    --cc=rdunlap@infradead.org \
    --cc=tkjos@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.