Diagnose problematic uses of constructor/destructor attribute #67673
Conversation
Functions with these attributes will be automatically called before main() or after main() exits gracefully. In glibc environments, the constructor function is passed the same arguments as main(), so that signature is allowed. In all other environments, we require the function to accept no arguments and either return `void` or `int`. The functions must use the C calling convention. In C++ language modes, the functions cannot be a nonstatic member function, or a consteval function. 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, but we downgrade the error to be a warning which defaults to an error to ease use for implementers like compiler-rt or libc.
AaronBallman
added
clang:frontend
AaronBallman
requested review from
zygoloid,
MaskRay,
cor3ntin,
nickdesaulniers,
shafik,
erichkeane and
tbaederr
llvmbot
added
clang
|
This is another pass at #67360 Note: llvm-libc will still have a test failure until I figure out the correct way to repair init_fini_array_test.cpp. I think I made the correct changes in compiler-rt's CMake so that all of the compiler-rt projects can use reserved priorities, but I'd appreciate confirmation from @MaskRay on those changes. |
|
@llvm/pr-subscribers-libc @llvm/pr-subscribers-clang ChangesFunctions with these attributes will be automatically called before main() or after main() exits gracefully. In glibc environments, the constructor function is passed the same arguments as main(), so that signature is allowed. In all other environments, we require the function to accept no arguments and either return 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, but we downgrade the error to be a warning which defaults to an error to ease use for implementers like compiler-rt or libc. Patch is 30.71 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67673.diff 11 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f314c9c72fa28b7..b6bfc4cb4093406 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/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 2f9d4d1b7907b20..6d82b8f6aa55d43 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -7251,8 +7251,14 @@ after returning from ``main()`` or when the ``exit()`` function has been
called. Note, ``quick_exit()``, ``_Exit()``, and ``abort()`` prevent a function
marked ``destructor`` from being called.
-The constructor or destructor function should not accept any arguments and its
-return type should be ``void``.
+In general, the constructor or destructor function must use the C calling
+convention, cannot accept any arguments, and its return type should be
+``void``, ``int``, or ``unsigned int``. The latter two types are supported for
+historical reasons. On targets with a GNU environment (one which uses glibc),
+the signature of the function can also be the same as that of ``main()``.
+
+In C++ language modes, the function cannot be marked ``consteval``, nor can it
+be a non-static member function.
The attributes accept an optional argument used to specify the priority order
in which to execute constructor and destructor functions. The priority is
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..e017ca45aeeeb67 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -105,6 +105,10 @@ def EnumConversion : DiagGroup<"enum-conversion",
[EnumEnumConversion,
EnumFloatConversion,
EnumCompareConditional]>;
+def InvalidPriority : DiagGroup<"priority-ctor-dtor">;
+// For compatibility with GCC.
+def : DiagGroup<"prio-ctor-dtor", [InvalidPriority]>;
+
def ObjCSignedCharBoolImplicitIntConversion :
DiagGroup<"objc-signed-char-bool-implicit-int-conversion">;
def ImplicitIntConversion : DiagGroup<"implicit-int-conversion",
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..81ebd3948706bd2 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2864,6 +2864,8 @@ def warn_cxx11_compat_constexpr_body_multiple_return : Warning<
InGroup<CXXPre14Compat>, DefaultIgnore;
def note_constexpr_body_previous_return : Note<
"previous return statement is here">;
+def err_ctordtor_attr_consteval : Error<
+ "%0 attribute cannot be applied to a 'consteval' function">;
// C++20 function try blocks in constexpr
def ext_constexpr_function_try_block_cxx20 : ExtWarn<
@@ -3176,6 +3178,13 @@ 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' or 'int' return type">;
+def err_ctor_dtor_member_func : Error<
+ "%0 attribute cannot be applied to a member function">;
+def err_ctor_dtor_calling_conv : Error<
+ "%0 attribute must be applied to a function with the C calling convention">;
def err_attribute_sizeless_type : Error<
"%0 attribute cannot be applied to sizeless type %1">;
def err_attribute_argument_n_type : Error<
@@ -3189,6 +3198,9 @@ def err_attribute_argument_out_of_range : Error<
def err_init_priority_object_attr : Error<
"can only use 'init_priority' attribute on file-scope definitions "
"of objects of class type">;
+def warn_priority_out_of_range : Warning<
+ err_attribute_argument_out_of_range.Summary>,
+ InGroup<InvalidPriority>, DefaultError;
def err_attribute_argument_out_of_bounds : Error<
"%0 attribute parameter %1 is out of bounds">;
def err_attribute_only_once_per_parameter : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 090a54eedaa07d0..133f8f7516cf83f 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2352,26 +2352,127 @@ 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 void 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::warn_priority_out_of_range)
+ << PriorityLoc << A << ReservedPriorityLower << ReservedPriorityUpper;
+ }
+}
+
+static bool FunctionParamsAreMainLike(ASTContext &Context,
+ const FunctionDecl *FD) {
+ assert(FD->hasPrototype() && "expected the function to have a prototype");
+ const auto *FPT = FD->getType()->castAs<FunctionProtoType>();
+ QualType CharPP =
+ Context.getPointerType(Context.getPointerType(Context.CharTy));
+ QualType Expected[] = {Context.IntTy, CharPP, CharPP, CharPP};
+
+ for (unsigned I = 0; I < FPT->getNumParams(); ++I) {
+ QualType AT = FPT->getParamType(I);
+
+ bool Mismatch = true;
+ if (Context.hasSameUnqualifiedType(AT, Expected[I]))
+ Mismatch = false;
+ else if (Expected[I] == CharPP) {
+ // As an extension, the following forms are okay:
+ // char const **
+ // char const * const *
+ // char * const *
+
+ QualifierCollector Qs;
+ const PointerType *PT;
+ if ((PT = Qs.strip(AT)->getAs<PointerType>()) &&
+ (PT = Qs.strip(PT->getPointeeType())->getAs<PointerType>()) &&
+ Context.hasSameType(QualType(Qs.strip(PT->getPointeeType()), 0),
+ Context.CharTy)) {
+ Qs.removeConst();
+ Mismatch = !Qs.empty();
+ }
+ }
+
+ if (Mismatch)
+ return false;
+ }
+ return true;
+}
+
+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;
+
+ // Diagnose an invalid priority, but continue to process the attribute.
+ diagnoseInvalidPriority(S, Priority, AL, AL.getArgAsExpr(0)->getExprLoc());
+ }
-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 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.
+ //
+ // However! glibc on ELF will pass the same arguments to a constructor
+ // function as are given to main(), so we will allow `int, char *[]` and
+ // `int, char *[], char *[]` (or qualified versions thereof), but only if
+ // the target is explicitly for glibc.
+ const auto *FD = cast<FunctionDecl>(D);
+ QualType RetTy = FD->getReturnType();
+ bool IsGlibC = S.Context.getTargetInfo().getTriple().isGNUEnvironment();
+ if (!(RetTy->isVoidType() ||
+ RetTy->isSpecificBuiltinType(BuiltinType::UInt) ||
+ RetTy->isSpecificBuiltinType(BuiltinType::Int)) ||
+ FD->isVariadic() ||
+ (FD->hasPrototype() &&
+ ((!IsGlibC && FD->getNumParams() != 0) ||
+ (IsGlibC && !FunctionParamsAreMainLike(S.Context, FD))))) {
+ S.Diag(AL.getLoc(), diag::err_ctor_dtor_attr_on_non_void_func)
+ << AL << FD->getSourceRange();
+ return;
+ }
+ if (FD->getType()->castAs<FunctionType>()->getCallConv() !=
+ CallingConv::CC_C) {
+ S.Diag(AL.getLoc(), diag::err_ctor_dtor_calling_conv)
+ << AL << FD->getSourceRange();
+ return;
+ }
+ if (const auto *MD = dyn_cast<CXXMethodDecl>(FD);
+ MD && MD->isInstance()) {
+ S.Diag(AL.getLoc(), diag::err_ctor_dtor_member_func)
+ << AL << FD->getSourceRange();
+ return;
+ }
+ if (FD->isConsteval()) {
+ S.Diag(AL.getLoc(), diag::err_ctordtor_attr_consteval)
+ << AL << FD->getSourceRange();
return;
+ }
- D->addAttr(::new (S.Context) DestructorAttr(S.Context, AL, priority));
+ D->addAttr(CtorDtorAttr::Create(S.Context, Priority, AL));
}
template <typename AttrTy>
@@ -3888,16 +3989,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();
- return;
- }
+ // Diagnose an invalid priority, but continue to process the attribute.
+ diagnoseInvalidPriority(S, prioritynum, AL, E->getExprLoc());
+
D->addAttr(::new (S.Context) InitPriorityAttr(S.Context, AL, prioritynum));
}
@@ -8930,13 +9024,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/PowerPC/aix-destructor-attribute.c b/clang/test/CodeGen/PowerPC/aix-destructor-attribute.c
index cfb1fd7b0171a41..f0d83d161be50bd 100644
--- a/clang/test/CodeGen/PowerPC/aix-destructor-attribute.c
+++ b/clang/test/CodeGen/PowerPC/aix-destructor-attribute.c
@@ -12,9 +12,8 @@
// RUN: -fno-use-cxa-atexit -fregister-global-dtors-with-atexit < %s | \
// RUN: FileCheck --check-prefix=REGISTER %s
-int bar(void) __attribute__((destructor(100)));
+int bar(void) __attribute__((destructor(101)));
int bar2(void) __attribute__((destructor(65535)));
-int bar3(int) __attribute__((destructor(65535)));
int bar(void) {
return 1;
@@ -24,16 +23,12 @@ int bar2(void) {
return 2;
}
-int bar3(int a) {
- return a;
-}
-
-// 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 }]
+// NO-REGISTER: @llvm.global_dtors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 101, ptr @bar, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @bar2, 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: @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 }]
-// 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
@@ -42,11 +37,10 @@ int bar3(int a) {
// REGISTER: define internal void @__GLOBAL_init_65535() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
// REGISTER: %0 = call i32 @atexit(ptr @bar2)
-// REGISTER: %1 = call i32 @atexit(ptr @bar3)
// 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
@@ -62,20 +56,11 @@ int bar3(int a) {
// REGISTER: define internal void @__GLOBAL_cleanup_65535() [[ATTR:#[0-9]+]] {
// REGISTER: entry:
-// REGISTER: %0 = call i32 @unatexit(ptr @bar3)
+// REGISTER: %0 = call i32 @unatexit(ptr @bar2)
// REGISTER: %needs_destruct = icmp eq i32 %0, 0
-// REGISTER: br i1 %needs_destruct, label %destruct.call, label %unatexit.call
+// REGISTER: br i1 %needs_destruct, label %destruct.call, label %destruct.end
// REGISTER: destruct.call:
-// REGISTER: call void @bar3()
-// REGISTER: br label %unatexit.call
-
-// REGISTER: unatexit.call:
-// REGISTER: %1 = call i32 @unatexit(ptr @bar2)
-// REGISTER: %needs_destruct1 = icmp eq i32 %1, 0
-// REGISTER: br i1 %needs_destruct1, label %destruct.call2, label %destruct.end
-
-// REGISTER: destruct.call2:
// REGISTER: call void @bar2()
// REGISTER: br label %destruct.end
diff --git a/clang/test/CodeGenCXX/aix-destructor-attribute.cpp b/clang/test/CodeGenCXX/aix-destructor-attribute.cpp
index 5ebea7a997c9db1..2cdf147af8d07f1 100644
--- a/clang/test/CodeGenCXX/aix-destructor-attribute.cpp
+++ b/clang/test/CodeGenCXX/aix-destructor-attribute.cpp
@@ -17,9 +17,8 @@ struct test {
~test();
} t;
-int bar() __attribute__((destructor(100)));
+int bar() __attribute__((destructor(101)));
int bar2() __attribute__((destructor(65535)));
-int bar3(int) __attribute__((destructor(65535)));
int bar() {
return 1;
@@ -29,17 +28,13 @@ int bar2() {
return 2;
}
-int bar3(int a) {
- return a;
-}
-
// 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 [3 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 @_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 +43,10 @@ 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: 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,20 +62,11 @@ 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 @_Z4bar2v)
// REGISTER: %needs_destruct = icmp eq i32 %0, 0
-// REGISTER: br i1 %needs_destruct, label %destruct.call, label %unatexit.call
+// REGISTER: br i1 %needs_destruct, label %destruct.call, label %destruct.end
// REGISTER: destruct.call:
-// REGISTER: call void @_Z4bar3i()
-// REGISTER: br label %unatexit.call
-
-// REGISTER: unatexit.call:
-// REGISTER: %1 = call i32 @unatexit(ptr @_Z4bar2v)
-// REGISTER: %needs_destruct1 = icmp eq i32 %1, 0
-// REGISTER: br i1 %needs_destruct1, label %destruct.call2, label %destruct.end
-
-// REGISTER: destruct.call2:
// REGISTER: call void @_Z4bar2v()
// REGISTER: br label %destruct.end
diff --git a/clang/test/Sema/constructor-attribu...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Hmm, nope, it seems the test cases still fail (but the source files work). |
After talking with llvm-libc maintainers, they suggested changing the priority to 101 rather than disabling the diagnostic.
|
Ping, especially @MaskRay |
|
I already changed the |
…#67673)" This reverts commit a633a37. Still breaking compiler-rt bots: https://lab.llvm.org/buildbot/#/builders/109/builds/75360
Functions with these attributes will be automatically called before main() or after main() exits gracefully. In glibc environments, the constructor function is passed the same arguments as main(), so that signature is allowed. In all other environments, we require the function to accept no arguments and either return `void` or `int`. The functions must use the C calling convention. In C++ language modes, the functions cannot be a nonstatic member function, or a consteval function. 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, but we downgrade the error to be a warning which defaults to an error to ease use for implementers like compiler-rt or libc. This relands a633a37 with fixes.
…#67673)" This reverts commit 27ecb63. Still fails compiler-rt: https://lab.llvm.org/buildbot/#/builders/109/builds/75364
|
I landed this and compiler-rt is still broken. I tried again to fix it up and wasn't able to find the right way to convince CMake to suppress the diagnostic. I have run out of time to put into trying to solve the build system issues (this was a drive-by set of fixes that have now eaten up more time than I can afford to spend on this), so abandoning this work. Anyone who wants to take it across the finish line is welcome to do so. |
|
Tagging @llvm/pr-subscribers-compiler-rt-sanitizer for visibility again, since this PR is abandoned. |
|
(Was out of town...) As mentioned at #67360 (comment) , I think a default error is probably not appropriate. A priority value <= 100 may compete with a runtime |
I disagree -- we decided ages ago with init_priority that using these reserved priorities should be an error that can be ignored for the few cases where the reservation does not apply. We should be designing the Ux for these features around the majority of users, not the majority of people who will be expert user of the feature. Otherwise, we should not pretend these are reserved values -- it's far too easy to ignore a warning IMO. That said, I'm abandoning this work regardless. It was unplanned work I took on only because I noticed how deficient the implementation was regarding diagnostics when I went to add documentation for the attribute in the first place. A lot of our attributes from 2000-2012 or so are similarly problematic, unfortunately. So I wouldn't be surprised to see this come up again in the future. |
First, it would be a shame if you abandon this over this bit, there is plenty of value to this patch, even as far as the refactoring (which makes some other folks' patches easier to review!). While I agree with you that the error is appropriate, it doesn't seem practical at the moment thanks to GCC's implementation. Abandoning this patch means that those will NEVER be an error, so at least having this patch in gives us an opportunity to make this an error 'someday'. IMO, a warning/Warning-as-default-error are both appropriate (with preference towards the latter) 'for now', and we should fight the battle of making this a hard error another time. |
That doesn't solve the issue though -- we have |
|
I missed the compiler-rt bit there, thats a real shame, this is a really valueable patch that I'd hope someone would take up. While "GCC Doesnt Error" isn't particularly compelling and we SHOULD be better than that, I'd also prefer not throwing the baby out with the bath water. That said, if you're unable to do the cmake work, I'm hopeful someone else can come along. Is there a 'beginner commandeer' (ala-beginner-bug?) tag we could throw on this? |
|
If Aaron weren't able to fix this in several commits, I'm not sure it's a |
Functions with these attributes will be automatically called before main() or after main() exits gracefully. In glibc environments, the constructor function is passed the same arguments as main(), so that signature is allowed. In all other environments, we require the function to accept no arguments and either return
voidorint. The functions must use the C calling convention. In C++ language modes, the functions cannot be a nonstatic member function, or a consteval function.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, but we downgrade the error to be a warning which defaults to an error to ease use for implementers like compiler-rt or libc.