[clang] All {con,de}structor attributes to use template args #67376

abrachet · 2023-09-25T21:40:48Z

Fixes #67154

Fixes llvm#67154

llvmbot · 2023-09-25T21:41:48Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Changes

Fixes #67154

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

6 Files Affected:

(modified) clang/include/clang/Basic/Attr.td (+6-2)
(modified) clang/lib/CodeGen/CodeGenModule.cpp (+14-3)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+22-11)
(modified) clang/test/CodeGenCXX/constructor-attr.cpp (+14)
(added) clang/test/CodeGenCXX/destructor-attr.cpp (+20)
(added) clang/test/Sema/ctor-dtor-attr.cpp (+13)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index dd4d45171db4899..8c4e63bebf110cf 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1156,9 +1156,11 @@ def ConstInit : InheritableAttr {
 
 def Constructor : InheritableAttr {
   let Spellings = [GCC<"constructor">];
-  let Args = [DefaultIntArgument<"Priority", 65535>];
+  let Args = [ExprArgument<"Priority", 1>];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [CtorDtorDocs];
+  let TemplateDependent = 1;
+  let AdditionalMembers = [{ static const int DefaultPriority = 65535; }];
 }
 
 def CPUSpecific : InheritableAttr {
@@ -1422,9 +1424,11 @@ def Deprecated : InheritableAttr {
 
 def Destructor : InheritableAttr {
   let Spellings = [GCC<"destructor">];
-  let Args = [DefaultIntArgument<"Priority", 65535>];
+  let Args = [ExprArgument<"Priority", 1>];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [CtorDtorDocs];
+  let TemplateDependent = 1;
+  let AdditionalMembers = [{ static const int DefaultPriority = 65535; }];
 }
 
 def EmptyBases : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 130cedcd4f2bf68..e93c36bbce95ec6 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -32,6 +32,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
@@ -5661,10 +5662,20 @@ void CodeGenModule::EmitGlobalFunctionDefinition(GlobalDecl GD,
   setNonAliasAttributes(GD, Fn);
   SetLLVMFunctionAttributesForDefinition(D, Fn);
 
-  if (const ConstructorAttr *CA = D->getAttr<ConstructorAttr>())
-    AddGlobalCtor(Fn, CA->getPriority());
+  auto getPrio = [this](auto *Attr) -> int {
+    Expr *E = Attr->getPriority();
+    if (!E)
+      return Attr->DefaultPriority;
+    if (auto CE = E->getIntegerConstantExpr(getContext()))
+      return CE->getExtValue();
+    return Attr->DefaultPriority;
+  };
+
+  if (const ConstructorAttr *CA = D->getAttr<ConstructorAttr>()) {
+    AddGlobalCtor(Fn, getPrio(CA));
+  }
   if (const DestructorAttr *DA = D->getAttr<DestructorAttr>())
-    AddGlobalDtor(Fn, DA->getPriority(), true);
+    AddGlobalDtor(Fn, getPrio(DA), true);
   if (D->hasAttr<AnnotateAttr>())
     AddGlobalAnnotations(D, Fn);
   if (getLangOpts().OpenMP && D->hasAttr<OMPDeclareTargetDeclAttr>())
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..d857f5e78ae97a3 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,37 @@ static void handleUnusedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
+template <typename Attr>
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t priority = Attr::DefaultPriority;
+  Expr *E = nullptr;
+  if (AL.getNumArgs()) {
+    E = AL.getArgAsExpr(0);
+    if (E->isValueDependent()) {
+      if (!E->isTypeDependent() && !E->getType()->isIntegerType()) {
+        S.Diag(getAttrLoc(AL), diag::err_attribute_argument_type)
+            << &AL << AANT_ArgumentIntegerConstant << E->getSourceRange();
+        return;
+      }
+    } else if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), priority)) {
+      return;
+    }
+  }
+
+  D->addAttr(::new (S.Context) Attr(S.Context, AL, E));
+}
+
 static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
   if (S.getLangOpts().HLSL && AL.getNumArgs()) {
     S.Diag(AL.getLoc(), diag::err_hlsl_init_priority_unsupported);
     return;
   }
-  if (AL.getNumArgs() &&
-      !checkUInt32Argument(S, AL, AL.getArgAsExpr(0), priority))
-    return;
 
