[PATCH] D90275: [clang][IR] Add support for leaf attribute
Gulfem Savrun Yeniceri via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 7 12:50:59 PST 2020
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];
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90275/new/
https://reviews.llvm.org/D90275
More information about the cfe-commits
mailing list