Fix `fallthrough` attribute

t-mat commented

2021-08-25 13:55:02 -05:00

(Migrated from github.com)

There're 2 trivial changes

(1) Fix clang version checking.

Only clang-10 and later supports GNU style attribute __attribute__((fallthrough)).

Without this change, CC=clang-9 make clean all fails with the following warning.

clang-9 -O3     -c -o xxhash.o xxhash.c
In file included from xxhash.c:43:
./xxhash.h:1959:26: warning: declaration does not declare anything [-Wmissing-declarations]
                         XXH_FALLTHROUGH;
                         ^
./xxhash.h:619:32: note: expanded from macro 'XXH_FALLTHROUGH'
#       define XXH_FALLTHROUGH __attribute__ ((fallthrough))
                               ^

Please refer the following Clang documentations for fallthrough.

(2) Add __STDC_VERSION__ checking for gcc-11 -std=c90 -pedantic.

CC=gcc-11 make clean all reports the following warning as an error.

gcc-11 -std=c90 -pedantic -Wno-long-long -Werror -Wall -Wextra -Wconversion -Wcast-qual -Wcast-align -Wshadow -Wstrict-aliasing=1 -Wswitch-enum -Wdeclaration-after-statement -Wstrict-prototypes -Wundef -Wpointer-arith -Wformat-security -Wvla -Wformat=2 -Winit-self -Wfloat-equal -Wwrite-strings -Wredundant-decls -Wstrict-overflow=2 -c -o xxhash.o xxhash.c

In file included from xxhash.c:43:
xxhash.h: In function ‘XXH32_finalize’:
xxhash.h:557:32: error: ISO C does not support ‘[[]]’ attributes before C2X [-Werror=pedantic]
557 | # define XXH_FALLTHROUGH [[fallthrough]]

There're 2 trivial changes (1) Fix clang version checking. Only `clang-10` and later supports GNU style attribute `__attribute__((fallthrough))`. Without this change, `CC=clang-9 make clean all` fails with the following warning. ``` clang-9 -O3 -c -o xxhash.o xxhash.c In file included from xxhash.c:43: ./xxhash.h:1959:26: warning: declaration does not declare anything [-Wmissing-declarations] XXH_FALLTHROUGH; ^ ./xxhash.h:619:32: note: expanded from macro 'XXH_FALLTHROUGH' # define XXH_FALLTHROUGH __attribute__ ((fallthrough)) ^ ``` Please refer the following Clang documentations for `fallthrough`. - https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html#fallthrough - https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html#fallthrough - https://releases.llvm.org/3.9.0/tools/clang/docs/AttributeReference.html#fallthrough-clang-fallthrough (2) Add `__STDC_VERSION__` checking for `gcc-11 -std=c90 -pedantic`. `CC=gcc-11 make clean all` reports the following warning as an error. ``` gcc-11 -std=c90 -pedantic -Wno-long-long -Werror -Wall -Wextra -Wconversion -Wcast-qual -Wcast-align -Wshadow -Wstrict-aliasing=1 -Wswitch-enum -Wdeclaration-after-statement -Wstrict-prototypes -Wundef -Wpointer-arith -Wformat-security -Wvla -Wformat=2 -Winit-self -Wfloat-equal -Wwrite-strings -Wredundant-decls -Wstrict-overflow=2 -c -o xxhash.o xxhash.c In file included from xxhash.c:43: xxhash.h: In function ‘XXH32_finalize’: xxhash.h:557:32: error: ISO C does not support ‘[[]]’ attributes before C2X [-Werror=pedantic] 557 | # define XXH_FALLTHROUGH [[fallthrough]] ```

👍 1

Cyan4973 commented

2021-08-25 14:05:53 -05:00

(Migrated from github.com)

Thanks @t-mat !

👍 1

Cyan4973 commented

2021-08-25 14:40:48 -05:00

(Migrated from github.com)

Note :
I just made a compilation test after merging this PR on my macos laptop,
using default compiler, which is kind of an apple's special clang,
Apple clang version 11.0.3 (clang-1103.0.32.62),
and got the following warning :

warning: declaration does not declare anything [-Wmissing-declarations]
                         XXH_FALLTHROUGH;
                         ^
