[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority
Xiangling Liao via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 10 11:05:20 PDT 2020
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
More information about the cfe-commits
mailing list