[PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

[Eric Fiselier via cfe-commits]

EricWF marked 7 inline comments as done.

================
Comment at: include/clang/Basic/AttrDocs.td:844
@@ +843,3 @@
+the indeterminate order of dynamic initialization. They can also be safely
+used by other static constructors across translation units.
+
----------------
rsmith wrote:
> static constructors -> dynamic initializers?
Changed "by other static constructors" -> "during dynamic initialization"

================
Comment at: include/clang/Basic/AttrDocs.td:858
@@ +857,3 @@
+  };
+  SAFE_STATIC T x = {42}; // OK.
+  SAFE_STATIC T y = 42; // error: variable does not have a constant initializer
----------------
rsmith wrote:
> OK even though T has a non-trivial destructor? This makes the variable unsafe to use during program shutdown in the general case.
Right, but I want this attribute to be able to work with (A)  the union trick for "trivial" destructors and (B) variables not used during shutdown.
I was planning on following this up with an additional feature to aid in the shutdown case as well, but I think there is value in separating the features.

Currently -Wglobal-destructors will still warn on that declaration, so at least the unsafe shutdown is not silently missed.

Does this behavior make sense to you?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6842
@@ +6841,3 @@
+def err_require_constant_init_failed : Error<
+  "variable does not have a constant initializer">;
+
----------------
rsmith wrote:
> It may be useful for this diagnostic to say why we consider this to be an error (that is, mention that there is a `require_constant_initialization` attribute attached to the variable). The attribute will typically be hidden behind a (perhaps unfamiliar to the reader) macro, so it may not be obvious if we don't point it out.
"variable does not have a constant initializer as required by 'require_constant_initializer' attribute"?

Or do we want the diagnostic to point to the attribute token in a "required from here"-like note?

================
Comment at: lib/Sema/SemaDecl.cpp:10519-10520
@@ +10518,4 @@
+      auto *CE = dyn_cast<CXXConstructExpr>(Init);
+      bool DiagErr = (var->isInitKnownICE() || (CE && CE->getConstructor()->isConstexpr()))
+          ? !var->checkInitIsICE() : !checkConstInit();
+      if (DiagErr)
----------------
rsmith wrote:
> Falling back to `checkConstInit` here will suppress the warning on some cases that are not technically constant initializers (but that Clang can emit as constants regardless). Is that what you want? If so, you should update the documentation to say that instead of saying that we only check for a constant initializer.
> Falling back to checkConstInit here will suppress the warning on some cases that are not technically constant initializers (but that Clang can emit as constants regardless). Is that what you want?

Not really. I would prefer this strictly conform to the standard so it can be used to portably detect possible dynamic initializers on other toolchains.

What would the correct fallback here be?

================
Comment at: test/SemaCXX/attr-require-constant-initialization.cpp:96-100
@@ +95,7 @@
+    ATTR static const int& temp_init = 42;
+#if 0
+    /// FIXME: Why is this failing?
+    __thread const int& tl_init = 42;
+    static_assert(__has_constant_initializer(tl_init), "");
+#endif
+}
----------------
rsmith wrote:
> Did you mean for this to still be here?
No.


https://reviews.llvm.org/D23385