-  D->addAttr(::new (S.Context) ConstructorAttr(S.Context, AL, priority));
+  handleCtorDtorAttr<ConstructorAttr>(S, D, AL);
 }
 
 static void handleDestructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = DestructorAttr::DefaultPriority;
-  if (AL.getNumArgs() &&
-      !checkUInt32Argument(S, AL, AL.getArgAsExpr(0), priority))
-    return;
-
-  D->addAttr(::new (S.Context) DestructorAttr(S.Context, AL, priority));
+  return handleCtorDtorAttr<DestructorAttr>(S, D, AL);
 }
 
 template <typename AttrTy>
diff --git a/clang/test/CodeGenCXX/constructor-attr.cpp b/clang/test/CodeGenCXX/constructor-attr.cpp
index ec27ed210ce760f..af0e147d3e51057 100644
--- a/clang/test/CodeGenCXX/constructor-attr.cpp
+++ b/clang/test/CodeGenCXX/constructor-attr.cpp
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -o - %s | FileCheck %s
 
 // CHECK: @llvm.global_ctors
+// CHECK-SAME: i32 101, ptr @_Z18template_dependentILi101EEvv
+// CHECK-SAME: i32 108, ptr @_Z18template_dependentILi108EEvv
+// CHECK-SAME: i32 111, ptr @_Z19template_dependent2ILi111EEvv
 
 // PR6521
 void bar();
@@ -10,3 +13,14 @@ struct Foo {
     bar();
   }
 };
+
+template <int P>
+[[gnu::constructor(P)]] void template_dependent() {}
+
+template void template_dependent<101>();
+template void template_dependent<100 + 8>();
+
+template <int P>
+__attribute__((constructor(P))) void template_dependent2() {}
+
+template void template_dependent2<111>();
diff --git a/clang/test/CodeGenCXX/destructor-attr.cpp b/clang/test/CodeGenCXX/destructor-attr.cpp
new file mode 100644
index 000000000000000..0f05385187165cf
--- /dev/null
+++ b/clang/test/CodeGenCXX/destructor-attr.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @llvm.global_dtors
+// CHECK-SAME: i32 101, ptr @_Z18template_dependentILi101EEvv
+// CHECK-SAME: i32 108, ptr @_Z18template_dependentILi108EEvv
+
+// PR6521
+void bar();
+struct Foo {
+  // CHECK-LABEL: define linkonce_odr {{.*}}void @_ZN3Foo3fooEv
+  static void foo() __attribute__((constructor)) {
+    bar();
+  }
+};
+
+template <int P>
+[[gnu::destructor(P)]] void template_dependent() {}
+
+template void template_dependent<101>();
+template void template_dependent<100 + 8>();
diff --git a/clang/test/Sema/ctor-dtor-attr.cpp b/clang/test/Sema/ctor-dtor-attr.cpp
new file mode 100644
index 000000000000000..a6340805b43c2d3
--- /dev/null
+++ b/clang/test/Sema/ctor-dtor-attr.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-strict-prototypes %s
+
+template <int I> [[gnu::constructor(I)]] void ok_ctor();
+template <int I> __attribute__((constructor(I))) void ok2_ctor();
+
+template <int *I> [[gnu::constructor(I)]] void bad_ctor(); // expected-error {{'constructor' attribute requires an integer constant}}
+template <int *I> __attribute__((constructor(I))) void bad2_ctor(); // expected-error {{'constructor' attribute requires an integer constant}}
+
+template <int I> [[gnu::destructor(I)]] void ok_ctor();
+template <int I> __attribute__((destructor(I))) void ok2_dtor();
+
+template <int *I> [[gnu::destructor(I)]] void bad_dtor(); // expected-error {{'destructor' attribute requires an integer constant}}
+template <int *I> __attribute__((destructor(I))) void bad2_dtor(); // expected-error {{'destructor' attribute requires an integer constant}}

erichkeane

So it looks like this breaks the ast-dump-attr.cpp test, so that needs fixing.

Also, there is quite a bit of changes happening around the SemaDeclAttr.cpp changes by @AaronBallman, so the two need to be merged in some way.

erichkeane · 2023-09-26T00:20:50Z

clang/include/clang/Basic/Attr.td

  let Subjects = SubjectList<[Function]>;
  let Documentation = [CtorDtorDocs];
