Date:   Sat, 18 Feb 2023 09:31:48 +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/CM9JtK0914YUE0@kroah.com>
References: <20230217202234.32260-1-quic_johmoo@quicinc.com>
 <20230217202234.32260-2-quic_johmoo@quicinc.com>
 <Y/CJhzSJ5YKvD7my@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y/CJhzSJ5YKvD7my@kroah.com>
Precedence: bulk

On Sat, Feb 18, 2023 at 09:17:12AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 17, 2023 at 12:22:34PM -0800, John Moon wrote:
> > While the kernel community has been good at maintaining backwards
> > compatibility with kernel UAPIs, it would be helpful to have a tool
> > to check if a patch introduces changes that break backwards
> > compatibility.
> > 
> > To that end, introduce check-uapi.sh: a simple shell script that
> > checks for changes to UAPI headers using libabigail.
> > 
> > libabigail is "a framework which aims at helping developers and
> > software distributors to spot some ABI-related issues like interface
> > incompatibility in ELF shared libraries by performing a static
> > analysis of the ELF binaries at hand."
> > 
> > The script uses one of libabigail's tools, "abidiff", to compile the
> > changed header before and after the patch to detect any changes.
> > 
> > abidiff "compares the ABI of two shared libraries in ELF format. It
> > emits a meaningful report describing the differences between the two
> > ABIs."
> > 
> > Signed-off-by: John Moon <quic_johmoo@quicinc.com>
> > ---
> >  scripts/check-uapi.sh | 245 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 245 insertions(+)
> >  create mode 100755 scripts/check-uapi.sh
> 
> Ok, this is very cool, thank you so much for doing this.
> 
> I know Randy Dunlap was also looking into this previously, so I've cc:ed
> him and bounced him the original.
> 
> I tried this out, and at first glance, this felt like it was just "too
> fast" in that nothing actually was being tested.  So I manually added a
> field to a structure I know would break the abi, and:
> 
> 	$ ./scripts/check-uapi.sh
> 	!!! ABI differences detected in include/uapi/linux/usb/ch9.h (compared to file at HEAD^1) !!!
> 
> 	    [C] 'struct usb_ctrlrequest' changed:
> 	      type size changed from 64 to 72 (in bits)
> 	      1 data member insertion:
> 		'__u8 abi_break', at offset 16 (in bits) at ch9.h:216:1
> 	      3 data member changes:
> 		'__le16 wValue' offset changed from 16 to 24 (in bits) (by +8 bits)
> 		'__le16 wIndex' offset changed from 32 to 40 (in bits) (by +8 bits)
> 		'__le16 wLength' offset changed from 48 to 56 (in bits) (by +8 bits)
> 
> 	0/1 UAPI header file changes are backwards compatible
> 	UAPI header ABI check failed
> 
> So it worked!

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

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

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

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

thanks,

greg k-h