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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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).
>
> 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






BTW, I recommend you to not pick up a patch before having
any reviewer read the code.










--
Best Regards
Masahiro Yamada
From dbc1cfe8aae33459e78b04df21a468f82bd56060 Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Sun, 19 Feb 2023 17:08:56 +0900
Subject: [PATCH 1/2] uapi/linux/cec-funcs.h: add harmless change for
 user-space interface
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a test-case for [1].

This adds one apparently harmless macro definition.

So, we expect scripts/check-uapi.sh will pass but it actually fails
(unless you use the very new distro as your build host).

  $ ./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.I861KDej54/usr/include/linux/cec-funcs.h.pre: In function ‘cec_msg_set_audio_volume_level’:
  /tmp/tmp.I861KDej54/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.I861KDej54/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

[1]: CAK7LNASf_RQGM6QYwvUN+FLq-v3jK66Pk5jW9M-vM1V4K0xH8g@mail.gmail.com/T/#m0a9c490d342541056edce7a0138b529f99cc899e">https://lore.kernel.org/linux-kbuild/CAK7LNASf_RQGM6QYwvUN+FLq-v3jK66Pk5jW9M-vM1V4K0xH8g@mail.gmail.com/T/#m0a9c490d342541056edce7a0138b529f99cc899e

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
 include/uapi/linux/cec-funcs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/cec-funcs.h b/include/uapi/linux/cec-funcs.h
index d58fa1cdcb08..524dd3e95b5c 100644
--- a/include/uapi/linux/cec-funcs.h
+++ b/include/uapi/linux/cec-funcs.h
@@ -1955,4 +1955,6 @@ static inline void cec_ops_cdc_hpd_report_state(const struct cec_msg *msg,
 	*hpd_error = msg->msg[5] & 0xf;
 }
 
+#define DUMMY_FOO 1
+
 #endif
-- 
2.34.1

From 65320bd962dd92b9376f0fdb93ce84364e6e1289 Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Sun, 19 Feb 2023 17:11:13 +0900
Subject: [PATCH 2/2] uapi/linux/signal.h: add harmless change for user-space
 interface
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a test-case for [1].

This adds one apparently harmless macro definition.

So, we expect scripts/check-uapi.sh will pass, but it actually fails
(unless you use the very new distro as your build host).

  $ ./scripts/check-uapi.sh
  Couldn't compile current version of UAPI header include/uapi/linux/signal.h...
  In file included from /tmp/tmp.EiAC9zS9ud/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

[1]: CAK7LNASf_RQGM6QYwvUN+FLq-v3jK66Pk5jW9M-vM1V4K0xH8g@mail.gmail.com/T/#m0a9c490d342541056edce7a0138b529f99cc899e">https://lore.kernel.org/linux-kbuild/CAK7LNASf_RQGM6QYwvUN+FLq-v3jK66Pk5jW9M-vM1V4K0xH8g@mail.gmail.com/T/#m0a9c490d342541056edce7a0138b529f99cc899e

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
 include/uapi/linux/signal.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/signal.h b/include/uapi/linux/signal.h
index e03238f8d478..c5bbbdb2ea0d 100644
--- a/include/uapi/linux/signal.h
+++ b/include/uapi/linux/signal.h
@@ -13,4 +13,6 @@
 /* mask for all SS_xxx flags */
 #define SS_FLAG_BITS	SS_AUTODISARM
 
+#define DUMMY_BAR 1
+
 #endif /* _UAPI_LINUX_SIGNAL_H */
-- 
2.34.1


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux