[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman created 
https://github.com/llvm/llvm-project/pull/67360

Functions with these attributes will be automatically called before main() or 
after main() exits gracefully, which means the functions should not accept 
arguments or have a returned value (nothing can provide an argument to the call 
in these cases, and nothing can use the returned value), nor should they be 
allowed on a non-static member function in C++.

Additionally, these reuse the same priority logic as the init_priority 
attribute which explicitly reserved priorty values <= 100 or > 65535. So we now 
diagnose use of reserved priorities the same as we do for the init_priority 
attribute.

>From 6e9bcfa7fc5eb3da4592909b9e199215064df700 Mon Sep 17 00:00:00 2001
From: Aaron Ballman 
Date: Mon, 25 Sep 2023 15:03:56 -0400
Subject: [PATCH] Diagnose problematic uses of constructor/destructor attribute

Functions with these attributes will be automatically called before
main() or after main() exits gracefully, which means the functions
should not accept arguments or have a returned value (nothing can
provide an argument to the call in these cases, and nothing can use
the returned value), nor should they be allowed on a non-static member
function in C++.

Additionally, these reuse the same priority logic as the init_priority
attribute which explicitly reserved priorty values <= 100 or > 65535.
So we now diagnose use of reserved priorities the same as we do for the
init_priority attribute.
---
 clang/docs/ReleaseNotes.rst   |  5 ++
 .../clang/Basic/DiagnosticSemaKinds.td|  5 ++
 clang/lib/Sema/SemaDeclAttr.cpp   | 74 +--
 clang/test/CodeGen/2008-03-03-CtorAttrType.c  |  4 +-
 .../PowerPC/aix-constructor-attribute.c   | 20 ++---
 .../PowerPC/aix-destructor-attribute.c| 30 +++-
 .../CodeGenCXX/aix-constructor-attribute.cpp  | 20 ++---
 .../CodeGenCXX/aix-destructor-attribute.cpp   | 36 -
 clang/test/Sema/constructor-attribute.c   | 60 ---
 9 files changed, 147 insertions(+), 107 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 294b77a3e260908..a9ef2e2c378c7e3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return
+type, or
+  - the function it is applied to is a non-static member function (C++).
 
 Improvements to Clang's diagnostics
 ---
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..1c522500fedcf6d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3176,6 +3176,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' return type">;
+def err_ctor_dtor_member_func : Error<
+  "%0 attribute cannot be applied to a member function">;
 def err_attribute_sizeless_type : Error<
   "%0 attribute cannot be applied to sizeless type %1">;
 def err_attribute_argument_n_type : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..79a73ba4618c85d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return f

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang


Changes

Functions with these attributes will be automatically called before main() or 
after main() exits gracefully, which means the functions should not accept 
arguments or have a returned value (nothing can provide an argument to the call 
in these cases, and nothing can use the returned value), nor should they be 
allowed on a non-static member function in C++.

Additionally, these reuse the same priority logic as the init_priority 
attribute which explicitly reserved priorty values <= 100 or > 65535. So 
we now diagnose use of reserved priorities the same as we do for the 
init_priority attribute.

---

Patch is 21.08 KiB, truncated to 20.00 KiB below, full version: 
https://github.com/llvm/llvm-project/pull/67360.diff


9 Files Affected:

- (modified) clang/docs/ReleaseNotes.rst (+5) 
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5) 
- (modified) clang/lib/Sema/SemaDeclAttr.cpp (+51-23) 
- (modified) clang/test/CodeGen/2008-03-03-CtorAttrType.c (+1-3) 
- (modified) clang/test/CodeGen/PowerPC/aix-constructor-attribute.c (+6-14) 
- (modified) clang/test/CodeGen/PowerPC/aix-destructor-attribute.c (+11-19) 
- (modified) clang/test/CodeGenCXX/aix-constructor-attribute.cpp (+6-14) 
- (modified) clang/test/CodeGenCXX/aix-destructor-attribute.cpp (+14-22) 
- (modified) clang/test/Sema/constructor-attribute.c (+48-12) 


``diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 294b77a3e260908..a9ef2e2c378c7e3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return
+type, or
+  - the function it is applied to is a non-static member function (C++).
 
 Improvements to Clang's diagnostics
 ---
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..1c522500fedcf6d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3176,6 +3176,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' return type">;
+def err_ctor_dtor_member_func : Error<
+  "%0 attribute cannot be applied to a member function">;
 def err_attribute_sizeless_type : Error<
   "%0 attribute cannot be applied to sizeless type %1">;
 def err_attribute_argument_n_type : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..79a73ba4618c85d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-static void handleDestru

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {

cor3ntin wrote:

Going from the comment, shouldn't it be 

```cpp
(Priority < 101 && !S.getSourceManager().isInSystemHeader(A.getLoc())) || 
Priority > 65535)
```

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;

cor3ntin wrote:

Where does `65535` comes from? GCC compat?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments
+  // and return no value (but it is not an error because it is theoretically

cor3ntin wrote:

I don't get the parenthesis: how is it not an error when the diag itself is an 
error?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Shafik Yaghmour via cfe-commits]

