Re: [PATCH 00/18] Annotate switch/case fallthrough cases

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On Fri, Mar 28, 2025 at 01:45:54PM +0000, Andre Przywara wrote:
> On Fri, 28 Mar 2025 10:39:59 +0000
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
> Hi,
> 
> > On Fri, 28 Mar 2025 11:28:05 +0100
> > Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > 
> > Hi,
> > 
> > > On 27.03.25 16:32, Andre Przywara wrote:  
> > > > C's implicit fallthrough behaviour in switch/case statements can lead to
> > > > subtle bugs. Quite some while ago many compilers introduced warnings in
> > > > those cases, requiring intentional fallthrough's to be annotated.
> > > >
> > > > So far we were not enabling that compiler option, so many ambiguities
> > > > and some bugs in the code went unnoticed.
> > > >
> > > > This series adds the required annotations in code paths that the first
> > > > stage of the U-Boot CI covers. There is a large number of cases left
> > > > in the libbz2 code. The usage of switch/case is borderline insane there,
> > > > labels are hidden in macros, and there are no breaks, but just goto's.
> > > > Upstream still uses very similar code, without any annotations. I still
> > > > am not 100% sure those are meant to fall through or not, and plan to do
> > > > further investigations, but didn't want to hold the rest of the patches
> > > > back. You can see for yourself by applying patch 18/18 and building for
> > > > sandbox64, for instance.    
> > > 
> > > Can we use something like
> > > 
> > > CFLAGS_REMOVE_bzlib.o = -Wimplicit-fallthrough  
> > 
> > Ah, didn't know we have that in U-Boot as well! Sounds promising, and
> > fixes the sandbox build for me (when using bzlib_decompress.o). I will add
> > this to the last patch and will check what the CI has to say about this.
> 
> So for the records: silencing the libbzip2 warnings uncovered a whole new
> bunch of warnings, in stage 1 still. Some compilers used in the CI (clang?)
> seem to be more picky about how to annotate, so a pure comment (/* fall
> through */) would not cut it, it has to be the attribute - provided by our
> "statement macro".
> So I fixed those quickly, stuffed them into some patch, and now the first
> CI stage (test.py) passes - but only to uncover a large number of new
> warnings in the world build.
> So I will keep on patching, as some kind of procrastination project ;-)

So, for the flag, what I would like is to borrow the
CONFIG_CC_IMPLICIT_FALLTHROUGH logic from the kernel. Then we should be
able to do:
CFLAGS_REMOVE_bzlib.o += $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
I think.

Then as follow-up convert any of our just /* fallthrough */ to
'fallthrough;'.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]