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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
