Move the [[noreturn]] attribute to the front of the declaration

[DMaroo]

C++17 requires attributes to be placed before the declaration specifiers. GCC 12 (and greater) seem to fail to compile when this syntax is not followed. There are no behavioral changes, only the attribute's position is moved.

Similar issue: jarro2783/cxxopts#353.

[xavierleroy]

Thanks for pointing this out. I was not aware that attributes had stronger constraints than the _Noreturn modifier. A quick check with clang-15 -std=c2x suggests that C23 has the same constraints as C++17, i.e. [[noreturn]] static int f(void) is OK but static [[noreturn]] int f(void) is not.

The proposed fix is fine, but I'm concerned that the problem will come back over and over again as we add new "noreturn" functions to the runtime system and compile with pre-C23 compilers. That [[noreturn]] should go first is visually apparent. However, when the attribute is hidden behind macros, it's not visually obvious that CAMLnoret CAMLextern is good and CAMLextern CAMLnoret is bad. But I don't see any other way out of this problem, except going back to using GCC/Clang attribute((noreturn)).

[DMaroo]

Wouldn't updating the CI to use a newer version of the compiler (C23 compliant) prevent any non-compliant changes from being merged? I think it may (?) be safe to assume that this behavior of the [[noreturn]] attribute won't be changing in the future versions of the compilers.

[xavierleroy]

Wouldn't updating the CI to use a newer version of the compiler (C23 compliant) prevent any non-compliant changes from being merged?

Yes, eventually. Right now, C23 is not yet published as a standard, support by GCC and Clang is spotty, and C23 is not their default mode, so it will take some time and some effort to get C23 in CI.

[DMaroo]

Makes sense.

[xavierleroy]

I'm making slow progress on this issue. It turns out that one of our CI machines runs the latest Ubuntu version, which has clang version 15, which supports a lot of C23, including the [[noreturn]] attribute. However, selecting this C23 preliminary support with -std=c2x or -std=gnu2x sets __STDC_VERSION__ to 202000L, too low for the [[noreturn]] definition of CAMLnoret to be picked. However squared, the following incantation works

CC=clang CFLAGS="-std=gnu2x -U__STDC_VERSION__ -D__STDC_VERSION__=202300L" ./configure

in that it turns C23 features on and pretends to be C23 compliant.

With that I can see the "misplaced attributes" errors, check that this PR fixes a number of them, but leaves out some others, which I could fix easily. (I'll add the fix to this PR soon.) So, it seems to work well as a "C23 readiness" check, and could be integrated in the Jenkins CI at Inria.

[DMaroo]

Ah okay. It's weird that one needs to manually set __STDC_VERSION__ to get C23 features. Anyways, thank you for adding the remaining fixes.

[xavierleroy]

We need to do something here, since #12235 introduced an incompatibility with C++17 and the upcoming C23: either we revert #12235 or move forward and merge this PR. I move to merge this PR, and try to have it tested by Jenkins CI as outlined above. If it doesn't work it will always be possible to revert both this PR and #12235.

[gasche]

Your plan (merge here + improve CI) sounds excellent to me. Is there something that would help besides this bystander opinion?

(Note: it would be nice to have a comment on the CAMLnoret definition that it needs to be placed at the beginning of the declaration.)

[xavierleroy]

(Note: it would be nice to have a comment on the CAMLnoret definition that it needs to be placed at the beginning of the declaration.)

Good idea! See 8c02bd9. Time to merge!

[xavierleroy]

We're out of luck: clang 17 refuses to un-define __STDC_VERSION__ ...

In file included from <built-in>:385:
<command line>:1:8: error: undefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
    1 | #undef __STDC_VERSION__

Checkmate! I give up on trying to test C23 conformance before C23 is officially released. Too bad...