[Clang] Add attribute for consteval builtin functions #91894

MitalAshok · 2024-05-12T18:57:36Z

Builtins with the new Consteval attribute will also be marked Constexpr and will only be available in C++20 mode where consteval makes sense.

…ins as constexpr in C++ Also support redeclaring now-constexpr builtins without constexpr

llvmbot · 2024-05-12T18:58:05Z

@llvm/pr-subscribers-clang

Author: Mital Ashok (MitalAshok)

Changes

Also support redeclaring now-constexpr builtins without constexpr

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

6 Files Affected:

(modified) clang/include/clang/Basic/Builtins.h (+5)
(modified) clang/include/clang/Basic/BuiltinsBase.td (+2)
(modified) clang/lib/Sema/SemaDecl.cpp (+11-4)
(modified) clang/lib/Sema/SemaDeclCXX.cpp (+13-5)
(modified) clang/lib/Sema/SemaExpr.cpp (+6-2)
(modified) clang/test/Sema/builtin-redecl.cpp (+10-5)

diff --git a/clang/include/clang/Basic/Builtins.h b/clang/include/clang/Basic/Builtins.h
index f955d21169556..e85ec5b2dca14 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -280,6 +280,11 @@ class Context {
     return strchr(getRecord(ID).Attributes, 'E') != nullptr;
   }
 
+  /// Returns true if this is an immediate (consteval) function
+  bool isImmediate(unsigned ID) const {
+    return strchr(getRecord(ID).Attributes, 'G') != nullptr;
+  }
+
 private:
   const Info &getRecord(unsigned ID) const;
 
diff --git a/clang/include/clang/Basic/BuiltinsBase.td b/clang/include/clang/Basic/BuiltinsBase.td
index 724747ec76d73..1196b9e15c10d 100644
--- a/clang/include/clang/Basic/BuiltinsBase.td
+++ b/clang/include/clang/Basic/BuiltinsBase.td
@@ -70,6 +70,8 @@ class VScanfFormat<int I> : IndexedAttribute<"S", I>;
 
 // Builtin can be constant evaluated
 def Constexpr : Attribute<"E">;
+// Builtin is immediate and must be constant evaluated. Implies Constexpr.
+def Consteval : Attribute<"EG">;
 
 // Builtin kinds
 // =============
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index fb913034bd836..6b0a04585928a 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2409,10 +2409,17 @@ FunctionDecl *Sema::CreateBuiltin(IdentifierInfo *II, QualType Type,
     Parent = CLinkageDecl;
   }
 