https://github.com/shafik edited https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Shafik Yaghmour via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&

shafik wrote:

So `101` and `65535` are "magic" numbers that are used multiple times, can we 
name them please.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Shafik Yaghmour via cfe-commits]

https://github.com/shafik commented:

Should they be valid for `constexpr` functions?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return

erichkeane wrote:

I don't think the 'non-void' return type should be diagnosed unless it is 
'nodiscard'.  Marking something like 
'closeAllHandlesAndReturnAStatusIIntendToIgnore` as a destructor isn't harmful 
IMO.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments
+  // and return no value (but it is not an error because it is theoretically
+  // possible to call the function during normal program execution and pass it
+  // valid values). It also cannot be a member function. We allow K&R C
+  // functions because that's a difficult edge case where it depends on how the
+  // function is defined as to whether it does or does not expect arguments.
+  auto *FD = cast(D);
+  if (!FD->getReturnType()->isVoidType() ||

erichkeane wrote:

As before, I think the 'no params' makes sense for obvious reasons.  I think 
making the 'non-void' should be a warning, and perhaps a 2nd warning if it is 
no-discard.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments
+  // and return no value (but it is not an error because it is theoretically
+  // possible to call the function during normal program execution and pass it
+  // valid values). It also cannot be a member function. We allow K&R C
+  // functions because that's a difficult edge case where it depends on how the
+  // function is defined as to whether it does or does not expect arguments.
+  auto *FD = cast(D);
+  if (!FD->getReturnType()->isVoidType() ||
+  (FD->hasPrototype() && FD->getNumParams() != 0)) {
+S.Diag(AL.getLoc(), diag::err_ctor_dtor_attr_on_non_void_func)
+<< AL << FD->getSourceRange();
 return;
+  } else if (auto *MD = dyn_cast(FD); MD && MD->isInstance()) {
+S.Diag(AL.getLoc(), diag::err_ctor_dtor_member_func)
+<< AL << FD->getSourceRange();
+return;
+  }
 
-  D->addAttr(::new (S.Context) DestructorAttr(S.Context, AL, priority));
+  D->addAttr(::new (S.Context) CtorDtorAttr(S.Context, AL, Priority));

erichkeane wrote:

I thought we were preferring to use the AttrName::Create methods these days?  
Can we switch to that?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Timm Baeder via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments
+  // and return no value (but it is not an error because it is theoretically
+  // possible to call the function during normal program execution and pass it
+  // valid values). It also cannot be a member function. We allow K&R C
+  // functions because that's a difficult edge case where it depends on how the
+  // function is defined as to whether it does or does not expect arguments.
+  auto *FD = cast(D);
+  if (!FD->getReturnType()->isVoidType() ||
+  (FD->hasPrototype() && FD->getNumParams() != 0)) {
+S.Diag(AL.getLoc(), diag::err_ctor_dtor_attr_on_non_void_func)
+<< AL << FD->getSourceRange();
 return;
+  } else if (auto *MD = dyn_cast(FD); MD && MD->isInstance()) {

tbaederr wrote:

```suggestion
  } else if (const auto *MD = dyn_cast(FD); MD && 
MD->isInstance()) {
```

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {

AaronBallman wrote:

Ah, no, I can clarify the comment -- the 65535 is the documented upper bound, 
but we could reasonably extend that in the future if we wanted.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;

AaronBallman wrote:

Yup, GCC compat: https://godbolt.org/z/hYa3bPMPW

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments
+  // and return no value (but it is not an error because it is theoretically

AaronBallman wrote:

Good catch! That comment is stale, I'll correct it.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> Should they be valid for `constexpr` functions?

Sure, but perhaps not for `consteval` functions. A `constexpr` function can be 
deferred to runtime, whereas a `consteval` one cannot. WDYT?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&

AaronBallman wrote:

Eh, I'm on the fence given that there are three different attributes which are 
unrelated except for having the notion of a priority. I'll make some local 
variables for this rather than try to associate it with a particular attribute.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return

AaronBallman wrote:

I was on the fence about diagnosing the return value, but ultimately decided it 
was a type error to have a non-void return value because of calling 
conventions. e.g., 
```
struct S {
  long double ld1, ld2;
};

__attribute__((constructor))
struct S func(void) {
  return (struct S){ 1.0l, 2.0l };
}
```
where the returned struct is actually returned via a hidden pointer passed to 
the function. I don't want to play whack-a-mole with signature checking unless 
there's some compelling use case in the wild where people actually are using 
these functions to obtain a value outside of ctor/dtor calls.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments
+  // and return no value (but it is not an error because it is theoretically
+  // possible to call the function during normal program execution and pass it
+  // valid values). It also cannot be a member function. We allow K&R C
+  // functions because that's a difficult edge case where it depends on how the
+  // function is defined as to whether it does or does not expect arguments.
+  auto *FD = cast(D);
+  if (!FD->getReturnType()->isVoidType() ||
+  (FD->hasPrototype() && FD->getNumParams() != 0)) {
+S.Diag(AL.getLoc(), diag::err_ctor_dtor_attr_on_non_void_func)
+<< AL << FD->getSourceRange();
 return;
+  } else if (auto *MD = dyn_cast(FD); MD && MD->isInstance()) {
+S.Diag(AL.getLoc(), diag::err_ctor_dtor_member_func)
+<< AL << FD->getSourceRange();
+return;
+  }
 
-  D->addAttr(::new (S.Context) DestructorAttr(S.Context, AL, priority));
+  D->addAttr(::new (S.Context) CtorDtorAttr(S.Context, AL, Priority));

AaronBallman wrote:

Sure, I can switch to that!

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return

erichkeane wrote:

I'm more considering a case where someone wants to use an otherwise 'normal' 
function as their constructor/destructor, where the return value is perhaps 
useful if the program is continuing (but then obviously not when it is not).  I 
would think that the RETURN should at minimum be only a warning (or perhaps 
warning-as-error?), since this is otherwise a breaking change.

The params list being nothing makes sense, because there is no valid code 
generation that did that before AFAIK, but an ignorable return type is 
meaningful.  In your `func` case, I don't see a problem with that, presume the 
1st line was something like:
` if (!GlobalAllocator) InitAllocator(GlobalAllocator); `
and they were using the `constructor` call to initialize that (and not caring 
about the object it was returning).  This would cause this error to be breaking.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

erichkeane wrote:

> runtime, whereas a `consteval` one cannot. WDYT?

This makes sense to me.  Also, see my comment on 
https://github.com/llvm/llvm-project/pull/67376.  It looks like someone else is 
messing with this area as well, and are editing things in an incompatible way, 
so we should make sure that these don't conflict.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman updated 
https://github.com/llvm/llvm-project/pull/67360

>From 6e9bcfa7fc5eb3da4592909b9e199215064df700 Mon Sep 17 00:00:00 2001
From: Aaron Ballman 
Date: Mon, 25 Sep 2023 15:03:56 -0400
Subject: [PATCH 1/2] Diagnose problematic uses of constructor/destructor
 attribute

Functions with these attributes will be automatically called before
main() or after main() exits gracefully, which means the functions
should not accept arguments or have a returned value (nothing can
provide an argument to the call in these cases, and nothing can use
the returned value), nor should they be allowed on a non-static member
function in C++.

Additionally, these reuse the same priority logic as the init_priority
attribute which explicitly reserved priorty values <= 100 or > 65535.
So we now diagnose use of reserved priorities the same as we do for the
init_priority attribute.
---
 clang/docs/ReleaseNotes.rst   |  5 ++
 .../clang/Basic/DiagnosticSemaKinds.td|  5 ++
 clang/lib/Sema/SemaDeclAttr.cpp   | 74 +--
 clang/test/CodeGen/2008-03-03-CtorAttrType.c  |  4 +-
 .../PowerPC/aix-constructor-attribute.c   | 20 ++---
 .../PowerPC/aix-destructor-attribute.c| 30 +++-
 .../CodeGenCXX/aix-constructor-attribute.cpp  | 20 ++---
 .../CodeGenCXX/aix-destructor-attribute.cpp   | 36 -
 clang/test/Sema/constructor-attribute.c   | 60 ---
 9 files changed, 147 insertions(+), 107 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 294b77a3e260908..a9ef2e2c378c7e3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return
+type, or
+  - the function it is applied to is a non-static member function (C++).
 
 Improvements to Clang's diagnostics
 ---
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..1c522500fedcf6d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3176,6 +3176,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' return type">;
+def err_ctor_dtor_member_func : Error<
+  "%0 attribute cannot be applied to a member function">;
 def err_attribute_sizeless_type : Error<
   "%0 attribute cannot be applied to sizeless type %1">;
 def err_attribute_argument_n_type : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..79a73ba4618c85d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argumen

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> > runtime, whereas a `consteval` one cannot. WDYT?
> 
> This makes sense to me. Also, see my comment on #67376. It looks like someone 
> else is messing with this area as well, and are editing things in an 
> incompatible way, so we should make sure that these don't conflict.

Good catch -- I think it would make sense to land these changes here first and 
then rebase on top of it, because this is generalizing the existing code in a 
way that should make it slightly easier to handle template instantiations (and 
hopefully handle them for `constructor`, `destructor`, and `init_priority` at 
the same time).

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return

AaronBallman wrote:

> I'm more considering a case where someone wants to use an otherwise 'normal' 
> function as their constructor/destructor, where the return value is perhaps 
> useful if the program is continuing (but then obviously not when it is not). 
> I would think that the RETURN should at minimum be only a warning (or perhaps 
> warning-as-error?), since this is otherwise a breaking change.

That was the case that concerned me as well, but I've not seen any evidence of 
such code existing in the wild.

> The params list being nothing makes sense, because there is no valid code 
> generation that did that before AFAIK, but an ignorable return type is 
> meaningful. In your func case, I don't see a problem with that, presume the 
> 1st line was something like:
> ```
> if (!GlobalAllocator) InitAllocator(GlobalAllocator);
> ```
> and they were using the constructor call to initialize that (and not caring 
> about the object it was returning). This would cause this error to be 
> breaking.

Look at the codegen for my example and that should make it more clear why it's 
an issue: https://godbolt.org/z/qncWqqovb

The problem is that there's no way to return from that function without 
inducing UB. You either return a value (and the hidden pointer is invalid so 
you just did an out of bounds write somewhere) or you don't return any value 
(no out of bounds write to the hidden pointer, but oops, UB to fall off the end 
of the function that way).

You can devise circumstances where it's safe, but unless there's evidence that 
people use the attribute this way, I don't see a compelling reason to give 
people a gun to point at their feet.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[via cfe-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 8586cd5ad8a7fa4f84b5913cbeaf634d68500095 
5c9509d2765f2cc803757b2d151a672f8571ab15 -- clang/lib/Sema/SemaDeclAttr.cpp 
clang/test/CodeGen/2008-03-03-CtorAttrType.c 
clang/test/CodeGen/PowerPC/aix-constructor-attribute.c 
clang/test/CodeGen/PowerPC/aix-destructor-attribute.c 
clang/test/CodeGenCXX/aix-constructor-attribute.cpp 
clang/test/CodeGenCXX/aix-destructor-attribute.cpp 
clang/test/Sema/constructor-attribute.c
``





View the diff from clang-format here.


``diff
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 9a5de7eda641..f80e0d39ef65 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2412,7 +2412,7 @@ static void handleCtorDtorAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
 << AL << FD->getSourceRange();
 return;
   }
-  
+
   D->addAttr(CtorDtorAttr::Create(S.Context, Priority, AL));
 }
 

``




https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman updated 
https://github.com/llvm/llvm-project/pull/67360

>From 6e9bcfa7fc5eb3da4592909b9e199215064df700 Mon Sep 17 00:00:00 2001
From: Aaron Ballman 
Date: Mon, 25 Sep 2023 15:03:56 -0400
Subject: [PATCH 1/3] Diagnose problematic uses of constructor/destructor
 attribute

Functions with these attributes will be automatically called before
main() or after main() exits gracefully, which means the functions
should not accept arguments or have a returned value (nothing can
provide an argument to the call in these cases, and nothing can use
the returned value), nor should they be allowed on a non-static member
function in C++.

Additionally, these reuse the same priority logic as the init_priority
attribute which explicitly reserved priorty values <= 100 or > 65535.
So we now diagnose use of reserved priorities the same as we do for the
init_priority attribute.
---
 clang/docs/ReleaseNotes.rst   |  5 ++
 .../clang/Basic/DiagnosticSemaKinds.td|  5 ++
 clang/lib/Sema/SemaDeclAttr.cpp   | 74 +--
 clang/test/CodeGen/2008-03-03-CtorAttrType.c  |  4 +-
 .../PowerPC/aix-constructor-attribute.c   | 20 ++---
 .../PowerPC/aix-destructor-attribute.c| 30 +++-
 .../CodeGenCXX/aix-constructor-attribute.cpp  | 20 ++---
 .../CodeGenCXX/aix-destructor-attribute.cpp   | 36 -
 clang/test/Sema/constructor-attribute.c   | 60 ---
 9 files changed, 147 insertions(+), 107 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 294b77a3e260908..a9ef2e2c378c7e3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return
+type, or
+  - the function it is applied to is a non-static member function (C++).
 
 Improvements to Clang's diagnostics
 ---
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..1c522500fedcf6d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3176,6 +3176,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' return type">;
+def err_ctor_dtor_member_func : Error<
+  "%0 attribute cannot be applied to a member function">;
 def err_attribute_sizeless_type : Error<
   "%0 attribute cannot be applied to sizeless type %1">;
 def err_attribute_argument_n_type : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..79a73ba4618c85d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argumen

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return

AaronBallman wrote:

Okay, after chatting with @erichkeane about this off-list, it seems we're going 
to have to support `int` as a return type as there is enough existing code in 
the wild using it and that returned value is safe on every ABI and calling 
convention we support. e.g.,

https://searchcode.com/file/664462906/src/sound/alure/CMakeLists.txt (see line 
70)
https://searchcode.com/file/624978895/deps/rocksdb/utilities/transactions/lock/range/range_tree/lib/portability/memory.h/
 (see line 63)
(and others)

As best we can find, it seems all of these uses are returning `int` 
specifically. So the current thinking is that we accept `void`, `int`, and 
`unsigned int` as valid return types, and error on any other type. I can update 
the documentation for the attributes to explain why.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return

erichkeane wrote:

Agreed, I've convinced myself that `int` is really the only currently used 
example (including the CMake tests, which are particularly troublesome to me), 
and that we should continue supporting it without diagnostic (again, because 
CMake).  Everything else is either problematic due to calling-convention issues 
or at least not used.

So I'm OK with this being a breaking-change error except for `int`.  We've also 
agreed that `unsigned` makes sense as well because 'why not'.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman updated 
https://github.com/llvm/llvm-project/pull/67360

>From 6e9bcfa7fc5eb3da4592909b9e199215064df700 Mon Sep 17 00:00:00 2001
From: Aaron Ballman 
Date: Mon, 25 Sep 2023 15:03:56 -0400
Subject: [PATCH 1/4] Diagnose problematic uses of constructor/destructor
 attribute

Functions with these attributes will be automatically called before
main() or after main() exits gracefully, which means the functions
should not accept arguments or have a returned value (nothing can
provide an argument to the call in these cases, and nothing can use
the returned value), nor should they be allowed on a non-static member
function in C++.

Additionally, these reuse the same priority logic as the init_priority
attribute which explicitly reserved priorty values <= 100 or > 65535.
So we now diagnose use of reserved priorities the same as we do for the
init_priority attribute.
---
 clang/docs/ReleaseNotes.rst   |  5 ++
 .../clang/Basic/DiagnosticSemaKinds.td|  5 ++
 clang/lib/Sema/SemaDeclAttr.cpp   | 74 +--
 clang/test/CodeGen/2008-03-03-CtorAttrType.c  |  4 +-
 .../PowerPC/aix-constructor-attribute.c   | 20 ++---
 .../PowerPC/aix-destructor-attribute.c| 30 +++-
 .../CodeGenCXX/aix-constructor-attribute.cpp  | 20 ++---
 .../CodeGenCXX/aix-destructor-attribute.cpp   | 36 -
 clang/test/Sema/constructor-attribute.c   | 60 ---
 9 files changed, 147 insertions(+), 107 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 294b77a3e260908..a9ef2e2c378c7e3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return
+type, or
+  - the function it is applied to is a non-static member function (C++).
 
 Improvements to Clang's diagnostics
 ---
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..1c522500fedcf6d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3176,6 +3176,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' return type">;
+def err_ctor_dtor_member_func : Error<
+  "%0 attribute cannot be applied to a member function">;
 def err_attribute_sizeless_type : Error<
   "%0 attribute cannot be applied to sizeless type %1">;
 def err_attribute_argument_n_type : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..79a73ba4618c85d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argumen

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman resolved 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

https://github.com/erichkeane approved this pull request.

Do we have a codegen test for the constexpr case?  I'm concerned that we don't 
'force' emission of these functions .

Either way, I think this set of diagnostic changes makes sense.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman updated 
https://github.com/llvm/llvm-project/pull/67360

>From 6e9bcfa7fc5eb3da4592909b9e199215064df700 Mon Sep 17 00:00:00 2001
From: Aaron Ballman 
Date: Mon, 25 Sep 2023 15:03:56 -0400
Subject: [PATCH 1/5] Diagnose problematic uses of constructor/destructor
 attribute

Functions with these attributes will be automatically called before
main() or after main() exits gracefully, which means the functions
should not accept arguments or have a returned value (nothing can
provide an argument to the call in these cases, and nothing can use
the returned value), nor should they be allowed on a non-static member
function in C++.

Additionally, these reuse the same priority logic as the init_priority
attribute which explicitly reserved priorty values <= 100 or > 65535.
So we now diagnose use of reserved priorities the same as we do for the
init_priority attribute.
---
 clang/docs/ReleaseNotes.rst   |  5 ++
 .../clang/Basic/DiagnosticSemaKinds.td|  5 ++
 clang/lib/Sema/SemaDeclAttr.cpp   | 74 +--
 clang/test/CodeGen/2008-03-03-CtorAttrType.c  |  4 +-
 .../PowerPC/aix-constructor-attribute.c   | 20 ++---
 .../PowerPC/aix-destructor-attribute.c| 30 +++-
 .../CodeGenCXX/aix-constructor-attribute.cpp  | 20 ++---
 .../CodeGenCXX/aix-destructor-attribute.cpp   | 36 -
 clang/test/Sema/constructor-attribute.c   | 60 ---
 9 files changed, 147 insertions(+), 107 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 294b77a3e260908..a9ef2e2c378c7e3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -171,6 +171,11 @@ Attribute Changes in Clang
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: `_)
+- The ``constructor`` and ``destructor`` attributes now diagnose when:
+  - the priority is not between 101 and 65535, inclusive,
+  - the function it is applied to accepts arguments or has a non-void return
+type, or
+  - the function it is applied to is a non-static member function (C++).
 
 Improvements to Clang's diagnostics
 ---
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..1c522500fedcf6d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3176,6 +3176,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' return type">;
+def err_ctor_dtor_member_func : Error<
+  "%0 attribute cannot be applied to a member function">;
 def err_attribute_sizeless_type : Error<
   "%0 attribute cannot be applied to sizeless type %1">;
 def err_attribute_argument_n_type : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..79a73ba4618c85d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,61 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range.
+  if ((Priority < 101 || Priority > 65535) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << 101 << 65535;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argumen

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Nick Desaulniers via cfe-commits]

https://github.com/nickdesaulniers edited 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Nick Desaulniers via cfe-commits]

https://github.com/nickdesaulniers approved this pull request.


https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Nick Desaulniers via cfe-commits]

@@ -3176,6 +3178,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' or 'int' return type">;

nickdesaulniers wrote:

IIRC, there's a similar restriction for `__attribute__((cleanup()))` functions. 
Is there an existing Diag we can use here from that?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Nick Desaulniers via cfe-commits]

https://github.com/nickdesaulniers edited 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -3176,6 +3178,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' or 'int' return type">;

AaronBallman wrote:

Not that I could find -- the function marked `cleanup` needs to accept one 
argument and it has to be a pointer type:
```
def err_attribute_cleanup_func_must_take_one_arg : Error<
  "'cleanup' function %0 must take 1 parameter">;
def err_attribute_cleanup_func_arg_incompatible_type : Error<
  "'cleanup' function %0 parameter has "
  "%diff{type $ which is incompatible with type $|incompatible type}1,2">;
```
I looked around for an existing diagnostic to augment but didn't spot any that 
worked well.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman closed 
https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Vitaly Buka via cfe-commits]

vitalybuka wrote:

https://lab.llvm.org/buildbot/#/builders/70/builds/44561/steps/7/logs/stdio

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[via cfe-commits]

dyung wrote:

Also caused some test failures (this bot uses GCC to build which is why it 
didn't hit the compilation failure): 
https://lab.llvm.org/buildbot/#/builders/247/builds/9484

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

I've reverted in 50abfc4298980fc219c7d70ca2567c61e856b614 due to bots going 
red, I think we may need to make priority-related issues a warning rather than 
an error.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

There are six failures I've found, three in tests and three in source:

Source:
* 
https://github.com/llvm/llvm-project/blob/ebfea261e6a28e0ba33f437476202a25212b2d34/compiler-rt/lib/fuzzer/afl/afl_driver.cpp#L112
* 
https://github.com/llvm/llvm-project/blob/ebfea261e6a28e0ba33f437476202a25212b2d34/compiler-rt/lib/builtins/cpu_model.c#L30
* 
https://github.com/llvm/llvm-project/blob/ebfea261e6a28e0ba33f437476202a25212b2d34/compiler-rt/lib/profile/GCDAProfiling.c#L560

Tests:
* 
https://github.com/llvm/llvm-project/blob/ebfea261e6a28e0ba33f437476202a25212b2d34/compiler-rt/test/ubsan/TestCases/Misc/Linux/sigaction.cpp#L14
* 
https://github.com/llvm/llvm-project/blob/ebfea261e6a28e0ba33f437476202a25212b2d34/compiler-rt/test/profile/Posix/gcov-destructor.c#L24
* 
https://github.com/llvm/llvm-project/blob/43d2ef2856fc3373068c020efa11a933477e11fa/libc/test/integration/startup/gpu/init_fini_array_test.cpp#L51

In each case, the failure is due to using a reserved priority. There are two 
(or more) ways to address this without changing priorities: 1) use line markers 
around the code using the problematic priorities, 2) downgrade the priority 
diagnostic from an error to a warning. GCC currently has `-Wprio-ctor-dtor` and 
treats the priority issue as a warning rather than an error. I don't like that 
approach because 1) warnings are trivial to ignore and so the reservation has 
no teeth, 2) the name of the warning group is startlingly bad. 3) 
`init_priority` has the same reservation as `constructor` and `destructor` and 
that's been an error in Clang since at least 3.0.

However, it's not clear to me what compiler-rt folks think (they're the ones 
predominately impacted). CC @MaskRay for more opinions on whether I should use 
line markers or downgrade the diagnostic. One potential option would be to have 
this as a warning which defaults to an error, but my preference is to try to 
retain it as a hard error if possible so that the reservation is meaningful.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Fangrui Song via cfe-commits]

MaskRay wrote:

I haven't looked closely, but the patch made constructor with priority 0~100 an 
error?
```
error: 'destructor' attribute requires integer constant between 101 and 65535 
inclusive
```

I think we should just use a warning to discourage user programs to use 0~100. 
Compiler runtime libraries and their tests can freely use 0~100. They know what 
they are doing and they are responsible for the interaction with other 
constructor users.

```
% gcc -c a.c
a.c:2:1: warning: destructor priorities from 0 to 100 are reserved for the 
implementation [-Wprio-ctor-dtor]
2 | int f(void) {
  | ^~~
```

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Erich Keane via cfe-commits]

erichkeane wrote:

I think we're going to be stuck with 'warning-as-default-error' here, and let 
compiler-rt and tests/etc opt-out.  I realize that makes the error not as 
effective, but it is really the one 'good' way for the 'implementation' in this 
case to handle itself.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Nick Desaulniers via cfe-commits]

@@ -3176,6 +3178,11 @@ def err_alignas_underaligned : Error<
   "requested alignment is less than minimum alignment of %1 for type %0">;
 def warn_aligned_attr_underaligned : Warning,
   InGroup;
+def err_ctor_dtor_attr_on_non_void_func : Error<
+  "%0 attribute can only be applied to a function which accepts no arguments "
+  "and has a 'void' or 'int' return type">;

nickdesaulniers wrote:

ah, perhaps what I'm thinking of is that we aught to emit a similar diagnostic 
for the return type of those functions.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Nick Desaulniers via cfe-commits]

nickdesaulniers wrote:

ah, yeah I was curious about what precisely `reserved for the implementation` 
meant. System libraries? That means they should be able to opt out via 
`-Wno-whatever`.

For anything thats */test/*, I assume they can be adjusted to just not use a 
reserved priority?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Alexander Richardson via cfe-commits]

arichardson wrote:

With regard to arguments: I know that the [FreeBSD runtime 
linker](https://github.com/freebsd/freebsd-src/blob/d06328c37bbcf3d3d3c7601372d29237996a6f6a/libexec/rtld-elf/aarch64/rtld_machdep.h#L59)
 (and according to https://stackoverflow.com/a/46331112 also glibc) passes 
argc, argv, envv to the constructor functions, so maybe that specific signature 
should be permitted?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> With regard to arguments: I know that the [FreeBSD runtime 
> linker](https://github.com/freebsd/freebsd-src/blob/d06328c37bbcf3d3d3c7601372d29237996a6f6a/libexec/rtld-elf/aarch64/rtld_machdep.h#L59)
>  (and according to https://stackoverflow.com/a/46331112 also glibc) passes 
> argc, argv, envv to the constructor functions, so maybe that specific 
> signature should be permitted?

Thank you for pointing this out!

That is an undocumented behavior of the attribute that I think only happens to 
work if the stars line up correctly. As I understand things, glibc does this, 
but musl does not and it seems MSVC CRT does something entirely different as 
well. Because we don't know which CRT will be loaded at runtime, it seems 
pretty dangerous to allow that behavior. We could maybe presume glibc on ELF 
targets, but it seems like a lot of security risk to support an undocumented 
use case. Do you know if this is documented somewhere and I'm just not seeing 
it? Any ideas on how we can add safety rails instead of allowing the signature 
and hoping for the best?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> I think we're going to be stuck with 'warning-as-default-error' here, and let 
> compiler-rt and tests/etc opt-out. I realize that makes the error not as 
> effective, but it is really the one 'good' way for the 'implementation' in 
> this case to handle itself.

Yeah, having slept on it, I think I agree that warning-defaults-to-error is 
probably the best we can do here. I think I will treat `init_priority` the same 
way so all three attributes have consistent priority handling.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Alexander Richardson via cfe-commits]

arichardson wrote:

> > With regard to arguments: I know that the [FreeBSD runtime 
> > linker](https://github.com/freebsd/freebsd-src/blob/d06328c37bbcf3d3d3c7601372d29237996a6f6a/libexec/rtld-elf/aarch64/rtld_machdep.h#L59)
> >  (and according to https://stackoverflow.com/a/46331112 also glibc) passes 
> > argc, argv, envv to the constructor functions, so maybe that specific 
> > signature should be permitted?
> 
> Thank you for pointing this out!
> 
> That is an undocumented behavior of the attribute that I think only happens 
> to work if the stars line up correctly. As I understand things, glibc does 
> this, but musl does not and it seems MSVC CRT does something entirely 
> different as well. Because we don't know which CRT will be loaded at runtime, 
> it seems pretty dangerous to allow that behavior. We could maybe presume 
> glibc on ELF targets, but it seems like a lot of security risk to support an 
> undocumented use case. Do you know if this is documented somewhere and I'm 
> just not seeing it? Any ideas on how we can add safety rails instead of 
> allowing the signature and hoping for the best?

I did not realize musl doesn't pass arguments - in that case the only safe 
option is to reject arguments.

Hopefully the number of constructors that expect getting arguments is extremely 
rare. But I do worry that there is code out there that assumes glibc behaviour, 
so it seems to me that err_ctor_dtor_attr_on_non_void_func needs to be a 
warning-as-error that can be downgraded? 

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Nick Desaulniers via cfe-commits]

nickdesaulniers wrote:

> We could maybe presume glibc on ELF targets

Isn't that part of the triple? `aarch64-linux-gnu` gnu -> glibc (as opposed to 
`aarch64-linux-musl` -> musl, or `aarch64-linux-android` -> bionic)

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> > We could maybe presume glibc on ELF targets
> 
> Isn't that part of the triple? `aarch64-linux-gnu` gnu -> glibc (as opposed 
> to `aarch64-linux-musl` -> musl, or `aarch64-linux-android` -> bionic)

Oh -- I wasn't aware we tracked that in the triple, that's awesome! I'll give 
that a shot.

> Hopefully the number of constructors that expect getting arguments is 
> extremely rare. But I do worry that there is code out there that assumes 
> glibc behaviour, so it seems to me that err_ctor_dtor_attr_on_non_void_func 
> needs to be a warning-as-error that can be downgraded?

Hopefully we can avoid emitting the diagnostic at all for glibc, then we can 
leave it as a hard error for the other cases. My concern about turning that 
into a warning which defaults to an error is that a less thorough programmer 
may shut the warning up rather than realize their CRT doesn't support that.

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Richard Smith via cfe-commits]

@@ -2352,26 +2352,78 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  constexpr uint32_t ReservedPriorityLower = 101, ReservedPriorityUpper = 
65535;
+
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range. Values > 65535
+  // are reserved for historical reasons.
+  if ((Priority < ReservedPriorityLower || Priority > ReservedPriorityUpper) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << ReservedPriorityLower << ReservedPriorityUpper;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments.
+  // In theory, a void return type is the only truly safe return type (consider
+  // that calling conventions may place returned values in a hidden pointer
+  // argument passed to the function that will not be present when called
+  // automatically). However, there is a significant amount of existing code
+  // which uses an int return type. So we will accept void, int, and
+  // unsigned int return types. Any other return type, or a non-void parameter
+  // list is treated as an error because it's a form of type system
+  // incompatibility. The function also cannot be a member function. We allow
+  // K&R C functions because that's a difficult edge case where it depends on
+  // how the function is defined as to whether it does or does not expect
+  // arguments.
+  const auto *FD = cast(D);
+  QualType RetTy = FD->getReturnType();
+  if (!(RetTy->isVoidType() ||

zygoloid wrote:

Would it also make sense to check that the function uses the C calling 
convention and isn't varargs?

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Diagnose problematic uses of constructor/destructor attribute (PR #67360)

[Aaron Ballman via cfe-commits]

@@ -2352,26 +2352,78 @@ static void handleUnusedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
   D->addAttr(::new (S.Context) UnusedAttr(S.Context, AL));
 }
 
-static void handleConstructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  uint32_t priority = ConstructorAttr::DefaultPriority;
+static bool diagnoseInvalidPriority(Sema &S, uint32_t Priority,
+const ParsedAttr &A,
+SourceLocation PriorityLoc) {
+  constexpr uint32_t ReservedPriorityLower = 101, ReservedPriorityUpper = 
65535;
+
+  // Only perform the priority check if the attribute is outside of a system
+  // header. Values <= 100 are reserved for the implementation, and libc++
+  // benefits from being able to specify values in that range. Values > 65535
+  // are reserved for historical reasons.
+  if ((Priority < ReservedPriorityLower || Priority > ReservedPriorityUpper) &&
+  !S.getSourceManager().isInSystemHeader(A.getLoc())) {
+S.Diag(A.getLoc(), diag::err_attribute_argument_out_of_range)
+<< PriorityLoc << A << ReservedPriorityLower << ReservedPriorityUpper;
+A.setInvalid();
+return true;
+  }
+  return false;
+}
+
+template 
+static void handleCtorDtorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  uint32_t Priority = CtorDtorAttr::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));
-}
+  // If we're given an argument for the priority, check that it's valid.
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Priority))
+  return;
 
-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))
+// Ensure the priority is in a reasonable range.
+if (diagnoseInvalidPriority(S, Priority, AL,
+AL.getArgAsExpr(0)->getExprLoc()))
+  return;
+  }
+
+  // Ensure the function we're attaching to is something that is sensible to
+  // automatically call before or after main(); it should accept no arguments.
+  // In theory, a void return type is the only truly safe return type (consider
+  // that calling conventions may place returned values in a hidden pointer
+  // argument passed to the function that will not be present when called
+  // automatically). However, there is a significant amount of existing code
+  // which uses an int return type. So we will accept void, int, and
+  // unsigned int return types. Any other return type, or a non-void parameter
+  // list is treated as an error because it's a form of type system
+  // incompatibility. The function also cannot be a member function. We allow
+  // K&R C functions because that's a difficult edge case where it depends on
+  // how the function is defined as to whether it does or does not expect
+  // arguments.
+  const auto *FD = cast(D);
+  QualType RetTy = FD->getReturnType();
+  if (!(RetTy->isVoidType() ||

AaronBallman wrote:

Yeah, I think those restrictions make sense, good call!

https://github.com/llvm/llvm-project/pull/67360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits