[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:277
+             llvm::FunctionType::get(CGM.VoidTy, false) &&
+         "atexit has wrong parameter type.");
+
----------------
s/atexit has wrong parameter type/Argument to atexit has a wrong type/;


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:302
+             llvm::FunctionType::get(CGM.VoidTy, false) &&
+         "unatexit has wrong parameter type.");
+
----------------
s/unatexit has wrong parameter type/Argument to unatexit has a wrong type/;


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:649
+    FuncName = llvm::Twine("__sinit80000000_clang_", GlobalUniqueModuleId)
+                   .toNullTerminatedStringRef(FuncName);
+  else {
----------------
Replace the call to `toNullTerminatedStringRef` and the assignment with a call to `toVector`.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:657
+                           getTransformedFileName(getModule(), Storage))
+                   .toNullTerminatedStringRef(FuncName);
   }
----------------
Replace the call to `toNullTerminatedStringRef` and the assignment with a call to `toVector`.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:664
+  if (getCXXABI().useSinitAndSterm())
+    Fn->setLinkage(llvm::Function::ExternalLinkage);
 
----------------
`CreateGlobalInitOrDestructFunction` currently calls `Create` with `llvm::GlobalValue::InternalLinkage` and also calls `SetInternalFunctionAttributes`. Setting `ExternalLinkage` after-the-fact seems brittle.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:690
 
-void CodeGenModule::EmitCXXGlobalDtorFunc() {
-  if (CXXGlobalDtors.empty())
+void CodeGenModule::EmitCXXGlobalDestructFunc() {
+  if (CXXGlobalDtorsOrStermFinalizers.empty())
----------------
I think "cleanup" works here too.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:697
 
   // Create our global destructor function.
+  llvm::Function *Fn = nullptr;
----------------
s/Create our global destructor function/Create our global cleanup function/;


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:822
 
     // Emit the dtors, in reverse order from construction.
+    for (unsigned i = 0, e = DtorsOrStermFinalizers.size(); i != e; ++i) {
----------------
s/dtors/cleanups/;


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:472
   /// Global destructor functions and arguments that need to run on termination.
+  /// Global destructor functions and arguments that need to run on termination;
+  /// When UseSinitAndSterm is set, it contains sterm finalizers functions
----------------
Near duplicate comment line.


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:473
+  /// Global destructor functions and arguments that need to run on termination;
+  /// When UseSinitAndSterm is set, it contains sterm finalizers functions
+  /// instead that need to run on unloading a shared library.
----------------
s/finalizers/finalizer/;
s/it contains/it instead contains/;


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:474
+  /// When UseSinitAndSterm is set, it contains sterm finalizers functions
+  /// instead that need to run on unloading a shared library.
   std::vector<
----------------
s/instead that need to run/, which also run/;


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1059
+
+  /// Add a destructor to the C++ global destructor function.
+  void AddCXXDtorEntry(llvm::FunctionCallee DtorFn) {
----------------
s/a destructor/an sterm finalizer/;
s/global destructor/global cleanup/;


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1060
+  /// Add a destructor to the C++ global destructor function.
+  void AddCXXDtorEntry(llvm::FunctionCallee DtorFn) {
+    CXXGlobalDtorsOrStermFinalizers.emplace_back(DtorFn.getFunctionType(),
----------------
s/Dtor/StermFinalizer/;


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1462
 
-  /// Emit the function that destroys C++ globals.
-  void EmitCXXGlobalDtorFunc();
+  /// Emit the function that destructs C++ globals.
+  void EmitCXXGlobalDestructFunc();
----------------
s/destructs/performs cleanup associated with/;


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4448
+  if (CGM.getCodeGenOpts().CXAAtExit)
+    llvm::report_fatal_error("using __cxa_atexit unsupported on AIX.");
+  // Register above __dtor with atexit().
----------------
There should not be period at the end of a `report_fatal_error` message.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4452
+
+  // Emit __finalize function to unregister __dtor and call __dtor.
+  emitCXXStermFinalizer(D, dtorStub, addr);
----------------
s/and/and (as appropriate)/;


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4477
+  // registered by the atexit subroutine. If the referenced function is found,
+  // the unatexit returns a value of 0 and the related __dtor function will be
+  // called to destruct the object. Otherwise, a non-zero value is returned.
----------------
The "and the related [ ... ]" part is ambiguous in terms of describing `unatexit` versus the actions generated here.

Suggestion:
[ ... ] returns a value of 0, meaning that the cleanup is still pending (and we should call the __dtor function).


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4478
+  // the unatexit returns a value of 0 and the related __dtor function will be
+  // called to destruct the object. Otherwise, a non-zero value is returned.
+  llvm::Value *V = CGF.unregisterGlobalDtorWithUnAtExit(dtorStub);
----------------
Remove the "Otherwise [ ... ]".


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4481
+
+  llvm::Value *NeedsDestruct =
+      CGF.Builder.CreateIsNull(V, "needsDestruct");
----------------
Does this fit on one line?


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4489
+  // DestructCallBlock, otherwise jump to EndBlock directly.
+  CGF.EmitCXXGuardedInitBranch(NeedsDestruct, DestructCallBlock, EndBlock,
+                               CodeGenFunction::GuardKind::VariableGuard, &D);
----------------
Please add a test for a static local. Note the `branch_weights`. I do not believe the `branch_weights` associated with guarded initialization for a static local (what the called function is meant to deal with) is necessarily appropriate for a branch associated with finalization-on-unload.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6944
+      llvm::report_fatal_error(
+          "'constructor' attribute unsupported on AIX yet");
+    else
----------------
s/unsupported on AIX yet/is not yet supported on AIX/;


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6953
+    if (S.Context.getTargetInfo().getTriple().isOSAIX())
+      llvm::report_fatal_error("'destructor' attribute unsupported on AIX yet");
+    else
----------------
s/unsupported on AIX yet/is not yet supported on AIX/;


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7151
+      llvm::report_fatal_error(
+          "'init_priority' attribute unsupported on AIX yet");
+    else
----------------
s/unsupported on AIX yet/is not yet supported on AIX/;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166