Message-ID: <f10b5d1f-4260-2b20-107a-9dbb8bc0e65f@quicinc.com>
Date:   Wed, 22 Feb 2023 12:33:29 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh
Content-Language: en-US
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        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>
References: <20230217202234.32260-1-quic_johmoo@quicinc.com>
 <20230217202234.32260-2-quic_johmoo@quicinc.com> <Y/CJhzSJ5YKvD7my@kroah.com>
From:   John Moon <quic_johmoo@quicinc.com>
In-Reply-To: <Y/CJhzSJ5YKvD7my@kroah.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/18/2023 12:17 AM, 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.
> 

Okay, will keep him in the loop!

> 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!
> 

Will resolve all shellcheck warnings in next rev. Not sure about the 
coding style differences you're referring to - is there a shell script 
coding style document?

> 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!
> 

Should be straightforward to add. Since I'm making another rev, I'll 
take care of this as well.

> thanks,
> 
> greg k-h