Annotate fallthrough paths with attribute

[compnerd]

This uses the GNU extension of __attribute__((__fallthrough__)) to indicate the fallthrough paths. The comments were introduced in GCC 7, which introduced this attribute. Adding this attribute additionally silences the warning from clang.

[mikkelfj]

Thanks, but that is bit too ugly with compiler detection conditionals spread all over the code.

Can you explain a bit more about how and why clang now complains without supporting GNU comment convention - or does with a slightly different spelling? Or is there a clang warning that can be disabled?

Alternatively, the portable library should add a new fallthrough.h facility that defines a macro that implements fallthrough.

How do other projects handle this?

[compnerd]

I agree that this is ugly due to the macro. I couldn't figure out how to ensure that the header would be included by default, but that would be better. If you could point me to how to ensure that the header is always included, I could refactor this. You could disable the warning using -Wno-implicit-fallthrough, though it seems better to address this for better static analysis. I don't believe that clang ever supported the comment form - the attribute is explicitly indicating the fallthrough - which one could argue is the alternative (and compatible) spelling. I believe most projects would just use a macro to avoid the explicit check at each location, which would be possible.

[mikkelfj]

I agree with better static analysis, but at this point, I just about have it with compilers breaking the code repeatedly over this issue. So I am inclided to just disable the warning, unless GCC and Clang can agree on something portable that doesn't involve this kind of mess.

[mikkelfj]

As to including in a common header, I can help look into that. There is a flatcc header that includes the most relevant files from portable which will get included just about everywhere. But I am not quite ready to invest in that.

[compnerd]

__attribute__((__fallthrough__)) is supported by both gcc[>=7], which is what the comments were introduced for in the first place, and clang. However, __attribute__ is not supported by MSVC and will require a check no matter what.

[compnerd]

@mikkelfj - I think that this was the alternative version. I couldn't figure out how to plumb this through portable.h, but this does show how it could work.

[mikkelfj]

I'll need to think about this a bit.

As to your latest fallthrough commit, that is fine as a proof of concept. There are some style changes required such as portable not being flatcc specific, so it would define either FALLTHROUGH or PFALLTHROUGH, or lower case ditto. The file would be named pfallthrough.h, and some flatcc header would include the portable header, and possibly redefine PFALLTHROUG to FLATCC_FALLTHROUGH, or just use FALLTHROUGH directly in switch statements.

But I'm mostly inclided to just disable the warning.

[mikkelfj]

Se also [clang] detect switch fallthrough marked by a comment (PR43465)

[mikkelfj]

Linux kernel uses fallthrough; in the code, but not sure how fallthrough is defined. I suspected this would be a convention which is why I mentioned lower case in the above comment.

[mikkelfj]

C17 apparently introduces [[fallthough]] syntax similar to C++17, but that will never be portable because the code must also work with older & different compilers.

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

Also, I supect defining fallthough as a macro will not work with C17 syntax. This is unusual - normally C standards make sure you can be backwards compatible through macro definitions.

Linux note

https://mjmwired.net/kernel/Documentation/process/deprecated.rst

As there have been a long list of flaws due to missing "break" statements <https://cwe.mitre.org/data/definitions/484.html>, we no longer allow implicit fall-through. In order to identify intentional fall-through cases, we have adopted a pseudo-keyword macro "fallthrough" which expands to gcc's extension __attribute__((__fallthrough__)) <https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html>. (When the C17/C18 [[fallthrough]] syntax is more commonly supported by C compilers, static analyzers, and IDEs, we can switch to using that syntax for the macro pseudo-keyword.)

[mikkelfj]

https://stackoverflow.com/a/60682054

I think the solution is to have PORTABLE_FALLTHROUGH similar to LLVM_FALLTHROUGH and then define FLATCC_FALLTHROUGH as PORTABLE_FALLTHROUGH where a flatcc header includes "portable/pfallthrough.h".

EDIT: except it needs to be adapted for C, not C++, but must also work for C++ since some C++ users include flatcc source.

[mikkelfj]

Or maybe it will work with fallthrough as a macro. We need to test that against C17 and C++17. That would be the cleanest solution.

You are welcome to do some research. Once we reach a conclusion I will probably write a pfallthrough.h file and put it on a branch where you can pick it up just to make sure the portable file follows convention. I'll also find a hook for its inclusion.

[compnerd]

I'm pretty sure that the solution here is portable across C/C++ with both clang and gcc: https://godbolt.org/z/zav7PshnM

[mikkelfj]

The question is if fallthrough is defined as __fallthrough__ because then defining a new falltrough could have interesting effects. Mere testing #ifndef fallthrough won't work then because semantics are different.

[compnerd]

It is not, it is defined as __attribute__((__fallthrough__)): https://github.com/torvalds/linux/blob/master/include/linux/compiler_attributes.h#L210

[mikkelfj]

No, that is just Linux source. They are doing the same thing we are trying to do - that is not authoritative, but a reason to use #ifndef. The question is what C17 and C++17 do, and possibly other compilers. I quick check on godbolt suggests it is not a problem to define fallthrough and also that it isn't defined already, but who knows what stdlib header might do that?

[compnerd]

The fallthrough that you are referring to IS a Linux extension. The C2x PoR is to use the C++ style [[fallthrough]] which will be compatible with C++ (the attribute is not required to be namespaced). [[fallthrough]], [[gnu::fallthrough]], [[clang::fallthrough]] are all interchangeable with C++11. C2x will adopt the same spelling as per WG14.

