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