./xxhash.h:619:32: note: expanded from macro 'XXH_FALLTHROUGH'
#       define XXH_FALLTHROUGH __attribute__ ((fallthrough))

Note : I just made a compilation test _after_ merging this PR on my macos laptop, using default compiler, which is kind of an apple's special `clang`, `Apple clang version 11.0.3 (clang-1103.0.32.62)`, and got the following warning : ``` warning: declaration does not declare anything [-Wmissing-declarations] XXH_FALLTHROUGH; ^ ./xxhash.h:619:32: note: expanded from macro 'XXH_FALLTHROUGH' # define XXH_FALLTHROUGH __attribute__ ((fallthrough)) ```

👀 1

t-mat commented

2021-08-25 15:04:21 -05:00

(Migrated from github.com)

I've failed to reproduce the warning with this PR and clang-11.

I could run the following command without error/warning.

cd
git clone https://github.com/Cyan4973/xxHash.git xxHash-pull-575
cd xxHash-pull-575
git reset b051c96 --hard    # force resetting
git rev-parse --short HEAD  # verify actual commit hash
CC=clang-11 make clean all

My clang-11 is

$ clang-11 --version
Ubuntu clang version 11.0.0-2~ubuntu20.04.1
Target: x86_64-pc-linux-gnu

I'll investigate the difference.

I've failed to reproduce the warning with this PR and `clang-11`. I could run the following command without error/warning. ```sh cd git clone https://github.com/Cyan4973/xxHash.git xxHash-pull-575 cd xxHash-pull-575 git reset b051c96 --hard # force resetting git rev-parse --short HEAD # verify actual commit hash CC=clang-11 make clean all ``` My `clang-11` is ```sh $ clang-11 --version Ubuntu clang version 11.0.0-2~ubuntu20.04.1 Target: x86_64-pc-linux-gnu ``` I'll investigate the difference.

Cyan4973 commented

2021-08-25 15:17:19 -05:00

(Migrated from github.com)

I believe that Apple clang is not the same as clang,
especially when it comes to version numbers.

That points to a need to test macos in CI
(which btw is already available on GA : https://github.com/Cyan4973/xxHash/runs/3425515504 ,
but it runs Apple clang version 12.0.0 (clang-1200.0.32.29) )

edit : I presume that Apple clang 11 is effectively based on clang-9.
In the past, I remember that Apple clang used to be always in advance compared to regular clang,
and +2 versions is what I had kept in mind. Plus, this would match the symptom pretty well.
However, one problem is that Apple clang hijacks the __clang_major__ macro, pretending being version 11 on my laptop.

