gulfem 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:
> 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?
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
