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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3910
+in library functions. 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.
----------------
gulfem wrote:
> 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?
> 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).


================
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.
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275