[PATCH] D104420: thread_local support for AIX

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

I think we're good after some last updates.



================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:340-342
+  // Ignore all attributes except ConstInit when IgnoreAttrs is true.
+  bool isEmittedWithConstantInitializer(const VarDecl *VD,
+                                        bool IgnoreAttrs = false) const {
----------------
See suggested change for renaming the parameter name from `IgnoreAttrs` to `InspectInitForWeakDef`.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:349
     // the variable is weak, such examination would not be correct.
-    if (VD->isWeak() || VD->hasAttr<SelectAnyAttr>())
+    if (!IgnoreAttrs && (VD->isWeak() || VD->hasAttr<SelectAnyAttr>()))
       return false;
----------------



================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2957-2958
+    // also when the symbol is weak.
+    if (CGM.getTriple().isOSAIX() &&
+        isEmittedWithConstantInitializer(VD, true) &&
+        !VD->needsDestruction(getContext())) {
----------------
The block here is meant to be entered only if there is a definition of the variable (even if weak) in the current translation unit. This is mostly enforced with `isEmittedWithConstantInitializer`, but that will return true for `constinit` even if only a declaration is available.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2971
+        isEmittedWithConstantInitializer(VD, true) &&
+        !VD->needsDestruction(getContext())) {
+      // Emit a weak global function referring to the initialization function.
----------------
jamieschmeiser wrote:
> hubert.reinterpretcast wrote:
> > Unfortunately `needsDestruction` cannot be counted on at this time for some incomplete types, see https://llvm.org/pr51079.
> I think it is okay to leave the code as-is as it will then be fixed when that problem is fixed.
Yes: I think (if the code works correctly), the `needsDestruction` query here only becomes significant when the type is complete -- and, in that case, we want it to work the way it currently does in relation to classes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104420/new/

https://reviews.llvm.org/D104420