Using fallthrough is permissible, that is not something that the system may co-opt, it is well within the user namespace. The question is about other libraries using the same name - the problem exists for any public interface. That is why I opted to namespace it with FLATCC_ in the patch.

[mikkelfj]

C11 does a lot of macro definitions so you cannot conclude fallthrough is freely available along that line of reasoning, although it appears to be. For example alignas is widely used in flatcc and emulated in the portable library. It is a C11 macro.

Also, I'm not arguing that it isn't a Linux extension, on the contrary. But is still a compatibility patch.

BTW: I wonder if there should be a pattributes.h instead of pfallthrough.h in portable. That would allow for adding other common attributes similar to the Linux file you found.

[compnerd]

C11 does a lot of macro definitions so you cannot conclude fallthrough is freely available along that line of reasoning, although it appears to be. For example alignas is widely used in flatcc and emulated in the portable library. It is a C11 macro.

C2X draft §6.7.11.5p1 states that fallthrough shall not be treated as a token literal outside of the attribute declarator.

Edit: re-reading your comment - oh, you are wondering if they will have the compatibility spelling of fallthrough. The stdalign.h extension was added for compatibility and requires opt-in by inclusion. That is, I don't think that the collision there should be a concern.

[mikkelfj]

That is useful info, although it still doesn't preclude fallthrough being defined as a macro in some library header.

So I think we should define a portable/pattributes.h file inspired by Linux and LLVM definitions. I'll whip something up and get back to you.

[compnerd]

BTW: I wonder if there should be a pattributes.h instead of pfallthrough.h in portable. That would allow for adding other common attributes similar to the Linux file you found.

That is probably a good idea. fallthrough is just one of the interesting attributes introduced in C2x. There is also [[deprecated]] (aka __attribute__((__deprecated__(...)))) as well as [[nodiscard]] (aka __attribute__((__warn_unused_result__))).

[mikkelfj]

I just read you edit: I agree I don't think it is a concern, I am just not sure. It is kind of annoying they didn't handle this the same as alignas, but I can see that they wanted to avoid attributes being sprinkled around without [[ ]] markers. But unfornately that makes it totally not portable leaving it for random users like us and Linux library developers to come up with suitable conventions.

[compnerd]

In the case of alignas it was for compatibility with C++ - C++ used the alignas expression, C used the compatible _Alignas keyword. The stdalign.h extension allowed it to be compatible across C/C++. This time, they did the compatibility by adding [[...]] attribute spellings.

[mikkelfj]

I have now added https://github.com/dvidelabs/flatcc/blob/attributes/include/flatcc/portable/pattributes.h to the attributes branch.

The file is included here: https://github.com/dvidelabs/flatcc/blob/8bdaefa84f999d82d9b0ce61b716b1a356423ce6/include/flatcc/flatcc_flatbuffers.h#L26-L27

I also fixed pprintint.h in the portable library.

The changes to config.h should not be necessary. I patched up luthor.c with a goto statement to avoid introducing a dependency on the portable library.

I think you should be able to just use fallthrough; in the remaining source.

[compnerd]

I'm not sure how to update the other instances. If you were to merge that change to main, I can rebase this change and update it.

[mikkelfj]

I can merge to master, but please test on the branch first, notably make sure it is running on Linux if you can. Unfortunately Travis has stopped working, and I don't have your problem on my system.

Also please verify that you have what you need aside from trival fallthrough; changes. I.e. that you don't need to include anything extra. I did test one case briefly.

[compnerd]

@mikkelfj seems that I do need to include flatcc/portable/pattributes.h in a few different instances. This PR includes the contents of the attributes branch and the change needed on top to build on Linux.

[mikkelfj]

Yeah, I think that is reasonable. This is in the compiler. It could make sense to put it in config.h but I think it is better is you have done. Only, I suggest you add a /* fallthrough */ comment on the include line of pattributes.

I'll merge my changes to master so I get clean commits on the different source code areas.

[mikkelfj]

BTW: do you have any suggestions to the pattributes.h file. I kind of like how it turned out without having to test for a lot of compiler versions, but I might have missed something.

[mikkelfj]

attributes branch has been merged

[compnerd]

I think that its pretty good for a starting point. It covers the major compilers (gcc, clang, MSVC). With the recent changes, ARMCC/DS and xlC/xlClang should also be covered by the existing file. The one other larger compiler which may be interesting for comparison would be GHC, but I don't know enough about that one to speculate.

[mikkelfj]

OK thanks. GHC? Great Hadron Collider, or Glasgow Haskell Compiler?

Latest commit looks fine. Ready to merge?

[compnerd]

GHC is the Green Hills Software C compiler. But, I'll take the other two expansions as well :-p. Yes, I believe this is ready to merge.

[mikkelfj]

Thanks

[mikkelfj]

FYI:

I updated pattributes.h such that names like fallthrough can be optionally namespaced using pattribute(fallthrough)

https://github.com/dvidelabs/flatcc/commit/f06da091109d19296ff603e0f91bf9c7eb23e983

[mikkelfj]

The change did not handle already defined names correctly. Fixed. Also, this makes it possible to use the portable attribute even if the name is used for something else by using pattribute(<attribute>).

https://github.com/dvidelabs/flatcc/commit/c6c2064ec1f1eb8e9734c7d3a1105fa6102eba9a

And here a link the latest version of the full file for reference https://github.com/dvidelabs/flatcc/blob/master/include/flatcc/portable/pattributes.h