Page MenuHomePhabricator

[clang][IR] Add support for leaf attribute
Needs ReviewPublic

Authored by gulfem on Tue, Oct 27, 5:21 PM.

Details

Summary

GCC supports attribute((leaf)) as an optimization hint described in:
https://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/Function-Attributes.html
This patch adds support for this attribute in Clang/LLVM.

Diff Detail

Unit TestsFailed

TimeTest
60 mslinux > Clang.Misc::pragma-attribute-supported-attributes-list.test
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang-tblgen -gen-clang-test-pragma-attribute-supported-attributes -I/mnt/disks/ssd0/agent/llvm-project/build/tools/clang/test/../../../../clang/include /mnt/disks/ssd0/agent/llvm-project/build/tools/clang/test/../../../../clang/include/clang/Basic/Attr.td -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/Misc/pragma-attribute-supported-attributes-list.test
360 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
50 mswindows > Clang.Misc::pragma-attribute-supported-attributes-list.test
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang-tblgen.exe -gen-clang-test-pragma-attribute-supported-attributes -Ic:\ws\w64\llvm-project\premerge-checks\build\tools\clang\test\..\..\..\..\clang/include c:\ws\w64\llvm-project\premerge-checks\build\tools\clang\test\..\..\..\..\clang/include/clang/Basic/Attr.td -o - | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\clang\test\Misc\pragma-attribute-supported-attributes-list.test
190 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w64\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w64\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w64\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

Event Timeline

gulfem created this revision.Tue, Oct 27, 5:21 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
gulfem requested review of this revision.Tue, Oct 27, 5:21 PM

Tests missing (on all levels), was there an RFC? What is the expected usage of this?

