[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+                     bool IsDtorAttrFunc = false);
 
----------------
Xiangling_L wrote:
> aaron.ballman wrote:
> > Xiangling_L wrote:
> > > aaron.ballman wrote:
> > > > Xiangling_L wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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?
> > > > > 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?
> > > > 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).
> > > 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.
> > > 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.
> 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.
Ahhh, I understand now -- thank you for clarifying. In that case, I think adding the `bool` to the parameter list is fine.


================
Comment at: clang/test/CodeGen/aix-sinit-register-global-dtors-with-atexit.cpp:25
+struct test {
+  test();
+  ~test();
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90892