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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added subscribers: aqjune, fhahn, efriedma, reames.
jdoerfert added a comment.

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



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3910
+Functions marked as ``leaf`` attribute are not allowed to enter their caller's translation unit.
+Therefore, they cannot use or modify any data that does not escape the current compilation unit.
+
----------------
aaron.ballman wrote:
> jdoerfert wrote:
> > gulfem wrote:
> > > aaron.ballman wrote:
> > > > 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
> > > > }
> > > > ```
> > > 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?
> > @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.
> > }
> > 
> > ```
> > @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?
> >    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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275