I believe that `Apple clang` is not the same as `clang`, especially when it comes to version numbers. That points to a need to test `macos` in CI (which btw is already available on GA : https://github.com/Cyan4973/xxHash/runs/3425515504 , but it runs `Apple clang version 12.0.0 (clang-1200.0.32.29)` ) _edit_ : I _presume_ that `Apple clang 11` is effectively based on `clang-9`. In the past, I remember that `Apple clang` used to be always in advance compared to regular `clang`, and +2 versions is what I had kept in mind. Plus, this would match the symptom pretty well. However, one problem is that `Apple clang` hijacks the `__clang_major__` macro, _pretending_ being version 11 on my laptop.

👍 1

t-mat commented

2021-08-25 15:41:51 -05:00

(Migrated from github.com)

Sorry. Since my macbook has been bricked, I don't use macos nowadays :(

Accoring to Xcode toolchain version history at Wikipedia,

Apple Clang 11.0.3 is LLVM 9.0.0 based
Apple Clang 12.0.0 -> LLVM 10.0.0
Apple Clang 12.0.5 -> LLVM 11.1.0
Apple Clang 13.0.0 -> LLVM 12.0.0

So, we can detect it with the following macro checking?

#if defined(__APPLE__) && defined(__clang__) /* Apple Clang (macos or ios) */
#  if (__clang_major__ >= 12) /* Apple Clang 12.0.0+ -> LLVM 10.0+ */
#    ...
#  endif
#elif defined(__clang__) /* mainline clang */
#  ...
#endif

Sorry. Since my macbook has been bricked, I don't use macos nowadays :( Accoring to [Xcode toolchain version history at Wikipedia](https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_-_13.x_(since_SwiftUI_framework)_2), - Apple Clang 11.0.3 is LLVM 9.0.0 based - Apple Clang 12.0.0 -> LLVM 10.0.0 - Apple Clang 12.0.5 -> LLVM 11.1.0 - Apple Clang 13.0.0 -> LLVM 12.0.0 So, we can detect it with the following macro checking? ```C #if defined(__APPLE__) && defined(__clang__) /* Apple Clang (macos or ios) */ # if (__clang_major__ >= 12) /* Apple Clang 12.0.0+ -> LLVM 10.0+ */ # ... # endif #elif defined(__clang__) /* mainline clang */ # ... #endif ```

👍 1

Cyan4973 commented

2021-08-25 15:43:27 -05:00

(Migrated from github.com)

Yep, that would work.

👍 1

t-mat commented

2021-08-25 15:48:33 -05:00

(Migrated from github.com)

@Cyan4973 , since GA doesn't provide older version of Apple clang so far, could you add and test version checking for Apple clang?

I'll look for we can add older Apple clang to GA or not.

@Cyan4973 , since GA doesn't provide older version of Apple clang so far, could you add and test version checking for Apple clang? I'll look for we can add older Apple clang to GA or not.

Cyan4973 commented

2021-08-25 15:52:05 -05:00

(Migrated from github.com)

Sure.

And don't worry for Apple tests on GA.
I'm glad they already exist, existing tests will allow us to detect the most glaring issues specific to macos.
For older versions, I'm not too hopeful either, but well, there is always more to test, we have to accept a few blind spots.

Sure. And don't worry for Apple tests on GA. I'm glad they already exist, existing tests will allow us to detect the most glaring issues specific to `macos`. For older versions, I'm not too hopeful either, but well, there is always more to test, we have to accept a few blind spots.

👍 1

TheNicker commented

2021-08-26 00:27:20 -05:00

(Migrated from github.com)

(2) Add STDC_VERSION checking for gcc-11 -std=c90 -pedantic.

That would solve the issue, but then if the compiler supports C23 standard, there's no need to check specific capabilities using the has_attribute. that way a user can implement a subset of the new standards to come, without the need for compiler vendors to be fully standard compliant.

With that said, I think __has_[c/cpp]_attribute doesn't work as expected, on MSVC it respect the language standard (-std:) in Clang and probably in GCC it doesn't. so in effect it becomes a general compiler version capability macro. since MSVC does work as expected I believe this feature isn't fully implemented in gcc and clang, that's why we had weird behavior as @t-mat mentioned here : https://github.com/Cyan4973/xxHash/pull/553#issuecomment-899730309

If the above is correct, then it's a chaotic scenario since these compilers are already out there.
The optimal solution would probably be to have compilers versions as a initial condition when defining base macros such as INLINE and FALLTHROUGH and not counting on the language standard (as much as we want to).

Theory is fun, in practice is frustrating.

> (2) Add __STDC_VERSION__ checking for gcc-11 -std=c90 -pedantic. That would solve the issue, but then if the compiler supports C23 standard, there's no need to check specific capabilities using the `has_attribute`. that way a user can implement a subset of the new standards to come, without the need for compiler vendors to be fully standard compliant. With that said, I think `__has_[c/cpp]_attribute` doesn't work as expected, on MSVC it respect the language standard `(-std:)` in Clang and probably in GCC it doesn't. so in effect it becomes a general compiler version capability macro. since MSVC does work as expected I believe this feature isn't fully implemented in gcc and clang, that's why we had weird behavior as @t-mat mentioned here : https://github.com/Cyan4973/xxHash/pull/553#issuecomment-899730309 If the above is correct, then it's a chaotic scenario since these compilers are already out there. The optimal solution would probably be to have compilers versions as a initial condition when defining base macros such as INLINE and FALLTHROUGH and not counting on the language standard (as much as we want to). Theory is fun, in practice is frustrating.

Rows
Columns

Fix fallthrough attribute #575

Fix `fallthrough` attribute #575