aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1435
+ let Spellings = [GCC<"leaf">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
----------------
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?
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