(To clarify, I am not opposing this, my comment before was brief but not meant to be negative. I'm generally for new attributes and this one is already in existing source so that is even better.)

There is an RFC going out with this prototype as reference. When there is consensus on the RFC, this will get in shape for landing with complete tests and all.

There is an RFC going out with this prototype as reference. When there is consensus on the RFC, this will get in shape for landing with complete tests and all.

Great. I didn't find anything when I looked for leaf in my cfe-dev inbox. Could someone put a link here, maybe directly into the commit message as it will be useful even after this goes in. Thanks :)

gulfem added a comment.EditedTue, Oct 27, 5:41 PM

There is an RFC going out with this prototype as reference. When there is consensus on the RFC, this will get in shape for landing with complete tests and all.

Great. I didn't find anything when I looked for leaf in my cfe-dev inbox. Could someone put a link here, maybe directly into the commit message as it will be useful even after this goes in. Thanks :)

I just sent the RFC with the subject line [RFC] Leaf Attribute Support in Clang/LLVM.

There is an RFC going out with this prototype as reference. When there is consensus on the RFC, this will get in shape for landing with complete tests and all.

Great. I didn't find anything when I looked for leaf in my cfe-dev inbox. Could someone put a link here, maybe directly into the commit message as it will be useful even after this goes in. Thanks :)

I just sent the RFC.

Yep, I'm writing my response there ;)

FWIW, http://lists.llvm.org/pipermail/llvm-dev/2020-October/146113.html

I'd recommend splitting this into two patches, one for LLVM changes and another for Clang changes.

gulfem updated this revision to Diff 301787.Thu, Oct 29, 4:39 PM

Add IR and bitcode tests

gulfem updated this revision to Diff 301789.Thu, Oct 29, 4:52 PM

Fix the typo in the test case

aaron.ballman added inline comments.Mon, Nov 2, 7:02 AM
clang/include/clang/Basic/Attr.td
1435

Should this attribute also be supported on things like ObjC method decls or other function-like interfaces?

1436

This should be referring to LeafDocs rather than undocumented.

clang/include/clang/Basic/AttrDocs.td
3909

as leaf -> with the leaf

I'm not certain how to interpret that functions are not allowed to enter their caller's translation unit. I sort of read that as leaf functions are not allowed to call (or otherwise jump) out of the translation unit in which they're defined -- is that about right?

3910

What sort of diagnostic checking should the user expect? e.g., can the user expect Clang to diagnose obviously incorrect code like:

[[gnu::leaf]] void func(void (*fp)(void)) {
  fp(); // This seems like a bad thing
}
3913

We should probably link to the latest docs (https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html) rather than 4.7.2.

gulfem updated this revision to Diff 302349.Mon, Nov 2, 11:10 AM
gulfem marked 3 inline comments as done.

Addressed the comments about the documentation

gulfem marked an inline comment as not done.Mon, Nov 2, 11:11 AM
gulfem added inline comments.
clang/include/clang/Basic/AttrDocs.td
3909

I think the description is a little confusing.
As far as I understand, leaf functions can actually call or jump out the the translation unit that they are defined ("Leaf functions might still call functions from other compilation units").
The manual refers caller function's translation unit as current translation unit.
"Calls to external functions with this attribute must return to the current compilation unit only by return or by exception handling. In particular, a leaf function is not allowed to invoke callback functions passed to it from the current compilation unit, directly call functions exported by the unit, or longjmp into the unit."
My interpretation of this statement is that a function marked with a leaf attribute can only return to its caller translation unit by a return or an exception, but it cannot enter into callers translation unit by invoking a callback function.
Does that make sense?

aaron.ballman added inline comments.Mon, Nov 2, 11:20 AM
clang/include/clang/Basic/AttrDocs.td
3909

Ah, thank you for the explanation! How about: Functions marked with the ''leaf'' attribute are not allowed to jump back into the caller's translation unit, whether through invoking a callback function, a direct external function call, use of 'longjmp', or other means.?

Is this property transitive? e.g.,

// TU1.c
void func(void) {
  leaf();
}

void bar(void) {}

// TU2.c
__attribute__((leaf)) void leaf(void) {
  baz(); // Is this not allowed?
}

// TU3.c
void baz(void) {
  bar();
}

The "directly call functions exported by the unit" makes me think the above code is fine but it could also be that "direct" in this case means "through a function designator rather than a function pointer".

gulfem added inline comments.Mon, Nov 2, 11:25 AM
clang/include/clang/Basic/Attr.td
1435

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.

aaron.ballman added inline comments.Mon, Nov 2, 11:31 AM
clang/include/clang/Basic/Attr.td
1435

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.

gulfem updated this revision to Diff 302369.Mon, Nov 2, 12:23 PM

Update the attribute documentation

gulfem added inline comments.Mon, Nov 2, 1:38 PM
clang/include/clang/Basic/AttrDocs.td
3910

This is a compiler hint provided by the user, and if the user misuses that attribute, it might result in undefined behavior.
I think it might be difficult to fully verify that leaf function does not enter into caller's translation unit.
For this simple case you provided, it might be easy to add such a check.
However, it might be difficult to add such checks for complicated cases.
Therefore, we were not planning to add such kind of diagnostic checking.
What do you think?

3910

I think this property is transitive.
If a leaf function somehow enters into caller's translation unit (either via direct call or a via its call chain), it will violate the rule that says "Calls to external functions with this attribute must return to the current compilation unit only by return or by exception handling".
Entering via its call chain is not a return or an exception handling.
Do you agree with that?

jdoerfert added inline comments.Mon, Nov 2, 2:03 PM
clang/include/clang/Basic/AttrDocs.td
3910

@aaron.ballman, your code example is not " obviously incorrect " and we should not diagnose that (see below). In fact, I doubt we can ever diagnose misuse except during the LTO linking stage, assuming the expected use case. That said, you could warn and ignore if a function definition is annotated.

// library.c
void bar(void) {}

[[gnu::leaf]] void func(void (*fp)(void)) {
  fp(); // not necessarily a bad thing
}
// user.c
#include <library.h>
void test() {
  func(&bar); // perfectly fine.
}
gulfem marked an inline comment as done.Mon, Nov 2, 5:28 PM
gulfem added inline comments.
clang/include/clang/Basic/AttrDocs.td
3909

I like your description, and I changed the doc that incorporates your feedback.

aaron.ballman added inline comments.Tue, Nov 3, 6:04 AM
clang/include/clang/Basic/AttrDocs.td
3910

@aaron.ballman, your code example is not " obviously incorrect " and we should not diagnose that (see below).

Oh, thank you for that example!

That said, you could warn and ignore if a function definition is annotated.

Why would you want to warn and ignore in that situation?

3910

I think this property is transitive. ... Do you agree with that?

That makes sense to me! I think I'd recommend a slight modification to the docs then:

Functions marked with the ``leaf`` attribute are not allowed to jump back into the caller's translation unit, whether through invoking a callback function, a direct, possibly transitive, external function call, use of ``longjmp``, or other means.

The last question is: by "direct" does the GCC docs imply that calls back to the caller's TU through a function *pointer* are not UB? I'd imagine those would also be bad, but I'd like to make sure (and perhaps we can remove the direct from the wording if calls through function pointers are disallowed).

The more I think about it, the more I think we should never create a leaf/nocallback definition. Only declarations should carry that attribute.

I'm also still not convinced nocallback is a good name. @efriedma @aqjune @fhahn @reames What are your thoughts on the name. My earlier idea was inaccesiblecodeonly, as it matches the inaccisblememonly idea. I'm not married to it, nocallback is just not great IMHO as direct calls back into the caller TU are also forbidden and there is already !callback.

clang/include/clang/Basic/AttrDocs.td
3910
That said, you could warn and ignore if a function definition is annotated.

Why would you want to warn and ignore in that situation?

Let me try to explain, sorry I didn't earlier.

GCC says (emphasis added):

Calls to *external* functions with this attribute
...
The attribute has *no effect on functions defined* within the current compilation unit. This is to allow easy merging of multiple compilation units into one, for example, by using the link-time optimization. For this reason the attribute is not allowed on types to annotate indirect calls.

The thing is, it doesn't make sense to have a leaf definition. Leaf functions can always call stuff "in their own TU".
Let's assume we have a leaf function definition in a TU and it is a single TU, all functions can be (=might be allowed to be) called by the leaf function, even external ones.
If it is a TU linked from multiple ones, we don't know anymore what came from where so there is no point in keeping leaf on a definition as the call sites all look the same.

The more I think about it, the more I think we should never create a leaf/nocallback definition. Only declarations should carry that attribute.

When talking about the IR that gets lowered to LLVM, I think that's very reasonable. When talking about the frontend, I'm a bit less certain. GCC and ICC both do not warn when you apply the attribute solely to a definition, so one concern is with code portability where existing code is harder to port into Clang. However, when I tried to find examples of the attribute being written on a definition, I couldn't spot any so this may not be a particularly large concern.

Another potential concern is that a situation like this seems harmless and diagnosing the use on the definition seems user-unfriendly:

// In a header file
__attribute__((leaf)) void func(void);

// In a source file
__attribute__((leaf)) void func(void) { ... }

The last concern is whether non-compiler-writing users will understand the behavior.

// #1
__attribute__((leaf)) void func(void) {} // Declares and defines a function at the same time

// #2
__attribute__((leaf)) void func(void);
void func(void) {} // Inherits the attribute from the previous declaration

I would suspect that users will see #1 and #2 as defining the same things and may be surprised if #1 is diagnosed but #2 is fine, but that's just a gut feeling without evidence backing it up.

I'm not insisting that we accept the attribute on a definition, but if we do want to make a distinction in the frontend between declaration and definition, we should document it with some explicit examples.

aqjune added a comment.EditedTue, Nov 3, 11:05 AM

Hi,

Naming is a hard thing... I have no special preference. :/

However, I'd like to understand the details of this attribute.

Would LTO be affected because leaf is guaranteed to untouch the current translation unit only?

// a.c
int x;
void f1() {
  f2();
}
void g() { x = 3; }

// b.c
void f2() {
  leaf();
}

// leaf.c
attribute((leaf)) void leaf() {
  g();
}

IIUC this program is okay because g() and the caller of leaf() are in different TUs.
But, let's assume that a.c and b.c are LTO-ed, and leaf.c is separately compiled.
If LTO merges a.c and b.c into the same module, the two TUs cannot be distinguished anymore; either leaf should be dropped, or LTO should somehow conceptually keep two TUs.
Would it be a valid concern? Then I think it should be mentioned.

Another question is more about the motivation of this attribute (well, I know it is introduced by gcc first; just throwing a question :) )
If the motivation is to support better data flow analysis, is there something special in callback itself?
The gcc document states that sin() is a leaf function, and IIUC this is because sin() never touches the memory allocated at caller's TU (because errno isn't at the caller's TU).
I think things are easier if we simply say that leaf cannot touch the memory of current TU, regardless of the existence of callbacks.
Is there something important in the callback itself?

