Diagnose problematic uses of constructor/destructor attribute #67360
Conversation
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.
AaronBallman
requested review from
jyknight,
nickdesaulniers,
erichkeane and
efriedma-quic
llvmbot
added
clang
|
@llvm/pr-subscribers-clang ChangesFunctions 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:
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: <https://github.com/llvm/llvm-project/issues/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<err_alignas_underaligned.Summary>,
InGroup<IgnoredAttributes>;
+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 <typename CtorDtorAttr>
+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<FunctionDecl>(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<CXXMethodDecl>(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));
}
template <typename AttrTy>
@@ -3888,16 +3923,9 @@ static void handleInitPriorityAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
return;
}
- // 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 ((prioritynum < 101 || prioritynum > 65535) &&
- !S.getSourceManager().isInSystemHeader(AL.getLoc())) {
- S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_range)
- << E->getSourceRange() << AL << 101 << 65535;
- AL.setInvalid();
+ if (diagnoseInvalidPriority(S, prioritynum, AL, E->getExprLoc()))
return;
- }
+
D->addAttr(::new (S.Context) InitPriorityAttr(S.Context, AL, prioritynum));
}
@@ -8930,13 +8958,13 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
handlePassObjectSizeAttr(S, D, AL);
break;
case ParsedAttr::AT_Constructor:
- handleConstructorAttr(S, D, AL);
+ handleCtorDtorAttr<ConstructorAttr>(S, D, AL);
break;
case ParsedAttr::AT_Deprecated:
handleDeprecatedAttr(S, D, AL);
break;
case ParsedAttr::AT_Destructor:
- handleDestructorAttr(S, D, AL);
+ handleCtorDtorAttr<DestructorAttr>(S, D, AL);
break;
case ParsedAttr::AT_EnableIf:
handleEnableIfAttr(S, D, AL);
diff --git a/clang/test/CodeGen/2008-03-03-CtorAttrType.c b/clang/test/CodeGen/2008-03-03-CtorAttrType.c
index dbd7bc0a7270953..efc219bf85e6f4e 100644
--- a/clang/test/CodeGen/2008-03-03-CtorAttrType.c
+++ b/clang/test/CodeGen/2008-03-03-CtorAttrType.c
@@ -1,6 +1,4 @@
// RUN: %clang_cc1 %s -emit-llvm -o - | grep llvm.global_ctors
-int __attribute__((constructor)) foo(void) {
- return 0;
-}
+void __attribute__((constructor)) foo(void) {}
void __attribute__((constructor)) bar(void) {}
diff --git a/clang/test/CodeGen/PowerPC/aix-constructor-attribute.c b/clang/test/CodeGen/PowerPC/aix-constructor-attribute.c
index 7cd72a3276936ff..031deeb3ab257a8 100644
--- a/clang/test/CodeGen/PowerPC/aix-constructor-attribute.c
+++ b/clang/test/CodeGen/PowerPC/aix-constructor-attribute.c
@@ -7,18 +7,10 @@
// CHECK: @llvm.global_ctors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @foo3, ptr null }, { i32, ptr, ptr } { i32 180, ptr @foo2, ptr null }, { i32, ptr, ptr } { i32 180, ptr @foo, ptr null }]
-int foo(void) __attribute__((constructor(180)));
-int foo2(void) __attribute__((constructor(180)));
-int foo3(void) __attribute__((constructor(65535)));
+void foo(void) __attribute__((constructor(180)));
+void foo2(void) __attribute__((constructor(180)));
+void foo3(void) __attribute__((constructor(65535)));
-int foo3(void) {
- return 3;
-}
-
-int foo2(void) {
- return 2;
-}
-
-int foo(void) {
- return 1;
-}
+void foo3(void) {}
+void foo2(void) {}
+void foo(void) {}
diff --git a/clang/test/CodeGen/PowerPC/aix-destructor-attribute.c b/clang/test/CodeGen/PowerPC/aix-destructor-attribute.c
index cfb1fd7b0171a41..6c49b0f638e760d 100644
--- a/clang/test/CodeGen/PowerPC/aix-destructor-attribute.c
+++ b/clang/test/CodeGen/PowerPC/aix-destructor-attribute.c
@@ -12,28 +12,20 @@
// RUN: -fno-use-cxa-atexit -fregister-global-dtors-with-atexit < %s | \
// RUN: FileCheck --check-prefix=REGISTER %s
-int bar(void) __attribute__((destructor(100)));
-int bar2(void) __attribute__((destructor(65535)));
-int bar3(int) __attribute__((destructor(65535)));
+void bar(void) __attribute__((destructor(101)));
+void bar2(void) __attribute__((destructor(65535)));
+void bar3(void) __attribute__((destructor(65535)));
-int bar(void) {
- return 1;
-}
+void bar(void) {}
+void bar2(void) {}
+void bar3(void) {}
-int bar2(void) {
- return 2;
-}
+// NO-REGISTER: @llvm.global_dtors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 101, ptr @bar, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @bar2, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @bar3, ptr null }]
-int bar3(int a) {
- return a;
-}
+// REGISTER: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 101, ptr @__GLOBAL_init_101, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_init_65535, ptr null }]
+// REGISTER: @llvm.global_dtors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 101, ptr @__GLOBAL_cleanup_101, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_cleanup_65535, ptr null }]
-// NO-REGISTER: @llvm.global_dtors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 100, ptr @bar, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @bar2, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @bar3, ptr null }]
-
-// REGISTER: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 100, ptr @__GLOBAL_init_100, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_init_65535, ptr null }]
-// REGISTER: @llvm.global_dtors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 100, ptr @__GLOBAL_cleanup_100, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_cleanup_65535, ptr null }]
-
-// REGISTER: define internal void @__GLOBAL_init_100() [[ATTR:#[0-9]+]] {
+// REGISTER: define internal void @__GLOBAL_init_101() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
// REGISTER: %0 = call i32 @atexit(ptr @bar)
// REGISTER: ret void
@@ -46,7 +38,7 @@ int bar3(int a) {
// REGISTER: ret void
// REGISTER: }
-// REGISTER: define internal void @__GLOBAL_cleanup_100() [[ATTR:#[0-9]+]] {
+// REGISTER: define internal void @__GLOBAL_cleanup_101() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
// REGISTER: %0 = call i32 @unatexit(ptr @bar)
// REGISTER: %needs_destruct = icmp eq i32 %0, 0
diff --git a/clang/test/CodeGenCXX/aix-constructor-attribute.cpp b/clang/test/CodeGenCXX/aix-constructor-attribute.cpp
index 9f1191278fcd34c..84c2b6722628344 100644
--- a/clang/test/CodeGenCXX/aix-constructor-attribute.cpp
+++ b/clang/test/CodeGenCXX/aix-constructor-attribute.cpp
@@ -8,9 +8,9 @@
// CHECK: @llvm.global_ctors = appending global [4 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_Z4foo3v, ptr null }, { i32, ptr, ptr } { i32 180, ptr @_Z4foo2v, ptr null }, { i32, ptr, ptr } { i32 180, ptr @_Z3foov, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I__, ptr null }]
// CHECK: @llvm.global_dtors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__D_a, ptr null }]
-int foo() __attribute__((constructor(180)));
-int foo2() __attribute__((constructor(180)));
-int foo3() __attribute__((constructor(65535)));
+void foo() __attribute__((constructor(180)));
+void foo2() __attribute__((constructor(180)));
+void foo3() __attribute__((constructor(65535)));
struct Test {
public:
@@ -18,14 +18,6 @@ struct Test {
~Test() {}
} t;
-int foo3() {
- return 3;
-}
-
-int foo2() {
- return 2;
-}
-
-int foo() {
- return 1;
-}
+void foo3() {}
+void foo2() {}
+void foo() {}
diff --git a/clang/test/CodeGenCXX/aix-destructor-attribute.cpp b/clang/test/CodeGenCXX/aix-destructor-attribute.cpp
index 5ebea7a997c9db1..2782c7ee74939e5 100644
--- a/clang/test/CodeGenCXX/aix-destructor-attribute.cpp
+++ b/clang/test/CodeGenCXX/aix-destructor-attribute.cpp
@@ -17,29 +17,21 @@ struct test {
~test();
} t;
-int bar() __attribute__((destructor(100)));
-int bar2() __attribute__((destructor(65535)));
-int bar3(int) __attribute__((destructor(65535)));
+void bar() __attribute__((destructor(101)));
+void bar2() __attribute__((destructor(65535)));
+void bar3() __attribute__((destructor(65535)));
-int bar() {
- return 1;
-}
-
-int bar2() {
- return 2;
-}
-
-int bar3(int a) {
- return a;
-}
+void bar() {}
+void bar2() {}
+void bar3() {}
// NO-REGISTER: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I__, ptr null }]
-// NO-REGISTER: @llvm.global_dtors = appending global [4 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 100, ptr @_Z3barv, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_Z4bar2v, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_Z4bar3i, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__D_a, ptr null }]
+// NO-REGISTER: @llvm.global_dtors = appending global [4 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 101, ptr @_Z3barv, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_Z4bar2v, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_Z4bar3v, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__D_a, ptr null }]
-// REGISTER: @llvm.global_ctors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I__, ptr null }, { i32, ptr, ptr } { i32 100, ptr @__GLOBAL_init_100, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_init_65535, ptr null }]
-// REGISTER: @llvm.global_dtors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__D_a, ptr null }, { i32, ptr, ptr } { i32 100, ptr @__GLOBAL_cleanup_100, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_cleanup_65535, ptr null }]
+// REGISTER: @llvm.global_ctors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I__, ptr null }, { i32, ptr, ptr } { i32 101, ptr @__GLOBAL_init_101, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_init_65535, ptr null }]
+// REGISTER: @llvm.global_dtors = appending global [3 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__D_a, ptr null }, { i32, ptr, ptr } { i32 101, ptr @__GLOBAL_cleanup_101, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__GLOBAL_cleanup_65535, ptr null }]
-// REGISTER: define internal void @__GLOBAL_init_100() [[ATTR:#[0-9]+]] {
+// REGISTER: define internal void @__GLOBAL_init_101() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
// REGISTER: %0 = call i32 @atexit(ptr @_Z3barv)
// REGISTER: ret void
@@ -48,11 +40,11 @@ int bar3(int a) {
// REGISTER: define internal void @__GLOBAL_init_65535() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
// REGISTER: %0 = call i32 @atexit(ptr @_Z4bar2v)
-// REGISTER: %1 = call i32 @atexit(ptr @_Z4bar3i)
+// REGISTER: %1 = call i32 @atexit(ptr @_Z4bar3v)
// REGISTER: ret void
// REGISTER: }
-// REGISTER: define internal void @__GLOBAL_cleanup_100() [[ATTR:#[0-9]+]] {
+// REGISTER: define internal void @__GLOBAL_cleanup_101() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
// REGISTER: %0 = call i32 @unatexit(ptr @_Z3barv)
// REGISTER: %needs_destruct = icmp eq i32 %0, 0
@@ -68,12 +60,12 @@ int bar3(int a) {
// REGISTER: define internal void @__GLOBAL_cleanup_65535() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
-// REGISTER: %0 = call i32 @unatexit(ptr @_Z4bar3i)
+// REGISTER: %0 = call i32 @unatexit(ptr @_Z4bar3v)
// REGISTER: %needs_destruct = icmp eq i32 %0, 0
// REGISTER: br i1 %needs_destruct, label %destruct.call, label %unatexit.call
// REGISTER: destruct.call:
-// REGISTER: call void @_Z4bar3i()
+// REGISTER: call void @_Z4bar3v()
// REGISTER: br label %unatexit.call
// REGISTER: unatexit.call:
diff --git a/clang/test/Sema/constructor-attribute.c b/clang/test/Sema/constructor-attribute.c
index 2317c7735bda586..8df5ac42378bdd8 100644
--- a/clang/test/Sema/constructor-attribute.c
+++ b/clang/test/Sema/constructor-attribute.c
@@ -1,16 +1,52 @@
// RUN: %clang_cc1 -fsyntax-only -verify -Wno-strict-prototypes %s
+// RUN: %clang_cc1 -fsyntax-only -verify -x c++ %s
-int x __attribute__((constructor)); // expected-warning {{'constructor' attribute only applies to functions}}
-int f(void) __attribute__((constructor));
-int f(void) __attribute__((constructor(1)));
-int f(void) __attribute__((constructor(1,2))); // expected-error {{'constructor' attribute takes no more than 1 argument}}
-int f(void) __attribute__((constructor(1.0))); // expected-error {{'constructor' attribute requires an integer constant}}
-int f(void) __attribute__((constructor(0x100000000))); // expected-error {{integer constant expression evaluates to value 4294967296 that cannot be represented in a 32-bit unsigned integer type}}
+int x1 __attribute__((constructor)); // expected-warning {{'constructor' attribute only applies to functions}}
+void f(void) __attribute__((constructor));
+void f(void) __attribute__((constructor(1))); // expected-error {{'constructor' attribute requires integer constant between 101 and 65535 inclusive}}
+void f(void) __attribute__((constructor(1,2))); // expected-error {{'constructor' attribute takes no more than 1 argument}}
+void f(void) __attribute__((constructor(1.0))); // expected-error {{'constructor' attribute requires an integer constant}}
+void f(void) __attribute__((constructor(0x100000000))); // expected-error {{integer constant expression evaluates to value 4294967296 that cannot be represented in a 32-bit unsigned integer type}}
+void f(void) __attribute__((constructor(101)));
-int x __attribute__((destructor)); // expected-warning {{'destructor' attribute only applies to functions}}
-int f(void) __attribute__((destructor));
-int f(void) __attribute__((destructor(1)));
-int f(void) __attribute__((destructor(1,2))); // expected-error {{'destructor' attribute takes no more than 1 argument}}
-int f(void) __attribute__((destructor(1.0))); // expected-error {{'destructor' attribute requires an integer constant}}
+int x2 __attribute__((destructor)); // expected-warning {{'destructor' attribute only applies to functions}}
+void f(void) __attribute__((destructor));
+void f(void) __attribute__((destructor(1))); // expected-error {{'destructor' attribute requires integer constant between 101 and 65535 inclusive}}
+void f(void) __attribute__((destructor(1,2))); // expected-error {{'destructor' attribute takes no more than 1 argument}}
+void f(void) __attribute__((destructor(1.0))); // expected-error {{'destructor' attribute requires an integer constant}}
+void f(void) __attribute__((destructor(101)));
-void knr() __attribute__((constructor));
+void knr1() __attribute__((constructor));
+void knr2() __attribute__((destructor));
+
+// Require a void return type
+int g(void) __attribute__((constructor)); // expected-error {{'constructor' attribute can only be applied to a function which accepts no arguments and has a 'void' return type}}
+int h(void) __attribute__((destructor)); // expected-error {{'destructor' attribute can only be applied to a function which accepts no arguments and has a 'void' return type}}
+
+// Require no parameters
+void i(int v) __attribute__((constructor)); // expected-error {{'constructor' attribute can only be applied to a function which accepts no arguments and has a 'void' return type}}
+void j(int v) __attribute__((destructor)); // expected-error {{'destructor' attribute can only be applied to a function which accepts no arguments and has a 'void' return type}}
+
+#ifdef __cplusplus
+struct S {
+ // Not allowed on a nonstatic member function, but is allowed on a static
+ // member function so long as it has no args/void return type.
+ void mem1() __attribute__((constructor)); // expected-error {{'constructor' attribute cannot be applied to a member function}}
+ void mem2() __attribute__((destructor)); // expected-error {{'destructor' attribute cannot be applied to a membe...
[truncated]
|
Sure, but perhaps not for |
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. |
* Updated some comments * Added some named constants * Diagnose consteval functions * Better const correctness
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 |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Now accepts 'int' and 'unsigned int' as return types, in addition to 'void'. Updated the diagnostic, test cases, and documentation accordingly.
This reverts some of the tests back to their original form now that we accept 'int' as a valid return type. The AIX destructor tests still needed to be modified due to 100 being a reserved priority value.
| 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">; |
There was a problem hiding this comment.
IIRC, there's a similar restriction for __attribute__((cleanup())) functions. Is there an existing Diag we can reuse here from that?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ah, perhaps what I'm thinking of is that we aught to emit a similar diagnostic for the return type of those functions.
|
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 |
|
I've reverted in 50abfc4 due to bots going red, I think we may need to make priority-related issues a warning rather than an error. |
|
There are six failures I've found, three in tests and three in source: Source: Tests: 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 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. |
|
I haven't looked closely, but the patch made constructor with priority 0~100 an error? I think we should just use a warning to discourage user programs to use For |
|
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. |
|
ah, yeah I was curious about what precisely For anything thats /test/, I assume they can be adjusted to just not use a reserved priority? |
|
With regard to arguments: I know that the FreeBSD runtime linker (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? |
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 |
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? |
Isn't that part of the triple? |
Oh -- I wasn't aware we tracked that in the triple, that's awesome! I'll give that a shot.
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. |
| // arguments. | ||
| const auto *FD = cast<FunctionDecl>(D); | ||
| QualType RetTy = FD->getReturnType(); | ||
| if (!(RetTy->isVoidType() || |
There was a problem hiding this comment.
Would it also make sense to check that the function uses the C calling convention and isn't varargs?
There was a problem hiding this comment.
Yeah, I think those restrictions make sense, good call!
…7360) 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 or consteval function in C++. We allow 'int' as a return type for the function due to finding a significant amount of historical code using `int(void)` as a signature. 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.
…llvm#67360)" This reverts commit b443510. This caused too many disruptions in compiler-rt where reserved priorities were being used. Reverting to investigate appropriate solutions.
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.