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

[Andre Przywara <andre.przywara@arm.com>]

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

Cheers,
Andre

> 
> Cheers,
> Andre
> 
> > 
> > in the Makefile if that module is too hard to fix?
> > 
> > Best regards
> > 
> > Heinrich
> >   
> > >
> > > Because of this we cannot quite enable the warning in the Makefile yet,
> > > but those fixes are worth regardless, and be it to increase readability.
> > >
> > > Please not that those patches do not fix anything, really, they just add
> > > those fallthrough annotations, so the series is not really critical.
> > >
> > > Cheers,
> > > Andre
> > >
> > > Andre Przywara (18):
> > >    spl: mmc: properly annotate fallthrough
> > >    zlib: annotate switch/case fallthrough cases
> > >    gadget: f_thor: annotate switch/case fallthrough
> > >    use proper fallthrough annotations
> > >    net/net: fix switch/case fallthrough annotations
> > >    fastboot: annotate switch/case fallthrough case
> > >    net: sun8i-emac: annotate fallthrough
> > >    usb: ohci-hcd: annotate switch/case fallthrough
> > >    usb: xhci: annotate switch/case fallthrough properly (TBF)
> > >    video: annotate switch/case fall-through
> > >    net: e1000: annotate switch/case fallthrough
> > >    mtd: ubi: annotate fallthrough (TBF)
> > >    arm: mach-k3: am62p: annotate switch/case fallthrough
> > >    mtd: spi-nor-tiny: annotate switch/case fallthrough
> > >    mtd: rawnand: nand_base: annotate switch/case fallthrough
> > >    cmd: pmic: annotate switch/case fallthrough
> > >    cmd: spl: annotate switch/case fallthrough
> > >    [DO NOT MERGE] Makefile: enable switch/case fallthrough warnings
> > >
> > >   Makefile                              |  1 +
> > >   arch/arm/mach-k3/am62px/am62p5_init.c |  1 +
> > >   cmd/pmic.c                            |  1 +
> > >   cmd/spl.c                             |  2 ++
> > >   common/command.c                      |  2 +-
> > >   common/spl/spl_mmc.c                  |  1 +
> > >   drivers/fastboot/fb_command.c         |  1 +
> > >   drivers/mtd/nand/raw/nand_base.c      |  4 ++++
> > >   drivers/mtd/spi/spi-nor-tiny.c        |  1 +
> > >   drivers/mtd/ubi/attach.c              |  1 +
> > >   drivers/mtd/ubi/build.c               |  3 +++
> > >   drivers/net/e1000.c                   |  2 ++
> > >   drivers/net/sun8i_emac.c              |  1 +
> > >   drivers/usb/gadget/f_thor.c           |  1 +
> > >   drivers/usb/host/ohci-hcd.c           |  3 ++-
> > >   drivers/usb/host/xhci.c               |  2 +-
> > >   drivers/video/video-uclass.c          |  2 ++
> > >   lib/tiny-printf.c                     |  2 +-
> > >   lib/zlib/inflate.c                    | 21 +++++++++++++++++++++
> > >   net/net.c                             |  5 ++---
> > >   20 files changed, 50 insertions(+), 7 deletions(-)
> > >    
> >   
>