[Clang] Make '-Wglobal-constructors` work on the GNU attributes

[jhuber6]

Summary:
The -Wglobal-constructors option is useful for restricting the usage
of global constructors / destructors. However, it currently ignores the
attributes that introduce global constructors, meaning that the module
can still have ctors if -Werror is set. If this is intentional by the
user, I believe it would be more correct to push the diagnostic.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Joseph Huber (jhuber6)

Changes

Summary:
The -Wglobal-constructors option is useful for restricting the usage
of global constructors / destructors. However, it currently ignores the
attributes that introduce global constructors, meaning that the module
can still have ctors if -Werror is set. If this is intentional by the
user, I believe it would be more correct to push the diagnostic.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaDeclAttr.cpp (+12)
• (modified) clang/test/SemaCXX/attr-require-constant-initialization.cpp (+5)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index fdf3b9c636127..9daa47c533ad2 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7596,6 +7596,18 @@ void Sema::ProcessDeclAttributeList(
     D->setInvalidDecl();
   }
 
+  // Warn on global constructors and destructors created by attributes.
+  if (D->hasAttr<ConstructorAttr>() &&
+      !getDiagnostics().isIgnored(diag::warn_global_constructor,
+                                  D->getLocation()))
+    Diag(D->getLocation(), diag::warn_global_constructor)
+        << D->getSourceRange();
+  if (D->hasAttr<DestructorAttr>() &&
+      !getDiagnostics().isIgnored(diag::warn_global_destructor,
+                                  D->getLocation()))
+    Diag(D->getLocation(), diag::warn_global_destructor)
+        << D->getSourceRange();
+
   // Do this check after processing D's attributes because the attribute
   // objc_method_family can change whether the given method is in the init
   // family, and it can be applied after objc_designated_initializer. This is a
diff --git a/clang/test/SemaCXX/attr-require-constant-initialization.cpp b/clang/test/SemaCXX/attr-require-constant-initialization.cpp
index 4c0a834551715..b1736ea7a3ebe 100644
--- a/clang/test/SemaCXX/attr-require-constant-initialization.cpp
+++ b/clang/test/SemaCXX/attr-require-constant-initialization.cpp
@@ -337,6 +337,11 @@ constexpr TestCtor<NotC> inval_constexpr(42); // expected-error {{must be initia
 ATTR constexpr TestCtor<NotC> inval_constexpr2(42); // expected-error {{must be initialized by a constant expression}}
 // expected-note@-1 {{in call to 'TestCtor(42)'}}
 
+[[gnu::constructor]] void ctor() {}
+// expected-warning@-1 {{declaration requires a global constructor}}
+[[gnu::destructor]] void dtor() {}
+// expected-warning@-1 {{declaration requires a global destructor}}
+
 #elif defined(TEST_THREE)
 #if defined(__cplusplus)
 #error This test requires C

[AaronBallman]

Thanks! You should also add a release note to clang/docs/ReleaseNotes.rst so users know about the change.

[AaronBallman]

I think this code should live in handleConstructorAttr() and handleDestructorAttr() instead. (The above code should live there as well, I suspect).

[erichkeane]

Also, why bother checking if it is ignored? We should just emit it, and count on the diagnostics engine to suppress it if it is ignored. We typically only check isIgnored if we are going to do 'a lot' of work.

[jhuber6]

Thanks for the feedback. I think the above code had to be put there because it relies on having the CUDA device attribute parsed already.

[erichkeane]

Why is this firing always? I thought this is only supposed to fire on Global and ThreadLocal variables according to the GCC patch summary. But this will fire any time there is a ConstructorAttr or DestructorAttr somewhere.

[AaronBallman]

How I see it is:

From GCC's docs: "The constructor attribute causes the function to be called automatically before execution enters main ()."

And -Wglobal-constructors is meant to tell you "this has a constructor which will fire before main" which matters for global and thread local variables, so the constructor attribute is sort of morally equivalent.

[erichkeane]

Ah! I missed that about the constructor attribute. Thanks! Makes sense to me.

[AaronBallman]

LGTM once precommit CI comes back green. Thank you!