The internal define `fallthrough` leaks to usercode

[bjosv]

When a source file includes both flatcc and Boost the following problem appears:

staging/include/boost/config/compiler/clang.hpp:314:26: error: missing ')' after '('
#if !__has_cpp_attribute(fallthrough) || __cplusplus < 201406L
                         ^~~~~~~~~~~
staging/include/flatcc/portable/pattributes.h:74:22: note: expanded from macro 'fallthrough'
# define fallthrough pattribute(fallthrough)
                     ^~~~~~~~~~~~~~~~~~~~~~~
staging/include/flatcc/portable/pattributes.h:69:23: note: expanded from macro 'pattribute'
#define pattribute(x) pattribute_##x
                      ^~~~~~~~~~~~~~
<scratch space>:358:1: note: expanded from here
pattribute_fallthrough
^~~~~~~~~~~~~~~~~~~~~~
staging/include/flatcc/portable/pattributes.h:63:46: note: expanded from macro 'pattribute_fallthrough'
# define pattribute_fallthrough __attribute__((__fallthrough__))
                                             ^
staging/include/boost/config/compiler/clang.hpp:314:25: note: to match this '('
#if !__has_cpp_attribute(fallthrough) || __cplusplus < 201406L
                        ^
1 error generated.

Is there a suggestion of a remedy to a problems like this?

I am new to flatcc's code, but there seem to be a use of both fallthrough and pattribute(fallthrough). Would it make sense to replace all fallthrough with pattribute(fallthrough) or is there a reason why using both variants?

[mikkelfj]

Hi Björn, as you are a new user let me explain a bit first: flatcc relies on the portable library to work with many C compilers and version while keeping its main source code in roughly C11 standard compliance even if the compiler is older or newer, or just different. GCC made a lot of noise about switch statement fall through and flatcc treats warnings as errors (mostly) (I note that you ae using clang C++ here).

There is no single method to silence fallthrough warnings because the accepted approach has changed several times. The portable library attempts to detect the compiler and version and then use the best approach. The main source code then uses just a single approach abstracted away in: https://github.com/dvidelabs/flatcc/blob/master/include/flatcc/portable/pattributes.h

In you case this conflicts with either Boost or standard Clang libraries.

To fix this, you need to patch pattributes.h. I cannot do this, but I'd like to support. The main concern is that it might break other users if not done carefully.

In this case it might work to simply detect if C++ is defined, but I suspect that is not enough. It might also the code is doing the right thing, but has a trivial bug in a preprocessor branch used infrequently. It could be that boost just defines something without checking and the conflicting with flatccs attempt to patch.

I think the error log you printed has some hints for adding C++ version checks to the pattributes.h file.

To test a much smaller setting, you could just include the pattributes file into a small project with switch statement using the fallthough attribute.

You can see an example here: https://github.com/dvidelabs/flatcc/blob/62a7f88611af08c68be5485ce6d9c7e92474eab8/include/flatcc/flatcc_json_parser.h#L246-L266 Note the fallthrough comment is outdated. / fallthrough / style comments longer works with all compilers so flatcc uses attribute syntax that sometimes expands to comments sometimes depending on pattributes.h.

[mikkelfj]

To answer your last question:

Would it make sense to replace all fallthrough with pattribute(fallthrough) or is there a reason why using both variants?

pattribute(x) is just a mechanism to define attibutes consistently in the portable library. It is also used as a backdoor to use when the portable attributes cannot be exposed (see PORTABLE_EXPOSE_ATTRIBUTES). Replacing with pattribute(fallthough) is not desireable because flatcc (ideally) does not depend on portable features when there is a modern (C11) standard feature present. attribute(fallthrough) would be a better approach, but that would require implementing attribute for older compiles, which likely makes the problem worse. Replacing with /* fallthrough */ has been done before, but later GCC ruined that.

I think what is going wrong is that portable decides that fallthrough needs to be defined and thus defines it. Boost goes through the same line of thinking, but the check for if the attribute has been defined fails because fallthrough is a defined symbol (by flatcc) and this apparantly causes problems.

For other attributes and similar keywords it is common to have a x_is_defined symbol to check for, but that was not available for fallthrough.

To avoid exposing fallthrough, assuming boost takes care of that, you could define -DPORTABLE_EXPOSE_ATTRIBUTES=0 in the build settings.

You might also get away with defining PORTABLE_HAS_C_ATTRIBUTE or PORTABLE_HAS_ATTRIBUTE or maybe it works to reorder the include files so Boost gets a chance to define first. I'm not sure if it is possible to check for boos without relying on include order.

However, flatcc will not work without fallthrough defined somehow because 1. that is how it is implemented, and 2. the alternatives are worse. You can thank GCC for this mess.

[mikkelfj]

fallthrough; is actually neither a standard nor a compiler extension. Clang uses FALLTHROUGH for backwards compat.: https://clang.llvm.org/docs/LanguageExtensions.html but the Linux kernel does use fallthrough https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through More discussion here: https://stackoverflow.com/questions/45129741/gcc-7-wimplicit-fallthrough-warnings-and-portable-way-to-clear-them

So flatcc chose the Linux kernel convention - which breaks Boost.

[bjosv]

Thanks for your thorough reply, much appreciated! It doesn't seem easy to please all the variants of compilers and versions regarding fallthrough We will experiment a bit with -DPORTABLE_EXPOSE_ATTRIBUTES=0 or see if there would be a need to patch pattributes.h.

[mikkelfj]

@bjosv If you wan't to contribute, we can rewrite all uses of fallthrough; to use goto lbl_xxx; and add labels instead of falling through.

[bjosv]

Sure, I have seen that there is something similar in external/lex/luthor.c. I will prepare a PR to see how it can be.

[mikkelfj]

Sounds good. I think all generated code is already doing that. The JSON parser has a lot of jumps to labels and there shouldn't be any fall throughs, just mentioning it.

[mikkelfj]

Maybe separate the PR in two commits, one for runtime, and one for compiler / CLI.