Page MenuHomePhabricator

[AIX][Frontend] Static init implementation for AIX considering no priority
ClosedPublic

Authored by Xiangling_L on Feb 6 2020, 2:00 PM.

Details

Summary
    • Provides no piroirity supoort && disables three priority related attributes: init_priority, ctor attr, dtor attr;
  • '-qunique' in XL compiler equivalent behavior of emitting sinit and sterm functions name using getUniqueModuleId() function in LLVM (currently no support for InternalLinkage and WeakODRLinkage symbols);
  • Add testcases to emit IR sample with sinit80000000, dtor, and __sterm80000000
  • Temporarily side-steps the need to implement the functionality of llvm.global_ctors and llvm.global_dtors arrays. The 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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Xiangling_L marked 35 inline comments as done.
Xiangling_L edited the summary of this revision. (Show Details)

Address another round of reviews;

clang/lib/CodeGen/CGDeclCXX.cpp
644

Sure. I will create a NFC patch for this part and try to commit it first. But so far, I think I can keep this part in this patch just for review purpose to make the patch look nicer?

699

Any suggestions here about how to represent the functionality of __sterm and _GLOBAL__D_a into one word? Cannot think of a good name...

Actually I am wondering do we need to rename the Destruct part? The __sterm and _GLOBAL__D_a do destruct the object by calling sterm finalizers and object destructors?

720

Sure, I will put it in the above mentioned clean-up NFC patch as well.

816

I will try GenerateCXXGlobalDestructFunc based on the thoughts as I also mentioned elsewhere that the __sterm and _GLOBAL__D_ function do destruct the object by calling sterm finalizers and object destructors.

Any suggestions or concerns about this renaming? Or do we really need to rename this one?

clang/lib/CodeGen/ItaniumCXXABI.cpp
4548

I don't get your point, can you elaborate on this a little bit?

clang/test/CodeGen/static-init.cpp
0

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.

hubert.reinterpretcast marked an inline comment as done.Jun 12 2020, 6:22 PM
hubert.reinterpretcast added inline comments.
clang/lib/CodeGen/CGDeclCXX.cpp
644

Sure; you'd need the new patch to exist before rebasing on it anyway. We can leave the rebasing to the commit or later in the review.

699

Being clear in the naming of the function helps to signal its purpose to future maintainers. The sterm finalizers are unlikely to be understood from Destruct (it implies plain destruction a bit too much). Maybe "cleanup"?

816

I think "cleanup" works here too.

clang/lib/CodeGen/ItaniumCXXABI.cpp
4548

This is my suggestion for the comment (to replace "Create a variable destruction function").

clang/test/CodeGen/static-init.cpp
15

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.
clang/test/CodeGen/static-init.cpp
0

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.

clang/lib/CodeGen/CGDeclCXX.cpp
276

s/atexit has wrong parameter type/Argument to atexit has a wrong type/;

301

s/unatexit has wrong parameter type/Argument to unatexit has a wrong type/;

645

Replace the call to toNullTerminatedStringRef and the assignment with a call to toVector.

646

CreateGlobalInitOrDestructFunction currently calls Create with llvm::GlobalValue::InternalLinkage and also calls SetInternalFunctionAttributes. Setting ExternalLinkage after-the-fact seems brittle.

653

Replace the call to toNullTerminatedStringRef and the assignment with a call to toVector.

689–690

I think "cleanup" works here too.

693–695

s/Create our global destructor function/Create our global cleanup function/;

826–827

s/dtors/cleanups/;

clang/lib/CodeGen/CodeGenModule.h
472

Near duplicate comment line.

473

s/finalizers/finalizer/;
s/it contains/it instead contains/;

474

s/instead that need to run/, which also run/;

1060

s/a destructor/an sterm finalizer/;
s/global destructor/global cleanup/;

1061

s/Dtor/StermFinalizer/;

1463–1464

s/destructs/performs cleanup associated with/;

clang/lib/CodeGen/ItaniumCXXABI.cpp
4532

There should not be period at the end of a report_fatal_error message.

4536

s/and/and (as appropriate)/;

4561

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).

4562

Remove the "Otherwise [ ... ]".

4565

Does this fit on one line?

4573

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.

clang/lib/Sema/SemaDeclAttr.cpp
6944

s/unsupported on AIX yet/is not yet supported on AIX/;

6953

s/unsupported on AIX yet/is not yet supported on AIX/;

7151

s/unsupported on AIX yet/is not yet supported on AIX/;

clang/lib/CodeGen/ItaniumCXXABI.cpp
4566

There are uses of CreateIsNull with snake_case; this is the only camelCase instance.

clang/test/CodeGen/static-init.cpp
5

Minor nit: Don't use a semicolon after a namespace definition. Apply throughout.

clang/test/CodeGen/static-init.cpp
15

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>;
}
Xiangling_L edited the summary of this revision. (Show Details)Jun 15 2020, 4:46 PM
Xiangling_L marked 35 inline comments as done.

Renamed some functions;
Add one more test;
etc.

clang/lib/CodeGen/ItaniumCXXABI.cpp
4573

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.

clang/test/CodeGen/static-init.cpp
0

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__))?

