On 2/19/2023 12:52 AM, Masahiro Yamada wrote:
On Sat, Feb 18, 2023 at 5:31 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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).
I'll make an update to improve this behavior. Should be able to specify
the commit in which a change is made and which past commits to check
against.
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
This tool does not even work with changes in HEAD.
I attached two test-case patches.
This patch does not mention any requirement for
the build host, but neither of them works for me,
on Ubuntu 22.04.
I guess people will find more bad cases
if they use older distros as the build host.
(I know why it does not work, though)
For patch 0001:
$ ./scripts/check-uapi.sh
Couldn't compile current version of UAPI header
include/uapi/linux/cec-funcs.h...
In file included from <command-line>:
/tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre: In function
‘cec_msg_set_audio_volume_level’:
/tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre:1575:23: error:
‘CEC_MSG_SET_AUDIO_VOLUME_LEVEL’ undeclared (first use in this
function)
1575 | msg->msg[1] = CEC_MSG_SET_AUDIO_VOLUME_LEVEL;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/tmp.gYBwfiWTqX/usr/include/linux/cec-funcs.h.pre:1575:23: note:
each undeclared identifier is reported only once for each function it
appears in
0/1 UAPI header file changes are backwards compatible
UAPI header ABI check failed
For patch 0002:
$ ./scripts/check-uapi.sh
Couldn't compile current version of UAPI header include/uapi/linux/signal.h...
In file included from /tmp/tmp.wm5RykUr3y/usr/include/linux/signal.h.pre:5,
from <command-line>:
/usr/include/x86_64-linux-gnu/asm/signal.h:103:9: error: unknown type
name ‘size_t’
103 | size_t ss_size;
| ^~~~~~
0/1 UAPI header file changes are backwards compatible
UAPI header ABI check failed
Thanks for the example patches! We understand the issue and I think
there's a simple solution. We can install all of the current UAPI
headers into the tmp directory and include those. That way, the user's
system headers shouldn't enter into the equation.
BTW, I recommend you to not pick up a patch before having
any reviewer read the code.
--
Best Regards
Masahiro Yamada
