Re: [PATCH v2] tools/nolibc: compiler: add macro __nolibc_fallthrough

[Willy Tarreau <w@1wt.eu>]

Hi David,

On Mon, Sep 30, 2024 at 03:29:26PM +0000, David Laight wrote:
> From: Thomas Weißschuh
> > Sent: 30 September 2024 06:35
> > 
> > Recent version of GCC and clang gained -Wimplicit-fallthrough,
> > warning about implicit fall-through between switch labels.
> > As nolibc does not control the compilation flags, this can trigger
> > warnings for when built by the user.
> > Make use of the "fallthrough" attribute to explicitly annotate the
> > expected fall-throughs and silence the warning.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > Changes in v2:
> > - Add a do-while loop to improve compiler compatibility
> > - Link to v1: https://lore.kernel.org/r/20240929-nolibc-fallthrough-v1-1-5ee07ea9a683@weissschuh.net
> ...
> > 
> > +#if __nolibc_has_attribute(fallthrough)
> > +#  define __nolibc_fallthrough do { } while (0); __attribute__((fallthrough))
> > +#else
> > +#  define __nolibc_fallthrough do { } while (0)
> > +#endif /* __nolibc_has_attribute(fallthrough) */
> > +
> >  #endif /* _NOLIBC_COMPILER_H */
> > diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> > index c968dbbc4ef8137e237b859bf18a6d2970230cbf..3892034198dd566d21a5cc0a9f67cf097d428393 100644
> > --- a/tools/include/nolibc/stdio.h
> > +++ b/tools/include/nolibc/stdio.h
> > @@ -15,6 +15,7 @@
> >  #include "stdarg.h"
> >  #include "stdlib.h"
> >  #include "string.h"
> > +#include "compiler.h"
> > 
> >  #ifndef EOF
> >  #define EOF (-1)
> > @@ -264,7 +265,7 @@ int vfprintf(FILE *stream, const char *fmt, va_list args)
> >  				case 'p':
> >  					*(out++) = '0';
> >  					*(out++) = 'x';
> > -					/* fall through */
> > +					__nolibc_fallthrough;
> >  				default: /* 'x' and 'p' above */
> 
> Doesn't this break any old tools that would have parsed the /* fall though */
> comment (or any of its variants)?

Apparently not, I had rechecked with gcc-6.5 which is the one that
used to expect comments, and it's apparently fine as well with the
macro, probably due to its name which contains "fallthrough" in it.
Anyway that one was broken when running cpp separately from cc1, so
those at risk of seeing any warning already see it in their programs.

> If you move the ; into the define the 'old' definition can be empty.
> And then it is possible that:
> 				case x:
> 					xxxxx;
> 					/* fall though */
> 					__nolibc_fallthough
> 				case y:
> 
> will be processed correctly be all tools.

Missing trailing commas is a real PITA, which can even cause indent
issues with editors, better not do that, frankly.

> I know I had to lower the warning level for one of our kernel drivers.
> But I've forgotten why - that code has to go through a lot of
> compilers - including Microsofts.

Probably a case like I mentioned above: pre-processing being run as
a first step, making the compiler not see the comment and complain.

Cheers,
Willy