15

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..

clang/lib/CodeGen/ItaniumCXXABI.cpp
4573

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.

clang/test/CodeGen/static-init.cpp
0

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.

15

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.

jasonliu added inline comments.Jun 17 2020, 7:58 AM
clang/lib/CodeGen/CGDeclCXX.cpp
707

Correct me if I'm wrong...
GlobalUniqueModuleId will always get “initialized" in EmitCXXGlobalInitFunc. And EmitCXXGlobalInitFunc will always run before EmitCXXGlobalCleanUpFunc in CodeGenModule::Release(). So in theory, we do not need to initialize it again in here.
If we could not assert (!GlobalUniqueModuleId.empty()) here, that tells me in EmitCXXGlobalInitFunc we effectively get an empty string after GlobalUniqueModuleId = GlobalUniqueModuleId.substr(1);. So something is not right?

clang/lib/CodeGen/CodeGenModule.h
471–473

Word after ; should not be Capitalize. We should use small case, or change this to ..
I would prefer changing it to ..

clang/test/CodeGen/static-init.cpp
126

Is checking this Attrs necessary?

141

I think we used to have dso_local marked on sinit and sterm function in previous diff. Now they are gone. What's the reason for removing them?

jasonliu added inline comments.Jun 17 2020, 8:45 AM
clang/lib/CodeGen/CGDeclCXX.cpp
834

nit: trailing whitespace

clang/test/CodeGen/static-init.cpp
15

nit: trailing whitespace in this line.
struct Test4 {

Xiangling_L marked 11 inline comments as done.
Xiangling_L edited the summary of this revision. (Show Details)

Remove trailing spaces;
Update the testcase;
Adjust the EmitGuardedInitBranch function;

Xiangling_L added inline comments.Jun 17 2020, 7:14 PM
clang/lib/CodeGen/CGDeclCXX.cpp
707

Actually GlobalUniqueModuleId will not always get “initialized" in EmitCXXGlobalInitFunc. In some cases, only __sterm will be generated for the var decl.
e.g.
The static local variable won't be inited before main() but when called at the first time.

struct Test4 {
  Test4();
    ~Test4();
  };

  void f() {
    static Test4 staticLocal;
  }
clang/test/CodeGen/static-init.cpp
126

I missed to delete this line. Thanks for pointing it out.

141

dso_local affects calls to the function in terms of how the compiler will do the optimization on the call. Since we won't be generating any calls to the function (the linker arranges for the calls to happen indirectly), marking them dso_local does not help any. So I think it doesn't really matter if we put dso_local here.

Xiangling_L marked an inline comment as done.

Adjust the patch corresponding to D81972;

Fix the previous bad version;

jasonliu added inline comments.Jun 18 2020, 7:40 AM
clang/lib/CodeGen/CGDeclCXX.cpp
345

Do we need to change/complicate the interface for this function, just to do a call to Builder.CreateCondBr()?
Could we call that function directly from where it's needed?

642–643

This comment does not apply well on top of this early return for some platform. I think you could move it to current line 665?

Xiangling_L marked 3 inline comments as done.

Removed a redundant header file;
Addressed comments;

clang/lib/CodeGen/CGDeclCXX.cpp
345

Sure, we can. Thank you for your suggestion. I was hoping to use one function to synthesize the guarded init or destruct branch. But I think it seems better if we wait for further more usage of guarded destruct branch to do so and not complicate stuff in this patch.

jasonliu accepted this revision.Jun 18 2020, 10:14 AM

LGTM. But I suggest to wait for @hubert.reinterpretcast to have another scan at this before landing.

This revision is now accepted and ready to land.Jun 18 2020, 10:14 AM
hubert.reinterpretcast marked an inline comment as done.

Thanks; LGTM with minor nit.

clang/lib/CodeGen/CodeGenModule.h
1060

Minor nit: "a sterm" should be "an sterm" if "sterm" is pronounced like "es-term".

clang/test/CodeGenCXX/aix-static-init.cpp
189

Just a comment: The finalization order of static locals in relation to non-locals cannot (using the mechanisms available) be guaranteed to be consistent with the reverse order of initialization. I don't think that there is a clearly "correct" order, so "whatever falls out" is probably okay.

Xiangling_L edited the summary of this revision. (Show Details)Jun 19 2020, 4:52 AM
This revision was automatically updated to reflect the committed changes.
Xiangling_L marked an inline comment as done.

@rjmccall

FYI this diff appears to be breaking a ptrauth test on apple/swift/master-next (clang/test/CodeGenCXX/ptrauth-static-destructors.cpp).

Investigating this further.

@rjmccall

FYI this diff appears to be breaking a ptrauth test on apple/swift/master-next (clang/test/CodeGenCXX/ptrauth-static-destructors.cpp).

Investigating this further.

Looks like this was addressed in:

https://github.com/apple/llvm-project/commit/8e8176c0bee7215b397d78cfff6e617cfc50e248

and
https://github.com/apple/llvm-project/commit/4292b7edfcf0672da076a0b4e3d4fde8b2672359#diff-75a7118d42116297f6a0b1a0fd020604