Date:   Sat, 18 Feb 2023 09:17:11 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     John Moon <quic_johmoo@quicinc.com>,
        Randy Dunlap <rdunlap@infradead.org>
Cc:     Masahiro Yamada <masahiroy@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>,
        linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        Trilok Soni <quic_tsoni@quicinc.com>,
        Bjorn Andersson <andersson@kernel.org>,
        Todd Kjos <tkjos@google.com>,
        Matthias Maennich <maennich@google.com>,
        Giuliano Procida <gprocida@google.com>,
        kernel-team@android.com, Jordan Crouse <jorcrous@amazon.com>
Subject: Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh
Message-ID: <Y/CJhzSJ5YKvD7my@kroah.com>
References: <20230217202234.32260-1-quic_johmoo@quicinc.com>
 <20230217202234.32260-2-quic_johmoo@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230217202234.32260-2-quic_johmoo@quicinc.com>
Precedence: bulk

On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon wrote:
> While the kernel community has been good at maintaining backwards
> compatibility with kernel UAPIs, it would be helpful to have a tool
> to check if a patch introduces changes that break backwards
> compatibility.
> 
> To that end, introduce check-uapi.sh: a simple shell script that
> checks for changes to UAPI headers using libabigail.
> 
> libabigail is "a framework which aims at helping developers and
> software distributors to spot some ABI-related issues like interface
> incompatibility in ELF shared libraries by performing a static
> analysis of the ELF binaries at hand."
> 
> The script uses one of libabigail's tools, "abidiff", to compile the
> changed header before and after the patch to detect any changes.
> 
> abidiff "compares the ABI of two shared libraries in ELF format. It
> emits a meaningful report describing the differences between the two
> ABIs."
> 
> Signed-off-by: John Moon <quic_johmoo@quicinc.com>
> ---
>  scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 245 insertions(+)
>  create mode 100755 scripts/check-uapi.sh

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

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

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

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

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

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

So it worked!

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

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

thanks,

greg k-h