Messages in this thread | | | Date | Wed, 22 Feb 2023 12:36:34 -0800 | Subject | Re: [PATCH RESEND 1/1] check-uapi: Introduce check-uapi.sh | From | John Moon <> |
| |
On 2/19/2023 12:52 AM, Masahiro Yamada wrote: > On Sat, Feb 18, 2023 at 5:31 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> 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).
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
| |