Hi,

Naming is a hard thing... I have no special preference. :/

However, I'd like to understand the details of this attribute.

Would LTO be affected because leaf is guaranteed to untouch the current translation unit only?

// a.c
int x;
void f1() {
  f2();
}
void g() { x = 3; }

// b.c
void f2() {
  leaf();
}

// leaf.c
attribute((leaf)) void leaf() {
  g();
}

IIUC this program is okay because g() and the caller of leaf() are in different TUs.
But, let's assume that a.c and b.c are LTO-ed, and leaf.c is separately compiled.
If LTO merges a.c and b.c into the same module, the two TUs cannot be distinguished anymore; either leaf should be dropped, or LTO should somehow conceptually keep two TUs.
Would it be a valid concern? Then I think it should be mentioned.

As noted by the GCC docs, it doesn't mean anything on a definition so that you can safely merge TUs. I want us to forbid leaf on IR function definitions for that reason, it would not mean anything and be only confusing.

Another question is more about the motivation of this attribute (well, I know it is introduced by gcc first; just throwing a question :) )
If the motivation is to support better data flow analysis, is there something special in callback itself?
The gcc document states that sin() is a leaf function, and IIUC this is because sin() never touches the memory allocated at caller's TU (because errno isn't at the caller's TU).

No, that is not it. It is leaf because it will not transfer control to the callers TU.

