[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/AST/ItaniumMangle.cpp:5217
+  else
+    Mangler.getStream() << D->getName();
+}
----------------
jasonliu wrote:
> 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. 
This is somewhere I am kinda on the fence. Personally, I think embed decl name in the __cxx_global_var_destruct_ / __cxx_global_vat_init_ as `mangleDynamicAtExitDestructor` does is more helpful for the user to debug the static init.
I am not sure if we want to give up that benefit and be consistent with current `__cxx_global_vat_init_ ` using number suffix or do we want to replace number suffix by decl name for `__cxx_global_vat_init_ ` as well.


================
Comment at: clang/lib/CodeGen/CGCXXABI.h:113
+
+  virtual bool isCXXGlobalInitAndDtorFuncInternal() const { return true; }
+
----------------
jasonliu wrote:
> Do we need this isCXXGlobalInitAndDtorFuncInternal? Looks like it's always going to return ! useSinitAndSterm() result.
AFAIK, not all platforms which use sinit and sterm have external linkage sinit/sterm functions. And also since for 7.2 AIX we are considering change sinit/sterm to internal linkage symbols, I seperate this query out.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:305
+  if (llvm::Function *unatexitFn =
+          dyn_cast<llvm::Function>(unatexit.getCallee()))
+    unatexitFn->setDoesNotThrow();
----------------
jasonliu wrote:
> Is there a valid case that unatexit.getCallee() returns a type which could not be cast to llvm::Function?
> i.e. Could we use cast instead of dyn_cast?
I used `cast` instead of `dyn_cast` before Diff 9 actually, and then I noticed that `clang-tidy` gave error and asked me to use `dyn_cast` instead. Cannot recall what the error says though...


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:20
 #include "SanitizerMetadata.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
----------------
jasonliu wrote:
> Is this Attr.h needed somewhere in this file?
Oops..this is something I forgot to remove. Good catch!


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4481
+
+  llvm::BasicBlock *DestructCheckBlock = CGF.createBasicBlock("destruct.check");
+  llvm::BasicBlock *EndBlock = CGF.createBasicBlock("destruct.end");
----------------
jasonliu wrote:
> 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. 
Thanks, I will try `destruct.call` instead.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74166