Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Improve type traits recognition in __has_builtin #111516

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

cor3ntin
Copy link
Contributor

@cor3ntin cor3ntin commented Oct 8, 2024

__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 llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 8, 2024

@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/111516.diff

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+4-2)
  • (modified) clang/include/clang/Basic/TokenKinds.def (+4-1)
  • (modified) clang/lib/Lex/PPMacroExpansion.cpp (+33-19)
  • (modified) clang/test/Preprocessor/feature_tests.cpp (+4-1)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 44d5f348ed2d54..d17d45c9331614 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -404,6 +404,8 @@ Bug Fixes to Compiler Builtins
 
 - ``__noop`` can now be used in a constant expression. (#GH102064)
 
+- Fix ``__has_builtin`` incorrectly returning ``false`` for some C++ type traits. (#GH111477)
+
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -635,8 +637,8 @@ New features
   if class of allocation and deallocation function mismatches.
   `Documentation <https://clang.llvm.org/docs/analyzer/checkers.html#unix-mismatcheddeallocator-c-c>`__.
 
-- Function effects, e.g. the ``nonblocking`` and ``nonallocating`` "performance constraint" 
-  attributes, are now verified. For example, for functions declared with the ``nonblocking`` 
+- Function effects, e.g. the ``nonblocking`` and ``nonallocating`` "performance constraint"
+  attributes, are now verified. For example, for functions declared with the ``nonblocking``
   attribute, the compiler can generate warnings about the use of any language features, or calls to
   other functions, which may block.
 
diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def
index c5c3838407cf48..fdfb35de9cf287 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 2b62f573857ee8..63402de894594d 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1614,6 +1614,34 @@ asm("_ZNKSt8time_putIcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE3putES3_"
     "RSt8ios_basecPK2tmPKcSB_");
 #endif
 
+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) {
@@ -1812,25 +1840,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..a73d7dbe716d13 100644
--- a/clang/test/Preprocessor/feature_tests.cpp
+++ b/clang/test/Preprocessor/feature_tests.cpp
@@ -31,7 +31,10 @@
     !__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)
 #error Clang should have these
 #endif
 

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me. Its a shame that TRANSFORM_TYPE_TRAIT_DEF can't be done in terms of TYPE_TRAIT, but otherwiseI think this is a good direction.

@@ -635,8 +637,8 @@ New features
if class of allocation and deallocation function mismatches.
`Documentation <https://clang.llvm.org/docs/analyzer/checkers.html#unix-mismatcheddeallocator-c-c>`__.

- Function effects, e.g. the ``nonblocking`` and ``nonallocating`` "performance constraint"
attributes, are now verified. For example, for functions declared with the ``nonblocking``
- Function effects, e.g. the ``nonblocking`` and ``nonallocating`` "performance constraint"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated :(

Copy link

@timsong-cpp timsong-cpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a fix be backported to 19?

!__has_builtin(__has_unique_object_representations)
!__has_builtin(__has_unique_object_representations) || \
!__has_builtin(__is_trivially_equality_comparable) || \
!__has_builtin(__reference_constructs_from_temporary) || \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__reference_converts_from_temporary too?

`__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 llvm#111477
@cor3ntin cor3ntin force-pushed the corentin/cleanup_has_builtin branch from e44a9cd to b684ba4 Compare October 8, 2024 18:24
@cor3ntin cor3ntin merged commit 75611ca into llvm:main Oct 8, 2024
10 checks passed
cor3ntin added a commit to cor3ntin/llvm-project that referenced this pull request Oct 9, 2024
`__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 llvm#111477
tru pushed a commit to cor3ntin/llvm-project that referenced this pull request Oct 15, 2024
`__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 llvm#111477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__has_builtin(__reference_binds_to_temporary) became false in Clang 19
4 participants