Re: [PATCH][next] xfs: Fix fall-through warnings for Clang

[Dave Chinner]

On Fri, May 28, 2021 at 09:48:39AM -0700, Kees Cook wrote:
> On Thu, May 27, 2021 at 05:34:54PM -0700, Darrick J. Wong wrote:
> > The choices are bad, so **turn it off** in fs/xfs/Makefile and don't go
> > making us clutter up shared library code that will then have to be
> > ported to userspace.
> 
> Ah! So the concern you have is with portable code shared outside of
> the kernel? This came up also with the ACPICA code (which is regularly
> imported into the kernel tree), and they just included their own macro
> directly[1].
> 
> Would you prefer something like that, which would be XFS-specific? I'm
> just trying to find a way to avoid losing fall-through coverage
> in XFS. (Especially since distros are slowly moving toward enabling
> -Wimplicit-fallthrough by default since it's a long-standing weakness
> in the C language, and has been hiding real bugs for years.)
> 
> It seems like the options here could be:
> 1) Use an XFS-specific macro like ACPICA does, so that the out-of-tree
>    userspace code can share it (more typing, keep coverage).
> 2) Add -Wno-implicit-fallthrough to fs/xfs/Makefile (easy, lose coverage).
> 
> For 1), which portions are shared between xfsprogs and the kernel? Only
> libxfs/ and scrub/? How does the below patch look? I could prepare similar
> for all of xfsprogs, or do this only for xfsprogs and leave the stuff
> outside of libxfs/ and scrube/ using the kernel's "fallthrough" macro?
> 
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 782fdd08f759..ade529ddb60b 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -184,4 +184,14 @@ struct xfs_ino_geometry {
>  
>  };
>  
> +/* Programmatically mark implicit fallthroughs for GCC and Clang. */
> +#ifndef __has_attribute
> +#define __has_attribute(x) 0
> +#endif
> +#if __has_attribute(__fallthrough__)
> +#define XFS_FALLTHROUGH __attribute__((__fallthrough__))
> +#else
> +#define XFS_FALLTHROUGH do { } while (0)
> +#endif

No. This is Obviously Stupid.

Did you listen to what Darrick just said? Wasn't it "Don't go making
us clutter up shared library code..."?

If we're complaining that replacing obvious comments demonstrating
intent with weird macros is not in our best interest, then replacing
the comments with an eye-bleeding, one off, XFS specific macro
doesn't address the objections being raised.

I don't even know why you are continuing trying to convince us to
take the changes - Darrick has already given you guys the option of
going straight to Linus with them. If you do that then we'll just
have to deal with the undefined macro fallout that results in
userspace.

Please just stop wasting more of our time on this.

-Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx