[PATCH] D90275: [clang][IR] Add support for leaf attribute

Wed Nov 11 20:29:15 PST 2020

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:
> > > > 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90275/new/

https://reviews.llvm.org/D90275