Message-ID: <ca30cded-4ea4-8c70-61ad-5d0d99b123d3@quicinc.com>
Date:   Sat, 18 Feb 2023 10:22:09 -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
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        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>,
        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>
 <Y/CM9JtK0914YUE0@kroah.com>
Content-Language: en-US
From:   Trilok Soni <quic_tsoni@quicinc.com>
In-Reply-To: <Y/CM9JtK0914YUE0@kroah.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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