-  FunctionDecl *New = FunctionDecl::Create(Context, Parent, Loc, Loc, II, Type,
-                                           /*TInfo=*/nullptr, SC_Extern,
-                                           getCurFPFeatures().isFPConstrained(),
-                                           false, Type->isFunctionProtoType());
+  ConstexprSpecKind ConstexprKind = ConstexprSpecKind::Unspecified;
+  if (getLangOpts().CPlusPlus && Context.BuiltinInfo.isConstantEvaluated(ID)) {
+    ConstexprKind = ConstexprSpecKind::Constexpr;
+    if (Context.BuiltinInfo.isImmediate(ID))
+      ConstexprKind = ConstexprSpecKind::Consteval;
+  }
+
+  FunctionDecl *New = FunctionDecl::Create(
+      Context, Parent, Loc, Loc, II, Type, /*TInfo=*/nullptr, SC_Extern,
+      getCurFPFeatures().isFPConstrained(), /*isInlineSpecified=*/false,
+      Type->isFunctionProtoType(), ConstexprKind);
   New->setImplicit();
   New->addAttr(BuiltinAttr::CreateImplicit(Context, ID));
 
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 53238d355ea09..1b558d70f9b48 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -676,11 +676,19 @@ bool Sema::MergeCXXFunctionDecl(FunctionDecl *New, FunctionDecl *Old,
   // template has a constexpr specifier then all its declarations shall
   // contain the constexpr specifier.
   if (New->getConstexprKind() != Old->getConstexprKind()) {
-    Diag(New->getLocation(), diag::err_constexpr_redecl_mismatch)
-        << New << static_cast<int>(New->getConstexprKind())
-        << static_cast<int>(Old->getConstexprKind());
-    Diag(Old->getLocation(), diag::note_previous_declaration);
-    Invalid = true;
+    if (Old->getBuiltinID() &&
+        Old->getConstexprKind() == ConstexprSpecKind::Constexpr &&
+        New->getConstexprKind() == ConstexprSpecKind::Unspecified) {
+      // Except allow redeclaring a builtin as non-constexpr to match C
+      // redeclarations which will not be constexpr
+      New->setConstexprKind(ConstexprSpecKind::Constexpr);
+    } else {
+      Diag(New->getLocation(), diag::err_constexpr_redecl_mismatch)
+          << New << static_cast<int>(New->getConstexprKind())
+          << static_cast<int>(Old->getConstexprKind());
+      Diag(Old->getLocation(), diag::note_previous_declaration);
+      Invalid = true;
+    }
   } else if (!Old->getMostRecentDecl()->isInlined() && New->isInlined() &&
              Old->isDefined(Def) &&
              // If a friend function is inlined but does not have 'inline'
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index bb4b116fd73ca..39aa32526d2b1 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -7095,8 +7095,12 @@ ExprResult Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl,
   }
 
   // Bail out early if calling a builtin with custom type checking.
-  if (BuiltinID && Context.BuiltinInfo.hasCustomTypechecking(BuiltinID))
-    return CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall);
+  if (BuiltinID && Context.BuiltinInfo.hasCustomTypechecking(BuiltinID)) {
+    ExprResult E = CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall);
+    if (!E.isInvalid() && Context.BuiltinInfo.isImmediate(BuiltinID))
+      E = CheckForImmediateInvocation(E, FDecl);
+    return E;
+  }
 
   if (getLangOpts().CUDA) {
     if (Config) {
diff --git a/clang/test/Sema/builtin-redecl.cpp b/clang/test/Sema/builtin-redecl.cpp
index 323c63e202883..31409a4d46a65 100644
--- a/clang/test/Sema/builtin-redecl.cpp
+++ b/clang/test/Sema/builtin-redecl.cpp
@@ -14,13 +14,18 @@ void __builtin_va_copy(double d);
 // expected-error@+2 {{cannot redeclare builtin function '__builtin_va_end'}}
 // expected-note@+1 {{'__builtin_va_end' is a builtin with type}}
 void __builtin_va_end(__builtin_va_list);
-// RUN: %clang_cc1 %s -fsyntax-only -verify 
-// RUN: %clang_cc1 %s -fsyntax-only -verify -x c
 
 void __va_start(__builtin_va_list*, ...);
 
+          void *__builtin_assume_aligned(const void *, size_t, ...);
 #ifdef __cplusplus
-void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
-#else
-void *__builtin_assume_aligned(const void *, size_t, ...);
+constexpr void *__builtin_assume_aligned(const void *, size_t, ...);
+          void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
+constexpr void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
+          void *__builtin_assume_aligned(const void *, size_t, ...) throw();
+constexpr void *__builtin_assume_aligned(const void *, size_t, ...) throw();
+
+// expected-error@+1 {{constexpr declaration of '__builtin_calloc' follows non-constexpr declaration}}
+constexpr void *__builtin_calloc(size_t, size_t);
+// expected-note@-1 {{previous declaration is here}}
 #endif

MitalAshok · 2024-05-12T19:12:46Z

Currently, GCC's behaviour for builtin function redeclarations is to just ignore constexpr: https://godbolt.org/z/e9jzjjrax. This is slightly stricter, allowing non-constexpr redeclarations for constexpr builtins but still disallowing constexpr declarations for non-constexpr builtins.

P2641R4 std::is_within_lifetime is most likely going to be implemented as a consteval builtin, which the attribute allows implementing easily as a builtin-function with getConstexprKind() == ConstexprSpecKind::Consteval.

cor3ntin · 2024-05-13T09:08:31Z

clang/test/Sema/builtin-redecl.cpp

+constexpr void *__builtin_assume_aligned(const void *, size_t, ...);
+          void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
+constexpr void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
+          void *__builtin_assume_aligned(const void *, size_t, ...) throw();
+constexpr void *__builtin_assume_aligned(const void *, size_t, ...) throw();
+


What is the motivation to allow that?

I kind of understand letting users defining builtin (actually, I am not sure I do). but once the builtin is redefined once, I think should be some sort of consistency with the language rules.
Here if you were to replace __builtin_assume_aligned with f, it would be ill-formed, for good reason.

For backwards compatibility (and compatibility with the same definitions in C or with GCC), to allow declarations with or without the constexpr. I suspect in any real code, it will always be declared without the constexpr.

The other solutions is to keep them all as ConstexprSpecKind::Unspecified like we currently do, but this is asymmetric with consteval builtins which will be marked ConstexprSpecKind::Consteval, and leads to the builtin versions of std::move/std::forward/constexpr cmath functions to have differing getConstexprKind() from their non-builtin counterparts

Does anyone know why we allow builtins to be redeclared at all? CC @zygoloid @rjmccall

I wonder just how much code we would break by not aiming for backwards compatibility here and requiring users to update their declarations on the assumption that declaring these at all is undefined behavior (using a reserved name) and so it's up to the user to match the compiler's behavior rather than make the compiler handle this with weird non-conforming language extension behavior.

I'm not looking to punish users in this case, but the workaround to keep things backwards compatible is pretty magical and awkward.

I don't think there are particularly good non-historical reasons why we allow non-LIBBUILTIN builtins to be redeclared -- I think it's allowed primarily for compatibility with GCC and existing code, and it's not clear to me why GCC allows it. There were some examples of code redeclaring builtins in this prior review thread.

@erichkeane

From reading through that (and trying to search my memory): A number of our LIBBUILTIN builtins are just replacements for things that would otherwise be implemented in the library.

So we need to allow them to be redeclared because they might be implemented in the library.

However, I would be unopposed to a patch that makes declaring a __builtin spelled as reserved identifier ill-formed.

That seems like that should go in a separate patch. For now, I'll change this one so ConstexprSpecKind::Constexpr isn't added to these builtins as before

…t make sense (!CPlusPlus20)

github-actions · 2024-05-14T18:03:37Z

✅ With the latest revision this PR passed the C/C++ code formatter.

github-actions · 2024-05-14T18:03:50Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 39d123f58a0e3c5f1a928940244b8dfd827fd4e5 a3aaf28d92499b1e76b49a0499be87b5b706155c -- clang/include/clang/Basic/Builtins.h clang/lib/Basic/Builtins.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaExpr.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 8554cb5972..c24ebc3ef4 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2373,7 +2373,8 @@ FunctionDecl *Sema::CreateBuiltin(IdentifierInfo *II, QualType Type,
 
   ConstexprSpecKind ConstexprKind = ConstexprSpecKind::Unspecified;
   if (Context.BuiltinInfo.isImmediate(ID)) {
-    assert(getLangOpts().CPlusPlus20 && "consteval builtins should only be available in C++20 mode");
+    assert(getLangOpts().CPlusPlus20 &&
+           "consteval builtins should only be available in C++20 mode");
     ConstexprKind = ConstexprSpecKind::Consteval;
   }

cor3ntin

LGTM
As discussed offline with @erichkeane we don't have good way to test this change until the patch to add is_within_lifetime lands

Summary: Builtins with the new `Consteval` attribute will also be marked `Constexpr` and will only be available in C++20 mode where `consteval` makes sense. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251624

[Clang] Add attribute for consteval builtins; Declare constexpr built…

56aed68

…ins as constexpr in C++ Also support redeclaring now-constexpr builtins without constexpr

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 12, 2024

MitalAshok mentioned this pull request May 12, 2024

[Clang] Add __builtin_is_within_lifetime to implement P2641R4's std::is_within_lifetime #91895

Merged

cor3ntin reviewed May 13, 2024

View reviewed changes

MitalAshok added 3 commits May 14, 2024 18:59

No longer declare constexpr builtins as constexpr

88c68cd

Prevent consteval builtins from being available when consteval doesn'…

ec88839

…t make sense (!CPlusPlus20)

Merge branch 'main' into consteval-builtins

a3aaf28

MitalAshok changed the title ~~[Clang] Add attribute for consteval builtins; Declare constexpr builtins as constexpr in C++~~ [Clang] Add attribute for consteval builtin functions May 14, 2024

clang-format

b498e90

cor3ntin approved these changes May 14, 2024

View reviewed changes

cor3ntin merged commit 396a5ba into llvm:main Jul 17, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Add attribute for consteval builtin functions #91894

[Clang] Add attribute for consteval builtin functions #91894

MitalAshok commented May 12, 2024 •

edited

Loading

llvmbot commented May 12, 2024

MitalAshok commented May 12, 2024

cor3ntin May 13, 2024 •

edited

Loading

MitalAshok May 13, 2024

AaronBallman May 13, 2024

zygoloid May 13, 2024

cor3ntin May 14, 2024

erichkeane May 14, 2024

MitalAshok May 14, 2024

github-actions bot commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024

cor3ntin left a comment

[Clang] Add attribute for consteval builtin functions #91894

[Clang] Add attribute for consteval builtin functions #91894

Conversation

MitalAshok commented May 12, 2024 • edited Loading

llvmbot commented May 12, 2024

MitalAshok commented May 12, 2024

cor3ntin May 13, 2024 • edited Loading

Choose a reason for hiding this comment

MitalAshok May 13, 2024

Choose a reason for hiding this comment

AaronBallman May 13, 2024

Choose a reason for hiding this comment

zygoloid May 13, 2024

Choose a reason for hiding this comment

cor3ntin May 14, 2024

Choose a reason for hiding this comment

erichkeane May 14, 2024

Choose a reason for hiding this comment

MitalAshok May 14, 2024

Choose a reason for hiding this comment

github-actions bot commented May 14, 2024 • edited Loading

github-actions bot commented May 14, 2024

cor3ntin left a comment

Choose a reason for hiding this comment

MitalAshok commented May 12, 2024 •

edited

Loading

cor3ntin May 13, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading