Page MenuHomePhabricator

[AIX][FE] Support constructor/destructor attribute
ClosedPublic

Authored by Xiangling_L on Nov 5 2020, 3:02 PM.

Details

Summary

Support attribute((constructor)) and attribute((destructor)) on AIX

Diff Detail

Event Timeline

Xiangling_L created this revision.Nov 5 2020, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2020, 3:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Xiangling_L requested review of this revision.Nov 5 2020, 3:02 PM

Remove unused function parameters;
Move testcases to CodeGenCXX folder;

aaron.ballman added inline comments.Nov 6 2020, 5:54 AM
clang/lib/CodeGen/CGDeclCXX.cpp
276–279

This will likely cause unused variable warnings in release builds due to the assert macro.

303–306

Same here.

clang/lib/CodeGen/CodeGenModule.h
1482

There's a fixme comment a few lines up about hardcoding priority being gross and this sort of extends the grossness a bit. Perhaps these functions should accept a DestructorAttr */ConstructorAttr * that can be null?

clang/lib/CodeGen/ItaniumCXXABI.cpp
2537

Do you need this? I think you can get VoidTy off CGM already which seems to be the only use of CGF.

2582

Is this assumption safe though given that there are calling convention attributes that can be written on the function?

2646

Same question here as above.

Xiangling_L marked 6 inline comments as done.Nov 10 2020, 8:19 AM
Xiangling_L added inline comments.
clang/lib/CodeGen/CodeGenModule.h
1482

Yeah, I can understand that putting a magic number as 65535 here is gross, but a bool with default argument also falls into that way? Or you are indicating it's better to not use default argument?

clang/lib/CodeGen/ItaniumCXXABI.cpp
2537

Thanks, you are right. I am gonna remove it.

2582

Actually I copied this comment from where linux implemented the dtor attribute functions. I think it makes sense to make this assumption. Because when they are used as destructor functions, they actually don't have any caller from source.

aaron.ballman added inline comments.Nov 10 2020, 8:53 AM
clang/lib/CodeGen/CodeGenModule.h
1482

I think the signature should be:

void AddGlobalDtor(llvm::Function *Dtor, DestructorAttr *DA = nullptr);

(I don't have strong opinions about whether the attribute pointer should be defaulted to null or not.) IsDtorAttrFunc is implied by a nonnull pointer and the priority can be gleaned directly from that attribute (or set to the magic number if the attribute pointer is null).

clang/lib/CodeGen/ItaniumCXXABI.cpp
2582

Ah, the comment confused me because the user could always write something like:

[[clang::stdcall, gnu::destructor]] void func();

where the destructor function is not something you can call with the default (cdecl) calling convention. Should the comment say "we can reasonably call with the correct CC" instead to avoid this confusion?

Xiangling_L marked 5 inline comments as done.Nov 10 2020, 10:37 AM
Xiangling_L added inline comments.
clang/lib/CodeGen/CodeGenModule.h
1482

Oh, I see what do you mean here. But the issue is AddGlobalDtor is not only used for dtor attribute functions, so we cannot always glean the priority from a DestructorAttr.

If use DestructorAttr, the function signature has to be:

void AddGlobalDtor(llvm::Function *Dtor, int Priority, DestructorAttr *DA = nullptr);

So that's why I think a bool is good enough here.

clang/lib/CodeGen/ItaniumCXXABI.cpp
2582

Sure, that would make more sense under AIX context.

aaron.ballman added inline comments.Nov 10 2020, 10:43 AM
clang/lib/CodeGen/CodeGenModule.h
1482

Oh, I see what do you mean here. But the issue is AddGlobalDtor is not only used for dtor attribute functions, so we cannot always glean the priority from a DestructorAttr.

The only place that calls AddGlobalDtor() without an attribute handy is AddCXXStermFinalizerToGlobalDtor() and the only call to that function always passes in the value 65535 (in ItaniumCXXABI.cpp), so passing a null attribute pointer there will behave correctly.

Xiangling_L marked 2 inline comments as done.Nov 10 2020, 11:13 AM
Xiangling_L added inline comments.
clang/lib/CodeGen/CodeGenModule.h
1482

The reason why AddCXXStermFinalizerToGlobalDtor() currently always pass in 65535 is because priority hasn't been supported by AIX yet(You can find the assertion few lines above there). And that would happen in the near future.

The same thing happens in function EmitCXXGlobalCleanUpFunc(), after we support init priority, we won't always use default value.

Address comments;
Move testcases to CodeGen folder instead of CodeGenCXX since ctor/dtor attribute should work in both C&C++ mode;

aaron.ballman accepted this revision.Nov 10 2020, 1:16 PM

I think this generally seems reasonable, but I'm far from an AIX expert so you should wait a few days in case other reviewers have feedback.

clang/lib/CodeGen/CodeGenModule.h
1482

Ahhh, I understand now -- thank you for clarifying. In that case, I think adding the bool to the parameter list is fine.

clang/test/CodeGen/aix-sinit-register-global-dtors-with-atexit.cpp
25 ↗(On Diff #304256)

I think the file probably should still live in CodeGenCXX given that this is a C++-specific test (alternatively, you could split the test file into separate C and C++ tests if you think that's important).

This revision is now accepted and ready to land.Nov 10 2020, 1:16 PM

I think this generally seems reasonable, but I'm far from an AIX expert so you should wait a few days in case other reviewers have feedback.

Thanks for reviews. And @sfertile has kindly offered a further review.

Add testcases to both CodeGen and CodeGenCXX folder;

Fix minor issues in the testcase;

sfertile added inline comments.Nov 13 2020, 7:56 AM
clang/test/CodeGen/aix-constructor-attribute.cpp
0

Did you mean for this test to be a C or C++ test? If it is meant to be C++ it needs to mangle the function names, but considering the director it is in and the fact we have the same test in CodeGenCXX directory I expect this was meant to be C in which case it needs a .c extension and lose the -x c++ in the run steps.

clang/test/CodeGen/aix-destructor-attribute.cpp
0

Similarly if this is meant to be a C test, give the file a .c extension.

clang/test/CodeGen/aix-sinit-register-global-dtors-with-atexit.cpp
1 ↗(On Diff #304616)

I really think this test should be included in aix-destructor-attribute.c to highlight the codeine differences with and without the option.

clang/test/CodeGenCXX/aix-destructor-attribute.cpp
2

Can I ask why this is added as a new file? Its the same test as aix-sinit-register-global-dtors-with-atexit.cpp but without using -fregister-global-dtors-with-atexit. I suggest combining the 2.

Xiangling_L marked 5 inline comments as done.Nov 16 2020, 8:25 AM
Xiangling_L added inline comments.
clang/test/CodeGen/aix-constructor-attribute.cpp
0

Thanks for pointing this out to me, I should've changed the file extension when I copied this testcase from CXX testcases.

clang/test/CodeGenCXX/aix-destructor-attribute.cpp
2

Agree, it makes more sense to combine two. I tried to make testcases look cleaner by splitting them.

Xiangling_L marked 2 inline comments as done.

Update testcases;

sfertile accepted this revision.Nov 18 2020, 8:54 AM

Thanks for the updates. LGTM.

This revision was automatically updated to reflect the committed changes.