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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Apr 10, 2023 at 04:32:49PM -0700, John Moon wrote:
> > > 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.

checkpatch.pl does not matter, it is a "hint", and many patches
explicitly ignore it (think about patches in the staging tree, you could
fix up one checkpatch issue for a line, but ignore another one as you
are not supposed to mix them up.)

Also for some subsystems, checkpatch does not matter because their
codebase is old and follows different rules.  And in some places,
checkpatch is just wrong, because it's a perl script and can not really
parse code.

So NEVER use that as a comparison to the user/kernel abi please.  It's a
false comparison.

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

That is not a breakage, that's a tool problem.

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

Again, not a real breakage, size is still the same.

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

Adding data to the end of a structure is a well-known way to extend the
api, in SOME instances if it is used properly.

So again, not a break.

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

Increasing enums is in no way an abi break unless the size of the
structure changes.

Using reserved fields too is not a breakage.

So yes, it looks like the tooling needs some work in order for us to be
able to use this properly, digging through false positives like this is
going to make it not used at all.

thanks,

greg k-h