[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added inline comments.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:639
+  if (CXXGlobalInits.empty())
+    return;
 
----------------
hubert.reinterpretcast wrote:
> Can this part be committed in a separate patch? It does not appear to have dependencies on other parts of this patch and has the appearance of being a possible change for other platforms.
Sure. I will create a NFC patch for this part and try to commit it first. But so far, I think I can keep this part in this patch just for review purpose to make the patch look nicer?


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:704
   AddGlobalDtor(Fn);
+  CXXGlobalDtors.clear();
 }
----------------
hubert.reinterpretcast wrote:
> If this is another drive-by fix, can we put it in a separate patch?
Sure, I will put it in the above mentioned clean-up NFC patch as well.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:700
+
+    Fn = CreateGlobalInitOrDestructFunction(
+        FTy, llvm::Twine("__sterm80000000_clang_") + GlobalUniqueModuleId, FI,
----------------
hubert.reinterpretcast wrote:
> The called function is to be renamed.
Any suggestions here about how to represent the functionality of `__sterm` and `_GLOBAL__D_a` into one word? Cannot think of a good name...

Actually I am wondering do we need to rename the `Destruct` part? The `__sterm` and `_GLOBAL__D_a` do destruct the object by calling sterm finalizers and object destructors?


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:807
 
 void CodeGenFunction::GenerateCXXGlobalDtorsFunc(
     llvm::Function *Fn,
----------------
hubert.reinterpretcast wrote:
> This function is to be renamed.
 I will try `GenerateCXXGlobalDestructFunc` based on the thoughts as I also mentioned elsewhere that the `__sterm` and `_GLOBAL__D_` function do destruct the object by calling sterm finalizers and object destructors.

Any suggestions or concerns about this renaming? Or do we really need to rename this one?


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4467
+
+  // Create a variable destruction function.
+  const CGFunctionInfo &FI = CGM.getTypes().arrangeNullaryFunction();
----------------
hubert.reinterpretcast wrote:
> Suggestion:
> Create the finalization action associated with a variable.
I don't get your point, can you elaborate on this a little bit?


================
Comment at: clang/test/CodeGen/static-init.cpp:8
+// RUN:   FileCheck %s
 
 struct test {
----------------
jasonliu wrote:
> Looks like the non-inline comments are easier to get ignored and missed, so I will copy paste the non-inlined comment I previously had:
> ```
> -fregister_global_dtors_with_atexit does not seem to work properly in current implementation.
> We should consider somehow disabling/report_fatal_error it instead of letting it generate invalid code on AIX.
> ```
Thanks for doing so!

The semantic of `global_dtors` here on AIX is `__sterm` function, which we are never gonna register with `__atexit`. I am thinking we can disable it in a follow-up driver patch with the handling of `-fno-use-cxa-atexit`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166