This is an archive of the discontinued LLVM Phabricator instance.

Remove checks for old gcc versions for LLVM_ATTRIBUTE_*
ClosedPublic

Authored by aeubanks on Oct 11 2021, 2:51 PM.

Details

Summary

According to [1] we only support gcc 5.1+. So these checks for older gcc versions are not supported.

Some gcc 5.1+ versions still don't support has_builtin, so just check GNUC__ in those cases.

Add a missing #endif for LLVM_ATTRIBUTE_UNREACHABLE.

[1] https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

Diff Detail

Event Timeline

aeubanks created this revision.Oct 11 2021, 2:51 PM
aeubanks requested review of this revision.Oct 11 2021, 2:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2021, 2:51 PM
MaskRay accepted this revision.Oct 11 2021, 3:21 PM

Thanks!

This revision is now accepted and ready to land.Oct 11 2021, 3:21 PM
This revision was landed with ongoing or failed builds.Oct 11 2021, 4:18 PM
This revision was automatically updated to reflect the committed changes.
aeubanks reopened this revision.Oct 11 2021, 4:54 PM
This revision is now accepted and ready to land.Oct 11 2021, 4:54 PM
aeubanks edited the summary of this revision. (Show Details)Oct 11 2021, 4:56 PM
This revision was landed with ongoing or failed builds.Oct 12 2021, 11:20 AM
This revision was automatically updated to reflect the committed changes.
yrouban added inline comments.
llvm/include/llvm/Support/Compiler.h
100

For gcc this change results in LLVM_HAS_RVALUE_REFERENCE_THIS set to 0.
That is because __ has_feature(x) is always 0 for gcc regardless of the version (I use 7.4.0).
__has_feature is not and will not be defined for gcc. (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60512). Please consider fixing or reverting this patch.

aeubanks added inline comments.Oct 13 2021, 11:17 PM
llvm/include/llvm/Support/Compiler.h
100

sorry about that, should be fixed with 60605a2b8fa2

mstorsjo added inline comments.Oct 14 2021, 12:03 AM
llvm/include/llvm/Support/Compiler.h
100

FWIW, just for the record, with __has_feature(cxx_rvalue_references) || defined(__GNUC__), this would also end up enabled for Clang (which identifies itself as __GNUC__ normally) even for older versions of Clang that don't support __has_feature(cxx_rvalue_references). In this particular case, __has_feature(cxx_rvalue_references) is true since Clang 3.5 which also is our current minimum supported compiler version, so it's probably ok - but it could have been an issue.

dexonsmith added inline comments.Mar 21 2022, 11:49 AM
llvm/include/llvm/Support/Compiler.h
333–334

I don't think this was the correct thing to do since it subverts the checks for defined(LLVM_BUILTIN_UNREACHABLE). See review for a partial revert in:
https://reviews.llvm.org/D122167

Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 11:49 AM
Herald added a subscriber: StephenFan. · View Herald Transcript