+  let TemplateDependent = 1;
+  let AdditionalMembers = [{ static const int DefaultPriority = 65535; }];


@AaronBallman : I see in your other patch (#67360) you're using this as a magic number! Might be useful to get on the same page as this author.

Suggested change

let AdditionalMembers = [{ static const int DefaultPriority = 65535; }];

let AdditionalMembers = [{ static constexpr int DefaultPriority = 65535; }];

erichkeane · 2023-09-26T00:21:28Z

clang/include/clang/Basic/Attr.td

  let Subjects = SubjectList<[Function]>;
  let Documentation = [CtorDtorDocs];
+  let TemplateDependent = 1;
+  let AdditionalMembers = [{ static const int DefaultPriority = 65535; }];


Suggested change

let AdditionalMembers = [{ static const int DefaultPriority = 65535; }];

let AdditionalMembers = [{ static constexpr int DefaultPriority = 65535; }];

Also wonder if these should be unsigned?

erichkeane · 2023-09-26T00:22:16Z

clang/test/CodeGenCXX/constructor-attr.cpp

@@ -10,3 +13,14 @@ struct Foo {
    bar();
  }
 };
+
+template <int P>


Please write a test where the argument is NTTP dependent on another template argument.

erichkeane · 2023-09-26T00:23:05Z

clang/lib/CodeGen/CodeGenModule.cpp

+      return Attr->DefaultPriority;
+    if (auto CE = E->getIntegerConstantExpr(getContext()))
+      return CE->getExtValue();
+    return Attr->DefaultPriority;


This condition seems to me should be an assertion, right?

erichkeane · 2023-09-26T00:24:16Z

clang/lib/Sema/SemaDeclAttr.cpp

+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t priority = Attr::DefaultPriority;
+  Expr *E = nullptr;
+  if (AL.getNumArgs()) {


We also should ensure we don't have more than 1 argument.

erichkeane · 2023-09-26T00:25:10Z

So it looks like this breaks the ast-dump-attr.cpp test, so that needs fixing.

Also, there is quite a bit of changes happening around the SemaDeclAttr.cpp changes by @AaronBallman, so the two need to be merged in some way.

"Merged" could be: 1-rebased on the other, but ONE Of the two things needs to happen.

cor3ntin · 2024-05-14T06:44:16Z

@abrachet Are you still working on that?

abrachet · 2024-05-14T13:17:22Z

No, feel free to commandeer this patch

[clang] All {con,de}structor attributes to use template args

5317e66

Fixes llvm#67154

abrachet requested review from shafik and erichkeane September 25, 2023 21:40

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Sep 25, 2023

erichkeane reviewed Sep 26, 2023

View reviewed changes

erichkeane mentioned this pull request Sep 26, 2023

Diagnose problematic uses of constructor/destructor attribute #67360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] All {con,de}structor attributes to use template args #67376

[clang] All {con,de}structor attributes to use template args #67376

abrachet commented Sep 25, 2023

llvmbot commented Sep 25, 2023 •

edited

Loading

erichkeane left a comment

erichkeane Sep 26, 2023

erichkeane Sep 26, 2023

erichkeane Sep 26, 2023

erichkeane Sep 26, 2023

erichkeane Sep 26, 2023

erichkeane commented Sep 26, 2023

cor3ntin commented May 14, 2024

abrachet commented May 14, 2024

	let AdditionalMembers = [{ static const int DefaultPriority = 65535; }];
	let AdditionalMembers = [{ static constexpr int DefaultPriority = 65535; }];

[clang] All {con,de}structor attributes to use template args #67376

Are you sure you want to change the base?

[clang] All {con,de}structor attributes to use template args #67376

Conversation

abrachet commented Sep 25, 2023

llvmbot commented Sep 25, 2023 • edited Loading

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Sep 26, 2023

Choose a reason for hiding this comment

erichkeane Sep 26, 2023

Choose a reason for hiding this comment

erichkeane Sep 26, 2023

Choose a reason for hiding this comment

erichkeane Sep 26, 2023

Choose a reason for hiding this comment

erichkeane Sep 26, 2023

Choose a reason for hiding this comment

erichkeane commented Sep 26, 2023

cor3ntin commented May 14, 2024

abrachet commented May 14, 2024

llvmbot commented Sep 25, 2023 •

edited

Loading