[Clang] Improve type traits recognition in __has_builtin (#111516)

[cor3ntin]

__has_builtin was relying on reversible identifiers and string matching to recognize builtin-type traits, leading to some newer type traits not being recognized.

Fixes #111477

[llvmbot]

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

__has_builtin was relying on reversible identifiers and string matching to recognize builtin-type traits, leading to some newer type traits not being recognized.

Fixes #111477

---

Full diff: https://github.com/llvm/llvm-project/pull/111660.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/TokenKinds.def (+4-1)
• (modified) clang/lib/Lex/PPMacroExpansion.cpp (+33-19)
• (modified) clang/test/Preprocessor/feature_tests.cpp (+5-1)

diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def
index 8c54661e65cf46..0526fbf51bd91a 100644
--- a/clang/include/clang/Basic/TokenKinds.def
+++ b/clang/include/clang/Basic/TokenKinds.def
@@ -64,6 +64,10 @@
 #ifndef EXPRESSION_TRAIT
 #define EXPRESSION_TRAIT(I,E,K) KEYWORD(I,K)
 #endif
+#ifndef TRANSFORM_TYPE_TRAIT_DEF
+#define TRANSFORM_TYPE_TRAIT_DEF(K, Trait) KEYWORD(__##Trait, KEYCXX)
+#endif
+
 #ifndef ALIAS
 #define ALIAS(X,Y,Z)
 #endif
@@ -534,7 +538,6 @@ TYPE_TRAIT_1(__has_unique_object_representations,
 TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
 TYPE_TRAIT_2(__is_pointer_interconvertible_base_of, IsPointerInterconvertibleBaseOf, KEYCXX)
 
-#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) KEYWORD(__##Trait, KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"
 
 // Clang-only C++ Type Traits
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
index 3913ff08c2eb55..fb88ec2bf603fe 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1602,6 +1602,34 @@ static bool isTargetVariantEnvironment(const TargetInfo &TI,
   return false;
 }
 
+static bool IsBuiltinTrait(Token &Tok) {
+
+#define TYPE_TRAIT_1(Spelling, Name, Key)                                      \
+  case tok::kw_##Spelling:                                                     \
+    return true;
+#define TYPE_TRAIT_2(Spelling, Name, Key)                                      \
+  case tok::kw_##Spelling:                                                     \
+    return true;
+#define TYPE_TRAIT_N(Spelling, Name, Key)                                      \
+  case tok::kw_##Spelling:                                                     \
+    return true;
+#define ARRAY_TYPE_TRAIT(Spelling, Name, Key)                                  \
+  case tok::kw_##Spelling:                                                     \
+    return true;
+#define EXPRESSION_TRAIT(Spelling, Name, Key)                                  \
+  case tok::kw_##Spelling:                                                     \
+    return true;
+#define TRANSFORM_TYPE_TRAIT_DEF(K, Spelling)                                  \
+  case tok::kw___##Spelling:                                                   \
+    return true;
+
+  switch (Tok.getKind()) {
+  default:
+    return false;
+#include "clang/Basic/TokenKinds.def"
+  }
+}
+
 /// ExpandBuiltinMacro - If an identifier token is read that is to be expanded
 /// as a builtin macro, handle it and return the next token as 'Tok'.
 void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
@@ -1798,25 +1826,11 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
                 getTargetInfo().getTargetOpts().FeatureMap);
           }
           return true;
-        } else if (II->getTokenID() != tok::identifier ||
-                   II->hasRevertedTokenIDToIdentifier()) {
-          // Treat all keywords that introduce a custom syntax of the form
-          //
-          //   '__some_keyword' '(' [...] ')'
-          //
-          // as being "builtin functions", even if the syntax isn't a valid
-          // function call (for example, because the builtin takes a type
-          // argument).
-          if (II->getName().starts_with("__builtin_") ||
-              II->getName().starts_with("__is_") ||
-              II->getName().starts_with("__has_"))
-            return true;
-          return llvm::StringSwitch<bool>(II->getName())
-              .Case("__array_rank", true)
-              .Case("__array_extent", true)
-#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) .Case("__" #Trait, true)
-#include "clang/Basic/TransformTypeTraits.def"
-              .Default(false);
+        } else if (IsBuiltinTrait(Tok)) {
+          return true;
+        } else if (II->getTokenID() != tok::identifier &&
+                   II->getName().starts_with("__builtin_")) {
+          return true;
         } else {
           return llvm::StringSwitch<bool>(II->getName())
               // Report builtin templates as being builtins.
diff --git a/clang/test/Preprocessor/feature_tests.cpp b/clang/test/Preprocessor/feature_tests.cpp
index 00421d74e6282a..13e2a9a261b667 100644
--- a/clang/test/Preprocessor/feature_tests.cpp
+++ b/clang/test/Preprocessor/feature_tests.cpp
@@ -31,7 +31,11 @@
     !__has_builtin(__underlying_type) || \
     !__has_builtin(__is_trivial) || \
     !__has_builtin(__is_same_as) || \
-    !__has_builtin(__has_unique_object_representations)
+    !__has_builtin(__has_unique_object_representations) || \
+    !__has_builtin(__is_trivially_equality_comparable) || \
+    !__has_builtin(__reference_constructs_from_temporary) || \
+    !__has_builtin(__reference_binds_to_temporary) || \
+    !__has_builtin(__reference_converts_from_temporary)
 #error Clang should have these
 #endif
 

[cor3ntin]

Cherry pick as discussed in #111477

[tru]

@AaronBallman ok to backport?

[AaronBallman]

@AaronBallman ok to backport?

Yeah, seems reasonable to me.

[github-actions]

@cor3ntin (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

[ye-luo]

@tru This change breaks use of llvm 19.1.2 and 19.1.3 #113125
Can it be reverted on the release branch?

[AaronBallman]

@tru This change breaks use of llvm 19.1.2 and 19.1.3 #113125 Can it be reverted on the release branch?

Oof, that's unfortunate because this is cherry picking a fix where Clang 19 started returning false for __has_builtin(__reference_binds_to_temporary) when it was previously reporting true which is a pretty important bug to fix.

CC @cor3ntin @timsong-cpp

Note, WG21 meetings are happening next week and so folks are currently traveling and may not respond in a timely manner.

[erichkeane]

Note that the NVCC thing here is that NVCC has an interesting programming model for its offload.

My understanding is that:
-Uses the (in this case) Clang Preprocessor to process the code.
-Does some messing around with that preprocessed output, generating the device code, plus a modified 'host' preprocessed file.
-Compiles that preprocessed/modified file with the host (Clang in this case).

Those errors LOOK like they came out of Clang, and the bug report has the EDG Frontend of NVCC trying to emulate Clang 20 (--clang_version=200000).

That said, if Godbolt is correct, this might be a regression in our handling of that builtin: https://godbolt.org/z/r8od54M1P
Or is this patch FIXING that regression?

This makes me wonder if NVCC is somehow using 2 different versions of Clang here, 1 to do preprocessing that enables this, and an older version of 19.1.x to do the actual compilation?

@t-lutz is a member at Nvidia on the NVCC team that might be able to help.

[cor3ntin]

this patch fixes the regression in 1.9.2 (or 3, I do not remember). I have absolutely no idea why it would cause issues with nvcc though

[ye-luo]

I tried __has_builtin(__reference_converts_from_temporary https://godbolt.org/z/TPjbqWT67
18.1.0, 19.1.0 both return false
19.1.2 and main returns true.
I have no issue with __has_builtin fixed in the main but the resulted behavior change in a point release is a bit troublesome.

[AaronBallman]

Yes. but other builtins were also impacted and went from returning true (18.x) to false (19.x) to true (20.x): https://godbolt.org/z/8vPvzb81x

Just to double-check assumptions -- we don't know of any builtins which Clang now reports true for which Clang does not actually support? And we don't know of any builtins which Clang now reports false for which Clang does actually support? IOW, __has_builtin is not lying after landing these changes, right?

[ye-luo]

Just to double-check assumptions -- we don't know of any builtins which Clang now reports true for which Clang does not actually support? And we don't know of any builtins which Clang now reports false for which Clang does actually support? IOW, __has_builtin is not lying after landing these changes, right?

I have little knowledge about builtin. At least __has_builtin is not lying about __reference_converts_from_temporary. I think this is not debatable on main but in a point release, I have no issue with __has_builtin lying about __reference_converts_from_temporary support to prevent breakage and maximize compatibility.

It is an ugly situation that a point release fixes one problem but breaks another. Preferably get intended behavior in both cases but we don't live in a perfect world and do live with compromises.

[AaronBallman]

It is an ugly situation that a point release fixes one problem but breaks another.

100% agreed, it's an unfortunate situation to be in.

Preferably get intended behavior in both cases but we don't live in a perfect world and do live with compromises.

Agreed, though there's not much room for compromise here because we either give the correct value or we don't.

Is this the only builtin you're having issues with, or is this impacting a bunch of them? If it's just one or two builtins, would using a compiler version check help NVCC avoid the disruption?

[tru]

Ugh. This is unfortunate. I have no preference in how to fix this properly. Do we know anyone working on NVCC that can chime in? @joker-eph - do you know the right people?

[t-lutz]

We're looking at this internally at NVIDIA; we typically don't expect builtins to changes on a patch release and we'll have to accommodate for that in our frontend.