Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh

[Trilok Soni <quic_tsoni@quicinc.com>]

On 2/18/2023 12:31 AM, Greg Kroah-Hartman wrote:
> 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?

These first patches were to introduce the tool w/ the one scenario only, 
and thanks for trying it out. We can fix it and add your suggestions.

---Trilok Soni