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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5217
+  else
+    Mangler.getStream() << D->getName();
+}
----------------
If I understand correctly, this function will come in pair with `__cxx_global_var_init`.
`__cxx_global_var_init` use number as suffix in the end to avoid duplication when there is more than one of those, but we are using the variable name as suffix here instead.
Do we want to use number as suffix here to match what `__cxx_global_var_init` does? It would help people to recognize the pairs and make them more symmetric. 


================
Comment at: clang/lib/CodeGen/CGCXXABI.h:113
+
+  virtual bool isCXXGlobalInitAndDtorFuncInternal() const { return true; }
+
----------------
Do we need this isCXXGlobalInitAndDtorFuncInternal? Looks like it's always going to return ! useSinitAndSterm() result.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:586
 
+  bool UseSinitAndSterm = getCXXABI().useSinitAndSterm();
+  if (UseSinitAndSterm) {
----------------
`const` for UseSinitAndSterm variable?


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:638
+  // Create our global initialization function.
+  if (!CXXGlobalInits.empty()) {
+    // Include the filename in the symbol name. When not using sinit and sterm
----------------
flip this for an early return?


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:691
+  if (UseSinitAndSterm) {
+    std::string FuncSuffix = GlobalUniqueModuleId;
+    Fn = CreateGlobalInitOrDestructFunction(
----------------
We might want to avoid this string copy construction if possible. 


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:20
 #include "SanitizerMetadata.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
----------------
Is this Attr.h needed somewhere in this file?


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:401
+  /// A unique trailing identifier as a part of sinit/sterm function when
+  /// UserSinitAndSterm set as true.
+  std::string GlobalUniqueModuleId;
----------------
nit: UserSinitAndSterm -> UseSinitAndSterm?
and we do not have that property here.
Suggestion:
when getCXXABI().useSinitAndSterm() return true.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4446
+
+  // Create __srterm function for the var decl.
+  llvm::Function *dtorStub = CGF.createAtExitStub(D, dtor, addr);
----------------
In this implementation we do not seem to have `__srterm` functions. I think we should refer to `__dtor` instead.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4479
+
+  llvm::Value *NeedsDestruct = CGF.Builder.CreateIsNull(V, "guard.hasSrterm");
+
----------------
Srterm is not used in this implementation.
Suggestion:
guard.hasDtorCalled


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4481
+
+  llvm::BasicBlock *DestructCheckBlock = CGF.createBasicBlock("destruct.check");
+  llvm::BasicBlock *EndBlock = CGF.createBasicBlock("destruct.end");
----------------
I think we need a better naming for this and make it consistent with the end block below.
As it is for now, `destruct.check` is confusing, as we are not checking anything here and we are just going to call destructor directly in this block. 


================
Comment at: clang/test/CodeGen/static-init.cpp:10
   ~test();
 } t;
 
----------------
Would it be better to test static init for at least two global variable?
Testing only one global variable does not show the whole picture of AIX static init.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166