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

Tue Jun 16 17:01:15 PDT 2020

hubert.reinterpretcast added inline comments.

================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4489
+  // DestructCallBlock, otherwise jump to EndBlock directly.
+  CGF.EmitCXXGuardedInitBranch(NeedsDestruct, DestructCallBlock, EndBlock,
+                               CodeGenFunction::GuardKind::VariableGuard, &D);
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > 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.
> Thank you for pointing this out. I will adjust the function name to `EmitCXXGuardedInitOrDestructBranch` later to match our usage.
> 
> In terms of `branch_weights`,  this is something I am not familiar with. So please correct me if I am wrong. Some of my superficial thoughts would be are we able to apply `branch_weights` on a branch associated with finalization-on-unload? Should it be set as `nullptr`,  because we would not know how many times we will call `__sterm`(and each sterm finalizer contained)?
> 
> BTW, I will add a testcase for a static local and we can update the `branch_weights` part later along with the reviews.
I don't think we want to generate `branch_weights`. We already expect `__sterm` to be called rarely. Just changing the function name is not going to help though. A caller would need to indicate which of "init" or "destruct" is intended.

================
Comment at: clang/test/CodeGen/static-init.cpp:8
+// RUN:   FileCheck %s

 struct test {
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Xiangling_L wrote:
> > > jasonliu wrote:
> > > > Looks like the non-inline comments are easier to get ignored and missed, so I will copy paste the non-inlined comment I previously had:
> > > > ```
> > > > -fregister_global_dtors_with_atexit does not seem to work properly in current implementation.
> > > > We should consider somehow disabling/report_fatal_error it instead of letting it generate invalid code on AIX.
> > > > ```
> > > Thanks for doing so!
> > > 
> > > The semantic of `global_dtors` here on AIX is `__sterm` function, which we are never gonna register with `__atexit`. I am thinking we can disable it in a follow-up driver patch with the handling of `-fno-use-cxa-atexit`.
> > The semantic of `global_dtors` is not limited to the `__sterm` functions associated with C++ cleanup actions. With respect to user-declared `__attribute__((__destructor__))` functions, the option could improve the interleaving of those cleanup actions with cleanup actions registered by user-declared `__attribute__((__constructor__))` functions.
> > 
> > This provides that rationale for separating the `__sterm` functions associated with the C++ cleanup actions from the other "destructor" functions.
> Yeah, I agree that the semantic of `global_dtors` should contain both `__sterm` and `__attribute__((__destructor__))` dtors. And we do need some rationale to separate `__sterm` and those other `destructor` functions out for AIX.
> 
> However, I doubt the `-fregister_global_dtors_with_atexit` option is something we should use. Cuz dtors with `__attribute__((__destructor__))` actually are equivalent to `__dtor` functions in AIX's semantic which need to be registered with `__atexit`. However, we shouldn't register `__sterm` with `__atexit`. So it seems this option does not work well for AIX either we set it to true or false, and we need to figure something else out when we start supporting `__attribute__((__destructor__))` and `__attribute__((__constructor__))`?
Yes, we should look further into this when `__attribute__((__destructor__))` is supported.

The difference between `atexit`-registered functions and `__sterm` functions is in whether or not they are run when unloading a module (which termination also does).

Thanks for your observation re: the similarity between `__attribute__((__destructor__))` functions and the `__dtor` functions. Thinking about it a bit, the level of similarity is rather tied to the option in question. If we choose to register using `atexit`, we need to consider introducing the corresponding `unatexit`. This means that even when the option in question is active, we will produce entries into the `global_dtors` array at the IR level.

================
Comment at: clang/test/CodeGen/static-init.cpp:31
+// CHECK: @llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @__sinit80000000_clang_510e898aa8d263cac999dd03eeed5b51, i8* null }]
+// CHECK: @llvm.global_dtors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @__sterm80000000_clang_510e898aa8d263cac999dd03eeed5b51, i8* null }]
+
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > Okay, the restricted scope of this patch seems to landed in a place where how the `llvm.global_dtors` array will be handled is not indicated...
> > > 
> > > The backend should implement the semantics of the IR construct. When handling said array in the IR, it seems the method to handle the requisite semantics would be to generate an alias (with the appropriate linkage for the linker to pick it up) that is named using the `__sinit`/`__sterm` convention. Which is to say that at least some part of the naming should eventually move into the LLVM side and out of Clang.
> > > 
> > > Please update the description of this patch to indicate that:
> > > 
> > >   - The `llvm.global_ctors` and `llvm.global_dtors` functionality has not yet been implemented on AIX.
> > >   - This patch temporarily side-steps the need to implement that functionality.
> > >   - The apparent uses of that functionality in this patch (with respect to the name of the functions involved) are not representative of how the functionality will be used once implemented.
> > > 
> > I think the `llvm.global_ctors` usage is also more extensive here (and not symmetric on the finalization side):
> > ```
> > struct C {
> >   C(int) {}
> >   ~C() {}
> > };
> > 
> > C c0 = 42;
> > template <typename T> C c = 42;
> > inline C c1 = 42;
> > 
> > int main(void) {
> >   (void) &c<int>;
> > }
> > ```
> Yes, we should definitely test template variable(and other WeakODR linkage symbols) and inline variable. However, this patch hasn't covered these aspects yet. My plan was to set the basic frame using this patch, and have some small follow-up patches to clean up the implementation. Do you think we should cover these in this patch as well? I kinda feel it's already a bit big one..
The patch is already limited in scope in such a way that covering the WeakODR linkage cases would not fit well. I'm fine with handling them later. There may be work specific for these with respect to the symmetry between the initialization and the finalization.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166