I think things are easier if we simply say that leaf cannot touch the memory of current TU, regardless of the existence of callbacks.
Is there something important in the callback itself?

Not really, IMHO, leaf means you cannot transfer control to the callers TU without going threw the original call site (via return or throw).
It is not a memory thing. However, the "almost" matching memory property is called inaccesiblememonly so that is why I wanted to call this inaccessiblecodeonly.

gulfem marked an inline comment as done.EditedTue, Nov 3, 12:20 PM

The more I think about it, the more I think we should never create a leaf/nocallback definition. Only declarations should carry that attribute.

I'm also still not convinced nocallback is a good name. @efriedma @aqjune @fhahn @reames What are your thoughts on the name. My earlier idea was inaccesiblecodeonly, as it matches the inaccisblememonly idea. I'm not married to it, nocallback is just not great IMHO as direct calls back into the caller TU are also forbidden and there is already !callback.

We are totally fine to rename it to something else, and we do see the potential confusion with the existing callback attribute that we have.
If I understand correctly, inaccessiblememonly functions may only access memory that is not accessible by the current compilation unit (current compilation unit means the compilation unit that inaccessiblememonly function is defined).
If we use inaccesiblecodeonly, it implies that leaf functions may only access code that is not accessible by the compilation unit that they are defined.
But, leaf attribute does not enforce any rule for accessing code in the leaf attribute function's compilation.
It only enforces that leaf function does not access any code in its caller's translation unit.
That's is why I'm not sure whether inaccesiblecodeonly is a good name for that.
Please let me know if I'm missing anything about inaccessiblememonly attribute.
@jdoerfert what do you think about that?

gulfem updated this revision to Diff 302716.Tue, Nov 3, 5:01 PM

Add a target into the test case

gulfem added inline comments.Tue, Nov 3, 5:26 PM
clang/include/clang/Basic/AttrDocs.td
3910

I think the idea is that the control-flow should never come back to the caller's translation unit by any kind of control-flow changing mechanism (direct/indirect and call/jump).
The compiler might use this hint to do optimizations based on that assumption, so the user should ensure that.
Could you please explain what do you mean by calling back via function pointer?
I thought that callback function is basically calling back via a function pointer.

