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

[John Moon <quic_johmoo@quicinc.com>]

On 4/10/2023 11:45 AM, Greg Kroah-Hartman wrote:
> On Mon, Apr 10, 2023 at 07:03:05PM +0900, Masahiro Yamada wrote:
>> On Sat, Apr 8, 2023 at 5:35 AM John Moon <quic_johmoo@quicinc.com> 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 commit 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 commit 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."
>>>
>>> The script also includes the ability to check the compatibility of
>>> all UAPI headers across commits. This allows developers to inspect
>>> the stability of the UAPIs over time.
>>>
>>> Signed-off-by: John Moon <quic_johmoo@quicinc.com>
>>
>>
>>
>> BTW, is there anybody (except the submitters) who loves this tool?
>> (or anybody who has ever evaluated this?)
> 
> I evaluated the first one, and yes, I do want this, but I haven't tested
> it out yet, sorry.
> 
> I get patches for header files all the time and hand-verifying that they
> don't break the abi is a pain at times
>

Agreed, this is the way we're using the tool internally. It's great as a 
quick spot-check on a change.

>> According to this tool, it looks like we broke a lot of UAPI
>> headers in the previous MW (between v6.2 and v6.3-rc1).
> 
> That's not ok, and needs to be fixed, otherwise this is useless as no
> one can rely on it at all.
> 

Right, there are several classes of false positives that we've 
documented and when examining thousands of commits at time, it'll flag 
many things.

For some comparison, if you run checkpatch on the same changeset 
(v6.2..v6.3-rc1), you get 995 errors and 7,313 warnings. Still, 
checkpatch is helpful for spot-checks.

"./scripts/check-uapi.sh -b v6.3-rc1 -p v6.2" flags 36 out of the 911 
files checked. Of those 36, 19 fell into the currently documented false 
positive categories:

Enum expansion: 17
Expanding into padded/reserved fields: 2

Beyond those, the tool appears to be flagging legitimate breakages.

Some fit into the definition of "intentional breakages" where support is 
being dropped or something is being refactored:

  File removals:
    - include/uapi/drm/i810_drm.h
    - include/uapi/drm/mga_drm.h
    - include/uapi/drm/r128_drm.h
    - include/uapi/drm/savage_drm.h
    - include/uapi/drm/sis_drm.h
    - include/uapi/drm/via_drm.h
    - include/uapi/linux/meye.h

  File moves:
    - include/uapi/misc/habanalabs.h

  Removal of struct:
    - include/uapi/linux/uuid.h (5e6a51787fef)
      - include/uapi/linux/mei.h (failed due to uuid.h)
      - include/uapi/linux/ublk_cmd.h (failed due to uuid.h)

Others do not seem to be intentional:

  Addition/use of flex arrays:
    - include/uapi/linux/rseq.h (f7b01bb0b57f)
    - include/uapi/scsi/scsi_bsg_mpi3mr.h (c6f2e6b6eaaf)

  Type change:
    - include/uapi/scsi/scsi_bsg_ufs.h (3f5145a615238)

  Additions into existing struct:
    - include/uapi/drm/amdgpu_drm.h (b299221faf9b)
    - include/uapi/linux/perf_event.h (09519ec3b19e)
    - include/uapi/linux/virtio_blk.h (95bfec41bd3d)

Is there something I'm missing that makes these changes false positives? 
If so, I'd be happy to add on to the documentation and work towards a 
way to filter them out.

In the mean time, we will start a thread on the libabigail mailing list 
to see if there's a way to add flags such as --ignore-enum-expansion, 
--ignore-expansion-into-reserved-fields, etc. Enum expansion seems to be 
making up the largest portion of false positives, so would be the best 
thing to filter out.

>> The script takes some time because it builds many objects
>> internally.
>>
>> However, once this script starts running, you must not hit Ctrl-C.
>> If you do it, your repository will be sprinkled with a ton
>> of untracked files.
> 
> That needs to be unwound and fixed.
> 
>> CAVEAT
>> This tool runs 'git checkout' a couple of times internally.
>> If you interrupt it, your worktree might be messed up.
> 
> ctrl-c can be properly caught and the git state needs to be restored for
> this to be able to be accepted.
>

Yes, this can be taken care of.

> thanks,
> 
> greg k-h