Diagnose problematic uses of constructor/destructor attribute

clang/docs/ReleaseNotes.rst

@@ -204,6 +204,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
 -----------------------------------

clang/include/clang/Basic/AttrDocs.td

@@ -7255,8 +7255,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

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",

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -2870,6 +2870,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<
@@ -3182,6 +3184,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<
@@ -3195,6 +3204,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<

clang/lib/Sema/SemaDeclAttr.cpp

@@ -2352,26 +2352,126 @@ 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 < sizeof(Expected) / sizeof(QualType) && I < FPT->getNumParams();
+       ++I) {
+    QualType AT = FPT->getParamType(I);
+
+    if (!Context.hasSameUnqualifiedType(AT, Expected[I])) {
+      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();
+          if (!Qs.empty())
+            return false;
+          continue; // Accepted as an extension.
+        }
+      }
+      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 +3988,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));
 }
 
@@ -9087,13 +9180,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);

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
 

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
 

clang/test/Sema/constructor-attribute-diag-group.c

@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify=err %s
+// RUN: %clang_cc1 -fsyntax-only -verify=warn -Wno-error=priority-ctor-dtor %s
+// RUN: %clang_cc1 -fsyntax-only -verify=okay -Wno-priority-ctor-dtor %s
+// RUN: %clang_cc1 -fsyntax-only -verify=okay -Wno-prio-ctor-dtor %s
+// okay-no-diagnostics
+
+void f(void) __attribute__((constructor(1)));   // warn-warning {{'constructor' attribute requires integer constant between 101 and 65535 inclusive}} \
+                                                   err-error {{'constructor' attribute requires integer constant between 101 and 65535 inclusive}}
+void f(void) __attribute__((destructor(1)));    // warn-warning {{'destructor' attribute requires integer constant between 101 and 65535 inclusive}} \
+                                                   err-error {{'destructor' attribute requires integer constant between 101 and 65535 inclusive}}