gulfem added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1435
+ let Spellings = [GCC<"leaf">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
----------------
aaron.ballman wrote:
> gulfem wrote:
> > aaron.ballman wrote:
> > > gulfem wrote:
> > > > gulfem wrote:
> > > > > gulfem wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > gulfem wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > gulfem wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > gulfem wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > gulfem wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > gulfem wrote:
> > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > Should this attribute also be supported on
> > > > > > > > > > > > > > > > > things like ObjC method decls or other
> > > > > > > > > > > > > > > > > function-like interfaces?
> > > > > > > > > > > > > > > > Do I need to do anything else to support this
> > > > > > > > > > > > > > > > attribute in Objective-C as well?
> > > > > > > > > > > > > > > > I think we should support it in all the C
> > > > > > > > > > > > > > > > languages family.
> > > > > > > > > > > > > > > >I think we should support it in all the C
> > > > > > > > > > > > > > > >languages family.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > That's already happening automatically -- there's
> > > > > > > > > > > > > > > a C and C++ spelling available for it and the
> > > > > > > > > > > > > > > attribute doesn't specify that it requires a
> > > > > > > > > > > > > > > particular language mode or target.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Do I need to do anything else to support this
> > > > > > > > > > > > > > > > attribute in Objective-C as well?
> > > > > > > > > > > > > > > You can add multiple subjects to the list here,
> > > > > > > > > > > > > > > so you can have this apply to `Function,
> > > > > > > > > > > > > > > ObjCMethod` for both of those. Another one to
> > > > > > > > > > > > > > > consider is whether this attribute can be written
> > > > > > > > > > > > > > > on a block declaration (like a lambda, but with
> > > > > > > > > > > > > > > different syntax). Beyond that, it's mostly just
> > > > > > > > > > > > > > > documentation, devising the test cases to ensure
> > > > > > > > > > > > > > > the ObjC functionality behaves as expected,
> > > > > > > > > > > > > > > possibly some codegen changes, etc.
> > > > > > > > > > > > > > AFAIK, users can specify function attributes in
> > > > > > > > > > > > > > lambda expressions.
> > > > > > > > > > > > > > Lambda functions can only be accessed/called by the
> > > > > > > > > > > > > > functions in the same translation unit, right?
> > > > > > > > > > > > > > Leaf attribute does not have any effect on the
> > > > > > > > > > > > > > functions that are defined in the same translation
> > > > > > > > > > > > > > unit.
> > > > > > > > > > > > > > For this reason, I'm thinking that leaf attribute
> > > > > > > > > > > > > > would not have any effect if they are used in
> > > > > > > > > > > > > > lambda expressions.
> > > > > > > > > > > > > > Do you agree with me?
> > > > > > > > > > > > > > AFAIK, users can specify function attributes in
> > > > > > > > > > > > > > lambda expressions.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I always forget that you can do that for declaration
> > > > > > > > > > > > > attributes using GNU-style syntax...
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Lambda functions can only be accessed/called by the
> > > > > > > > > > > > > > functions in the same translation unit, right?
> > > > > > > > > > > > >
> > > > > > > > > > > > > Not necessarily, you could pass one across TU
> > > > > > > > > > > > > boundaries like a function pointer, for instance.
> > > > > > > > > > > > > e.g.,
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > // TU1.cpp
> > > > > > > > > > > > > void foo() {
> > > > > > > > > > > > > auto l = []() { ... };
> > > > > > > > > > > > > bar(l);
> > > > > > > > > > > > > }
> > > > > > > > > > > > >
> > > > > > > > > > > > > // TU2.cpp
> > > > > > > > > > > > > void bar(auto func) {
> > > > > > > > > > > > > func();
> > > > > > > > > > > > > }
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > Not necessarily, you could pass one across TU
> > > > > > > > > > > > > boundaries like a function pointer, for instance.
> > > > > > > > > > > > > e.g.,
> > > > > > > > > > > > As I mentioned before, leaf attribute is specifically
> > > > > > > > > > > > intended for library functions and I think all the
> > > > > > > > > > > > existing usage of leaf attribute is in the library
> > > > > > > > > > > > function declarations. For this reason, I think we do
> > > > > > > > > > > > not need to support them for lambdas. Is that
> > > > > > > > > > > > reasonable?
> > > > > > > > > > > >
> > > > > > > > > > > > For this reason, I think we do not need to support them
> > > > > > > > > > > > for lambdas. Is that reasonable?
> > > > > > > > > > >
> > > > > > > > > > > Is this considered a library function?
> > > > > > > > > > >
> > > > > > > > > > > ```
> > > > > > > > > > > struct S {
> > > > > > > > > > > void f(); // Is this a library function?
> > > > > > > > > > > void operator()(); // How about this?
> > > > > > > > > > > };
> > > > > > > > > > > ```
> > > > > > > > > > > If the answer is "no", then I think we only need to
> > > > > > > > > > > support `FunctionDecl` and nothing else (not even
> > > > > > > > > > > `ObjCMethodDecl`, which is like a member function for
> > > > > > > > > > > ObjC). If the answer is "yes", then it's not clear to me
> > > > > > > > > > > whether lambdas should or should not be supported given
> > > > > > > > > > > that the attribute on the lambda expression is attached
> > > > > > > > > > > to the function call operator for the lambda declaration.
> > > > > > > > > > > If the answer is "no", then I think we only need to
> > > > > > > > > > > support `FunctionDecl` and nothing else (not even
> > > > > > > > > > > `ObjCMethodDecl`, which is like a member function for
> > > > > > > > > > > ObjC). If the answer is "yes", then it's not clear to me
> > > > > > > > > > > whether lambdas should or should not be supported given
> > > > > > > > > > > that the attribute on the lambda expression is attached
> > > > > > > > > > > to the function call operator for the lambda declaration.
> > > > > > > > > >
> > > > > > > > > > I see your point @aaron.ballman. I would say the second one
> > > > > > > > > > is not really a library function.
> > > > > > > > > > @jdoerfert also suggested to allow leaf attribute only on
> > > > > > > > > > declarations.
> > > > > > > > > > I can add FunctionDecl, so we only allow leaf attribute on
> > > > > > > > > > function declarations, not on function definitions or
> > > > > > > > > > member functions.
> > > > > > > > > > Does that sound good to both of you?
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I see your point @aaron.ballman. I would say the second one
> > > > > > > > > > is not really a library function.
> > > > > > > > >
> > > > > > > > > I feel like either they both are or they both aren't, but
> > > > > > > > > it's a question of how this attribute is intended to be used.
> > > > > > > > >
> > > > > > > > > > @jdoerfert also suggested to allow leaf attribute only on
> > > > > > > > > > declarations.
> > > > > > > > > > I can add FunctionDecl, so we only allow leaf attribute on
> > > > > > > > > > function declarations, not on function definitions or
> > > > > > > > > > member functions.
> > > > > > > > > > Does that sound good to both of you?
> > > > > > > > >
> > > > > > > > > I've come around to that approach, but `FunctionDecl`
> > > > > > > > > represents any declaration of a function, including a
> > > > > > > > > definition. So you'll probably want to add a new
> > > > > > > > > `SubsetSubject` in `Attr.td` to represent a function
> > > > > > > > > declaration that's not a definition (and we could potentially
> > > > > > > > > reuse that subject for a few other attributes that can't be
> > > > > > > > > written on a definition). You can use
> > > > > > > > > `FunctionDecl::isThisDeclarationADefinition()` to distinguish
> > > > > > > > > between declarations and definitions.
> > > > > > > > > I feel like either they both are or they both aren't, but
> > > > > > > > > it's a question of how this attribute is intended to be used.
> > > > > > > > Sorry for being vague, and please let me try to clarify that.
> > > > > > > > What I meant was that existing leaf attribute cases are not
> > > > > > > > like the cases that you provided.
> > > > > > > > It is used in library function declarations in Fuchsia and
> > > > > > > > other existing use cases.
> > > > > > > > Are we ok banning this attribute in function definitions in
> > > > > > > > clang even though this behavior is different than other
> > > > > > > > compilers (GCC, ICC, etc.)?
> > > > > > > > If yes, I will incorporate the changes that you are suggesting.
> > > > > > > >
> > > > > > > >
> > > > > > > > Sorry for being vague, and please let me try to clarify that.
> > > > > > > > What I meant was that existing leaf attribute cases are not
> > > > > > > > like the cases that you provided.
> > > > > > > > It is used in library function declarations in Fuchsia and
> > > > > > > > other existing use cases.
> > > > > > >
> > > > > > > Okay, I think I'm on the same page as you now -- this attribute
> > > > > > > is most frequently written on free functions (ones that are not
> > > > > > > class members). However, I don't see a reason to disallow the
> > > > > > > attribute on a class member function though, or am I
> > > > > > > misunderstanding something? (GCC and ICC both seem to allow it on
> > > > > > > a class member function.)
> > > > > > >
> > > > > > > > Are we ok banning this attribute in function definitions in
> > > > > > > > clang even though this behavior is different than other
> > > > > > > > compilers (GCC, ICC, etc.)?
> > > > > > >
> > > > > > > I don't think it's okay to *ban* use of this attribute on
> > > > > > > function definitions (e.g., we shouldn't reject the user's code)
> > > > > > > because that will make porting code more difficult, but I think
> > > > > > > diagnosing as a warning is reasonable..
> > > > > > >
> > > > > > > This is what I think should happen: Let's drop the support for
> > > > > > > `ObjCMethodDecl` as that support can be added later if we find
> > > > > > > use cases that need it (this will make CodeGen easier in this
> > > > > > > patch).
> > > > > > >
> > > > > > > Let's use a custom subject so that the attribute can only be
> > > > > > > written on a function declaration (which will automatically
> > > > > > > include member functions) but continue to not pass `ErrorDiag` in
> > > > > > > the `SubjectList` (so that we continue to warn rather than err if
> > > > > > > the subject is a function definition).
> > > > > > >
> > > > > > > Let's not support blocks or lambdas unless a user comes up with
> > > > > > > use cases for it, but let's add tests to ensure that the behavior
> > > > > > > of the attribute on those is not harmful since the implicit
> > > > > > > methods generated for them may be a bit strange. For instance,
> > > > > > > the `alias` attribute cannot be written on a definition and yet:
> > > > > > > https://godbolt.org/z/vbbxKj To be clear -- I think the default
> > > > > > > behavior you get from the suggested `SubjectList` changes will be
> > > > > > > fine, but if it turns out that adding this attribute on a
> > > > > > > definition through a lambda causes harm (UB, crashes, etc) then
> > > > > > > we may have to put in extra effort to explicitly disallow it
> > > > > > > there.
> > > > > > >
> > > > > > > And then add plenty of Sema tests for all of this so we're
> > > > > > > explicitly testing the behaviors we care about.
> > > > > > >
> > > > > > > Does that sound reasonable to you?
> > > > > > > Okay, I think I'm on the same page as you now -- this attribute
> > > > > > > is most frequently written on free functions (ones that are not
> > > > > > > class members). However, I don't see a reason to disallow the
> > > > > > > attribute on a class member function though, or am I
> > > > > > > misunderstanding something? (GCC and ICC both seem to allow it on
> > > > > > > a class member function.)
> > > > > > Your understanding is right. Technically, leaf attributes should be
> > > > > > able to be used in methods as well.
> > > > > > However, I'm not aware of such existing cases.
> > > > > > As you suggested, I think we can extend leaf attribute support to
> > > > > > methods and lambdas if we encounter such cases later.
> > > > > >
> > > > > > > Does that sound reasonable to you?
> > > > > > It sounds great! I agree with the plan, and I'll upload the changes
> > > > > > in that direction.
> > > > > >
> > > > > @aaron.ballman I just added a simple rule for function declarations
> > > > > only.
> > > > > ```
> > > > > def FunctionDeclOnly : SubsetSubject<Function,
> > > > > [{!S->isThisDeclarationADefinition()}],
> > > > > "function declaration only">;
> > > > > ```
> > > > >
> > > > > I used that one in the leaf attribute definition:
> > > > > ```
> > > > > def Leaf : InheritableAttr {
> > > > > let Spellings = [GCC<"leaf">];
> > > > > let Subjects = SubjectList<[FunctionDeclOnly]>;
> > > > > let Documentation = [LeafDocs];
> > > > > let SimpleHandler = 1;
> > > > > }
> > > > > ```
> > > > >
> > > > > I thought that this will be straightforward, but after testing it on
> > > > > the following definition, surprisingly I did not get a warning.
> > > > > I was expecting to get `function declaration only` warning.
> > > > > ```
> > > > > __attribute__((leaf)) void f()
> > > > > {
> > > > > }
> > > > > ```
> > > > >
> > > > > After some debugging, I think this is what's happening:
> > > > > When we parse the function attributes, body is not parsed yet.
> > > > > As the following comment states in `isThisDeclarationADefinition`
> > > > > function, it returns false even for a definition.
> > > > >
> > > > > ```
> > > > > /// Note: the function declaration does not become a definition
> > > > > until the
> > > > > /// parser reaches the definition, if called before, this function
> > > > > will return
> > > > > /// `false`.
> > > > > ```
> > > > >
> > > > > Do you have any suggestions? Is there anything that I'm missing?
> > > > @aaron.ballman did you have a chance to take a look at my comment?
> > > Sorry about the delay in getting back to you on this (holiday schedule +
> > > C standards meetings got in the way of doing some reviews).
> > >
> > > Ugh, that's a good point -- we've not seen the function body by the time
> > > we're processing attributes, so we don't know whether to diagnose or not.
> > > I can think of two ways forward:
> > >
> > > 0) Not diagnose when the attribute is written on a function definition.
> > > 1) Add some code to Sema::ActOnFinishFunctionBody to diagnose if the
> > > attribute appears on the declaration. However, I'm not certain if there's
> > > an easy way to distinguish between an attribute on the definition and an
> > > attribute inherited from the definition. e.g.,
> > >
> > > ```
> > > [[gnu::leaf]] void func(void);
> > > void func(void) { } // hasAttr<LeafAttr> on this will return true
> > > ```
> > > I'm pretty sure that `Attr::isInherited()` reports whether the attribute
> > > should be inherited, not whether it actually has been inherited, so #1
> > > could be tricky.
> > >
> > > Personally, I'm fine with #0 if it turns out that #1 is painful.
> > > @jdoerfert, are you okay with that?
> > > Personally, I'm fine with #0 if it turns out that #1 is painful.
> > > @jdoerfert, are you okay with that?
> > Would that be ok if we land this patch without diagnosing a warning, and
> > work on a diagnosis patch later?
> >
> >
> I'd be fine with that!
> I'd be fine with that!
@aaron.ballman would you please approve the patch so I can merge it to the
master then?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90275/new/
https://reviews.llvm.org/D90275
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
