| Message ID | 20211205174951.GQ8467@magnolia (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [xfsprogs-5.14.2,URGENT] libxfs: hide the drainbamaged fallthrough macro from xfslibs | expand |
On Sun, Dec 05, 2021 at 09:49:51AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Back in mid-2021, Kees and Gustavo rammed into the kernel a bunch of > static checker "improvements" that redefined '/* fallthrough */' > comments for switch statements as a macro that virtualizes either that > same comment, a do-while loop, or a compiler __attribute__. This was > necessary to work around the poor decision-making of the clang, gcc, and > C language standard authors, who collectively came up with four mutually > incompatible ways to document a lack of branching in a code flow. > > Having received ZERO HELP porting this to userspace, Eric and I > foolishly dumped that crap into linux.h, which was a poor decision > because we keep forgetting that linux.h is exported as a userspace > header. This has now caused downstream regressions in Debian[1] and > will probably cause more problems in the other distros. > > Move it to platform_defs.h since that's not shipped publicly and leave a > warning to anyone else who dare modify linux.h. > > [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1000974 > > Fixes: df9c7d8d ("xfs: Fix fall-through warnings for Clang") > Cc: 1000974@bugs.debian.org, gustavoars@kernel.org, keescook@chromium.org > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > include/linux.h | 20 ++------------------ > include/platform_defs.h.in | 21 +++++++++++++++++++++ > 2 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/include/linux.h b/include/linux.h > index 24650228..054117aa 100644 > --- a/include/linux.h > +++ b/include/linux.h > @@ -360,24 +360,8 @@ fsmap_advance( > #endif /* HAVE_MAP_SYNC */ > > /* > - * Add the pseudo keyword 'fallthrough' so case statement blocks > - * must end with any of these keywords: > - * break; > - * fallthrough; > - * continue; > - * goto <label>; > - * return [expression]; > - * > - * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes > + * Reminder: anything added to this file will be compiled into downstream > + * userspace projects! This comment belongs at the top of the file before all the includes, not at the end of it. Otherwise looks ok for a quick fix. Reviewed-by: Dave Chinner <dchinner@redhat.com> But I have to wonder - why is this file even exported to userspace? It's mostly the xfsprogs source build wrapper stuff for linux - there's no public API in it except for xfsctl().... Cheers, Dave.
On 12/5/21 11:49 AM, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Back in mid-2021, Kees and Gustavo rammed into the kernel a bunch of > static checker "improvements" that redefined '/* fallthrough */' > comments for switch statements as a macro that virtualizes either that > same comment, a do-while loop, or a compiler __attribute__. This was > necessary to work around the poor decision-making of the clang, gcc, and > C language standard authors, who collectively came up with four mutually > incompatible ways to document a lack of branching in a code flow. > > Having received ZERO HELP porting this to userspace, Eric and I > foolishly dumped that crap into linux.h, which was a poor decision > because we keep forgetting that linux.h is exported as a userspace > header. This has now caused downstream regressions in Debian[1] and > will probably cause more problems in the other distros. > > Move it to platform_defs.h since that's not shipped publicly and leave a > warning to anyone else who dare modify linux.h. > > [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1000974 > > Fixes: df9c7d8d ("xfs: Fix fall-through warnings for Clang") > Cc: 1000974@bugs.debian.org, gustavoars@kernel.org, keescook@chromium.org > Signed-off-by: Darrick J. Wong <djwong@kernel.org> Thanks Darrick, I'll get a 5.14.2 pushed out today. Not sure if it was you or I who stuffed it into this header originally, but apologies for not thinking that through before merging it in any case. Reviewed-by: Eric Sandeen <sandeen@redhat.com> -Eric > --- > include/linux.h | 20 ++------------------ > include/platform_defs.h.in | 21 +++++++++++++++++++++ > 2 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/include/linux.h b/include/linux.h > index 24650228..054117aa 100644 > --- a/include/linux.h > +++ b/include/linux.h > @@ -360,24 +360,8 @@ fsmap_advance( > #endif /* HAVE_MAP_SYNC */ > > /* > - * Add the pseudo keyword 'fallthrough' so case statement blocks > - * must end with any of these keywords: > - * break; > - * fallthrough; > - * continue; > - * goto <label>; > - * return [expression]; > - * > - * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes > + * Reminder: anything added to this file will be compiled into downstream > + * userspace projects! > */ > -#if defined __has_attribute > -# if __has_attribute(__fallthrough__) > -# define fallthrough __attribute__((__fallthrough__)) > -# else > -# define fallthrough do {} while (0) /* fallthrough */ > -# endif > -#else > -# define fallthrough do {} while (0) /* fallthrough */ > -#endif > > #endif /* __XFS_LINUX_H__ */ > diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in > index 7c6b3ada..6e6f26ef 100644 > --- a/include/platform_defs.h.in > +++ b/include/platform_defs.h.in > @@ -113,4 +113,25 @@ static inline size_t __ab_c_size(size_t a, size_t b, size_t c) > sizeof(*(p)->member) + __must_be_array((p)->member), \ > sizeof(*(p))) > > +/* > + * Add the pseudo keyword 'fallthrough' so case statement blocks > + * must end with any of these keywords: > + * break; > + * fallthrough; > + * continue; > + * goto <label>; > + * return [expression]; > + * > + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes > + */ > +#if defined __has_attribute > +# if __has_attribute(__fallthrough__) > +# define fallthrough __attribute__((__fallthrough__)) > +# else > +# define fallthrough do {} while (0) /* fallthrough */ > +# endif > +#else > +# define fallthrough do {} while (0) /* fallthrough */ > +#endif > + > #endif /* __XFS_PLATFORM_DEFS_H__ */ >
On Sun, Dec 05, 2021 at 09:49:51AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Back in mid-2021, Kees and Gustavo rammed into the kernel a bunch of > static checker "improvements" that redefined '/* fallthrough */' > comments for switch statements as a macro that virtualizes either that > same comment, a do-while loop, or a compiler __attribute__. This was > necessary to work around the poor decision-making of the clang, gcc, and > C language standard authors, who collectively came up with four mutually > incompatible ways to document a lack of branching in a code flow. > > Having received ZERO HELP porting this to userspace, Eric and I Look, I know you don't like this feature, but claiming that you received no help with it is just false. I explicitly offered to help with xfsprogs, and even sent a first-attempt at a patch to do so[1], which looks very similar to what you've got here, almost 6 months later. I even went through and changed all the comments to an explicitly XFS-specific macro when you made it clear you hated the statement-like "fallthrough" macro name. I continue to be baffled about this whole saga. We're all trying to help make Linux better, and I went out of my way to help with xfsprogs too to minimize the impact on you (since you said you wanted to have nothing to do with it), yet Gustavo and I got continually flamed by yourself and Dave, including even now in this very misleading commit log. What is going on here? -Kees [1] https://lore.kernel.org/lkml/202105280915.9117D7C@keescook/
diff --git a/include/linux.h b/include/linux.h index 24650228..054117aa 100644 --- a/include/linux.h +++ b/include/linux.h @@ -360,24 +360,8 @@ fsmap_advance( #endif /* HAVE_MAP_SYNC */ /* - * Add the pseudo keyword 'fallthrough' so case statement blocks - * must end with any of these keywords: - * break; - * fallthrough; - * continue; - * goto <label>; - * return [expression]; - * - * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes + * Reminder: anything added to this file will be compiled into downstream + * userspace projects! */ -#if defined __has_attribute -# if __has_attribute(__fallthrough__) -# define fallthrough __attribute__((__fallthrough__)) -# else -# define fallthrough do {} while (0) /* fallthrough */ -# endif -#else -# define fallthrough do {} while (0) /* fallthrough */ -#endif #endif /* __XFS_LINUX_H__ */ diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in index 7c6b3ada..6e6f26ef 100644 --- a/include/platform_defs.h.in +++ b/include/platform_defs.h.in @@ -113,4 +113,25 @@ static inline size_t __ab_c_size(size_t a, size_t b, size_t c) sizeof(*(p)->member) + __must_be_array((p)->member), \ sizeof(*(p))) +/* + * Add the pseudo keyword 'fallthrough' so case statement blocks + * must end with any of these keywords: + * break; + * fallthrough; + * continue; + * goto <label>; + * return [expression]; + * + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes + */ +#if defined __has_attribute +# if __has_attribute(__fallthrough__) +# define fallthrough __attribute__((__fallthrough__)) +# else +# define fallthrough do {} while (0) /* fallthrough */ +# endif +#else +# define fallthrough do {} while (0) /* fallthrough */ +#endif + #endif /* __XFS_PLATFORM_DEFS_H__ */