The GCC documentation specifically gives the example of the standard C function qsort as one that does not qualify as __attribute__((leaf)) because it uses a callback function (that presumably might be from the caller's own TU). AIUI the claim the attribute makes is that no normal control flow (i.e. excluding only signal handlers) would call any function in the caller's TU by any means, nor longjmp to any state captured by a setjmp call in the caller's TU, and thus only normal return or normal C++ exception handling return/unwind would reach the caller's TU again after the jump (call) to this entry point. The caller is thus presumed not to share potentially-called functions with the callee by any means, whether direct symbol references or function pointers passed in the call or function pointers previously stored somewhere (e.g. a C++ vtable). The compiler can make whatever assumptions about potential dataflow/side-effect and the like are implied by this exclusion of TU-local code paths from the otherwise unknown call graph of the external call.

aqjune added a comment.Wed, Nov 4, 5:47 AM

As noted by the GCC docs, it doesn't mean anything on a definition so that you can safely merge TUs. I want us to forbid leaf on IR function definitions for that reason, it would not mean anything and be only confusing.

Okay, I see.
I agree that having this attribute at definitions is slightly dangerous..!

But I am not still 100% sure about the safety of merging... I see that it is okay when leaf is on a definition, but what about declaration?

// a.ll
define void f1() { f2(); }
define void g() { x = 3; }
// b.ll
define void f2() { leaf(); }
declare leaf void @leaf() ; If @leaf() was actually calling @g(), is merging a.ll and b.ll valid?

It is not a memory thing. However, the "almost" matching memory property is called inaccesiblememonly so that is why I wanted to call this inaccessiblecodeonly.

Eh, actually my question was more like a question to gcc people about why leaf was defined that way. Maybe my question is not relevant in this thread. Thank you for answering though.

gulfem added inline comments.Wed, Nov 4, 10:50 AM
clang/include/clang/Basic/Attr.td
1435

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?

gulfem added a comment.Wed, Nov 4, 1:18 PM

The more I think about it, the more I think we should never create a leaf/nocallback definition. Only declarations should carry that attribute.

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 ex, it is only used in syscalls in Fuchsia.
Therefore, I'm not sure whether it is really necessary to ban leaf attribute in function definitions.
Even though function attributes are typically intended to be used in the function declaration, compilers do not have policy to forbid using them in the function definition.

aqjune added a comment.Wed, Nov 4, 8:45 PM

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 ex, it is only used in syscalls in Fuchsia.
Therefore, I'm not sure whether it is really necessary to ban leaf attribute in function definitions.
Even though function attributes are typically intended to be used in the function declaration, compilers do not have policy to forbid using them in the function definition.

Maybe it's because the leaf (or nocallback or whatever) attribute at the function definition in IR can't be used for any optimization?
For the callers, leaf attribute at the function declarations is already giving the information.
But the attribute at function definitions really give no additional information inside the TU, so it's redundant.

aaron.ballman added inline comments.Thu, Nov 5, 5:19 AM
clang/include/clang/Basic/Attr.td
1435

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();
}
clang/include/clang/Basic/AttrDocs.td
3910

Could you please explain what do you mean by calling back via function pointer?
I thought that callback function is basically calling back via a function pointer.

I had confused myself -- I think @jdoerfert answered my confusion in an earlier comment and I just wasn't thinking about that case hard enough. I think we want to drop the word "direct" from "external function call" because it doesn't matter whether the user says:

void bar(void); // Assume calling this is bad
bar();

and

void bar(void); // Assume calling this is bad
void (*fp)(void) = bar;
fp();
gulfem updated this revision to Diff 303200.Thu, Nov 5, 11:22 AM

Remove direct from description and ObjCMethod for Obj functionality

gulfem marked an inline comment as done.Thu, Nov 5, 11:38 AM
gulfem added inline comments.
clang/include/clang/Basic/Attr.td
1435

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?

clang/include/clang/Basic/AttrDocs.td
3910

Could you please explain what do you mean by calling back via function pointer?
I thought that callback function is basically calling back via a function pointer.

I had confused myself -- I think @jdoerfert answered my confusion in an earlier comment and I just wasn't thinking about that case hard enough. I think we want to drop the word "direct" from "external function call" because it doesn't matter whether the user says:

void bar(void); // Assume calling this is bad
bar();

and

void bar(void); // Assume calling this is bad
void (*fp)(void) = bar;
fp();
3910

I removed the direct from the documentation.

aaron.ballman added inline comments.Thu, Nov 5, 12:11 PM
clang/include/clang/Basic/Attr.td
1435

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.

gulfem marked an inline comment as done.Mon, Nov 9, 6:08 PM
gulfem added inline comments.
clang/include/clang/Basic/Attr.td
1435

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?

aaron.ballman added inline comments.Tue, Nov 10, 7:34 AM
clang/include/clang/Basic/Attr.td
1435

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.

gulfem added inline comments.Tue, Nov 10, 5:33 PM
clang/include/clang/Basic/Attr.td
1435

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.

aaron.ballman added inline comments.Wed, Nov 11, 5:24 AM
clang/include/clang/Basic/Attr.td
1435

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?

gulfem added inline comments.Wed, Nov 11, 8:29 PM
clang/include/clang/Basic/Attr.td
1435

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.

gulfem added inline comments.Wed, Nov 18, 6:11 PM
clang/include/clang/Basic/Attr.td
1435

@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?