Implicit fallthroughs

[r-barnes]

LLVM 15 with -Wimplicit-fallthrough enabled refuses to compile libpng because there are implicit fallthroughs.

Some of these, such as those in pngread.c are commented. Others might not be (I'm still iterating).

[[fallthrough]] is part of C23 and __attribute__((fallthrough)) is available now to explicit mark fallthroughs in a way that's unambiguous to compilers.

Would it be alright if I put up a PR introducing a cross-compiler compatible INTENTIONAL_FALLTHROUGH macro?

[jbowler]

The code has to be ANSI-C, aka ISO-C90 aka C89. It is expected that the code will be compiled with "-ansi" (or equivalent) and compiling without this, while it typically works, is not supported. So far as testing is concerned "-pedantic" should be used too.

The code is tested with -Wimplicit-fallthrough with GCC and so far as warnings are concerned GCC really is the only supported compiler; obviously warnings that correspond to real bugs are valid from any compiler but trying to kill warnings in all compilers is impossible because they end up demanding different things.

The compiler flags when testing with GCC are "-Wall -Wextra -Werror", there is configure support for -Werror (because configure fails if -Werror is set; use --enable-werror). Note that those settings are not set by default. This is because different versions of GCC have different warnings and new versions often have gratuitous additions that get removed PDQ.

clang 18.1.8 -ansi (I think that is pretty much the latest version) does not warn about implicit-fallthrough even with -Wall and -Wextra An explicit -Wimplicit-fallthrough is required and even then it's just a warning (unless -Werror is given of course).

With '-Wall -Wextra -std=c89 -pedantic" neither GCC nor clang warn. GCC does check for implicit fallthrough - I deleted line 3333 from pngfix.c and got the warning.

So clang has removed -Wimplicit-fallthrough from -Wall - Wextra; presumably they got too many bugs about not recognizing the comment! Perhaps they should fix that :-)

I really don't like the idea of replacing a safe, innocuous, comment with a mess of preprocessor. It's replacing something that works (with GCC at least) and is 100% safe with something that will require continuous maintenance to remove a warning that clang have already disabled themselves! It achieves nothing because GCC -Wall -Wextra -Werror already ensures that the build fails if there is an implicit fall-through.

[r-barnes]

Thanks, @jbowler. I'm afraid I'm stuck using LLVM-15 (and greater) with -Wimplicit-fallthrough enabled for the whole of the codebase I work with, so I've already had to manually adjust libpng to get compilation and will continue to have to do that.

It'd be swell if the fallthrough protections were cross-compiler compatible and it feels as though the machinery to do that (introduction of a preprocessor macro) isn't too bad (other projects, such as sqlite use it).

I can understand if you feel otherwise, though, and appreciate that you're already using -Wimplicit-fallthrough - that's worlds better than other projects I've interacted where implicit fallthroughs are common and undocumented. Thanks for writing safe code.

[jbowler]

It'd be swell if the fallthrough protections were cross-compiler compatible

That would be [[fallthrough]]; It's part of the 2023 standard:

https://en.cppreference.com/w/c/language/attributes/fallthrough

but before that there was no standard. This is certainly an @ctruta thing; the GCC comment hack was always a comment hack and can't be implemented in any way that is even vaguely C90 compliant (or, indeed, anything until C23). The problem is that the hack is in the preprocessor, not the compiler, so that comment should always have disappeared into a space before the compiler gets invoked.

I haven't tested to see whether/*FALLTHROUGH*/ (difficult to speel correctly :-) works in a macro but @ctruta is a clanger too so maybe doesn't care; all that matters is that it gets done in one place!

[jbowler]

__attribute__(fallthrough); works with gcc, including gcc -std=c90 -pendantic. None of the other approaches, including:

#define FALLTHROUGH /*FALLTHROUGH*/

work. We're already using __attribute__in other cases including noreturn, warn_unused_result, deprecated and; well, quite a lot. The code in pngconf.h already has cases for clang, gcc, msvc and watcom. PNG_PASSTHROUGH could be added there at little risk. Pretty much the only risk is that a different compiler recognises /*FALLTHROUGH*/

[jbowler]

In fact [[fallthrough]] works with gcc with all --std versions so far as I can see. The trick is:

#ifdef __has_c_attribute
... test for fallthrough ...
#define PNGFallThrough [[fallthrough]];
#else
#define PNGFallThrough
#endif

This is completely compliant ANSI-C code which happens to work in GCC even if --std is defined to be less than 2023. The only issue for @ctruta is that -pedantic without --std=c23 reports, erroneously, that [[fallthrough]] is a C23 feature. I.e. the earlier --std versions do, in fact, "define" __has_c_attribute even with -pedantic and do, in fact, enable [[fallthrough]]; that's a clear GCC bug.

Some explanation: C23 added __has_c_attribute which has, in fact, some very weird behavior. GCC supports it even if --std=!c23 and, the bug, even if -pendantic.

[r-barnes]

Sqlite uses this:

#if defined(__has_attribute)
#  if __has_attribute(fallthrough)
#    define deliberate_fall_through __attribute__((fallthrough));
#  endif
#endif
#if !defined(deliberate_fall_through)
#  define deliberate_fall_through
#endif

Not sure if that's helpful or not.

[jbowler]

#if defined(__has_attribute)
#  if __has_attribute(fallthrough)
#    define deliberate_fall_through __attribute__((fallthrough));
#  endif

The word of god is in the standard (which I cannot afford to purchase) but cppreference.com (which I trust) says this:

https://en.cppreference.com/w/c/language/attributes

The relevant quote is:

Attributes provide the unified standard syntax for implementation-defined language extensions, such as the GNU and IBM language extensions attribute((...)), Microsoft extension __declspec(), etc.

So what sqlite is doing is not standard (it only works with GCC variants, which include clang and, IRC, ARM Ltd's native compiler).

No doubt sqlite might fix this in the future by checking for the C23 defined "has**c**attribute". A mess of twisty little #ifdefs all alike. Phlugh.

In my own code I'm just going for C23. I don't see any value in writing complex tests for diagnostics in out-of-date compilers; support, maybe (but I only support C99 and, maybe C11 or better) but diagnostics?

So my current approach is to, yes, use a #define for "fallthrough" (et al.) but only define it when it is not previously defined; so this "works" because someone who must can do a -D on the command line (or in CFLAGS) and those who don't just get a warning. The libpng approach is radically different; it tries to do everything for everyone and that's a problem :-)

[r-barnes]

@jbowler - In order to provide some more meat for us to discuss this, I've put up #572, which adds an appropriate macro and the accompanying warning flag to CMakeLists.txt. Compiling that code doesn't reveal any unannotated fallthroughs (nice!), at least for whatever set of macros the system set me up with.