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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
