gulfem marked an inline comment as not done. 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: > 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? 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