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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:293
+CodeGenFunction::unregisterGlobalDtorWithUnAtExit(llvm::Function *dtorStub) {
+  // extern "C" int unatexit(void (*f)(void));
+  assert(dtorStub->getFunctionType() ==
----------------
Please add a comment explaining the meaning of the zero and non-zero return values.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:603
   if (!PrioritizedCXXGlobalInits.empty()) {
+    assert(!UseSinitAndSterm && "Prioritized Sinit and Sterm functions are not"
+                                " supported yet.");
----------------
I don't think we should capitalize "sinit" and "sterm" in comments. Indeed, there's a comment below where they are not capitalized.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:639
+  if (CXXGlobalInits.empty())
+    return;
 
----------------
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.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:641
 
-  for (size_t i = 0; i < FileName.size(); ++i) {
-    // Replace everything that's not [a-zA-Z0-9._] with a _. This set happens
-    // to be the set of C preprocessing numbers.
-    if (!isPreprocessingNumberBody(FileName[i]))
-      FileName[i] = '_';
-  }
+  // Include the filename in the symbol name. When not using sinit and sterm
+  // functions, including "sub_" matches gcc and makes sure these symbols
----------------
The first sentence does not apply unconditionally in the status quo of this patch. Perhaps keep the original comment completely with something like:
```
// When not using sinit and sterm functions:
// ...
```


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:682
 void CodeGenModule::EmitCXXGlobalDtorFunc() {
   if (CXXGlobalDtors.empty())
     return;
----------------
Following from my previous comments on `CXXGlobalDtors`, I am not convinced that `__sterm` functions match the role.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:704
   AddGlobalDtor(Fn);
+  CXXGlobalDtors.clear();
 }
----------------
If this is another drive-by fix, can we put it in a separate patch?


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:817
       std::tie(CalleeTy, Callee, Arg) = DtorsAndObjects[e - i - 1];
-      llvm::CallInst *CI = Builder.CreateCall(CalleeTy, Callee, Arg);
+      llvm::CallInst *CI = (Arg == nullptr)
+                               ? Builder.CreateCall(CalleeTy, Callee)
----------------
This change would not belong in this function if its scope remains the generation of a "GlobalDtorsFunc" (based on my understanding of the latter).


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4445
+  if (D.getTLSKind() != VarDecl::TLS_None)
+    llvm::report_fatal_error("thread local storage unimplemented on AIX yet");
+
----------------
s/unimplemented on AIX yet/not yet implemented on AIX/;


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4467
+
+  // Create a variable destruction function.
+  const CGFunctionInfo &FI = CGM.getTypes().arrangeNullaryFunction();
----------------
Suggestion:
Create the finalization action associated with a variable.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4480
+
+  llvm::Value *NeedsDestruct = CGF.Builder.CreateIsNull(V, "guard.hasDtor");
+
----------------
I don't think this is a real "guard variable". I think the comments need to explain this (including scare quotes for "guard variable".


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

https://reviews.llvm.org/D74166