[PATCH] D137360: [Linker] Remove nocallback attribute while linking

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem marked an inline comment as done.
gulfem added inline comments.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1536
+/// nocallback attribute might call an imported function. When it's caller
+/// module and important function's module are linked together, the function
+/// with nocallback attribute now jumps back into its caller's module.
----------------
tejohnson wrote:
> gulfem wrote:
> > tejohnson wrote:
> > > gulfem wrote:
> > > > tejohnson wrote:
> > > > > Should "important" be "imported"?
> > > > > 
> > > > > Also, I'm not completely following the situation described in these sentences. Can you give me a more detailed example? I might be able to suggest clearer wording once I understand better.
> > > > This is the great example that @mysterymath came up with to explain the problem in the bug reported to `gcc` (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106725)
> > > > ```
> > > > ==> ext.c <==
> > > > void set_value(int v);
> > > > 
> > > > void external_call(void) {
> > > >   set_value(0);
> > > > }
> > > > 
> > > > ==> lto.c <==
> > > > static int value;
> > > > void set_value(int v) { value = v; }
> > > > int get_value(void) { return value; }
> > > > 
> > > > ==> main.c <==
> > > > #include <stdio.h>
> > > > 
> > > > void set_value(int v);
> > > > int get_value(void);
> > > > __attribute__((leaf)) void external_call(void);
> > > > 
> > > > int main(void) {
> > > >   set_value(42);
> > > >   external_call();
> > > >   printf("%d\n", get_value());
> > > > }
> > > > ```
> > > > 
> > > > This is the scenario that I was trying to summarize here, where a function with `nocallback` attribute returns to its caller's module other than a `return` or an `exception`. `external_call()` is the function that has the `leaf` attribute, and `main()` is its caller. Normally, `external_call()` returns to its caller's module via a `return`. When `lto.c` and `main.c` are linked together, `external_call()` calls an imported function(`set_value()`), which is now in the same module with its caller. So, it returned to its caller's module via a function call, not with a return or an exception.
> > > Ok, thanks. I understand this better after looking at the example here and in that bug. Per the bug, the issue comes in when ext.c is compiled to native code, and only main.c and lto.c are LTO-linked (merged).
> > > 
> > > What would happen if all 3 of these were LTO-linked (merged)? Would there still be an issue? I.e. if the definition of external_call was linked into the same module as the call and the declaration that was marked leaf (which I guess turns into "nocallback" internally). In that case you would have one module that would not be calling out at all, so it isn't clear to me that nocallback would be wrong.
> > > 
> > > Because the current code is stripping the nocallback attribute in all situations, it isn't clear to me whether it needs to or whether this is overly conservative. Essentially, you will never end up with a nocallback attribute on any LTO'ed code.
> > > 
> > > Also, the usage of the word "imported" is a little confusing, because in LLVM we tend to use this terminology for ThinLTO, where definitions may be imported into other modules (i.e. we aren't merging the full modules like in regular LTO). In this case, the imported definitions get available_externally linkage, and thus have their definitions dropped after inlining (because the original definition is kept in the original module). In that case, I think we wouldn't need to drop the nocallback attribute, so I think in the ThinLTO case this may be a little overly conservative.
> > When all  three modules are LTO-linked, leaf attribute is not going to have any effect based on the `GCC` description ("The attribute has no effect on functions defined within the current compilation unit").
> > https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html
> > 
> > You are right that we are being overly conservative about the usage of `leaf` attribute, and we plan to drop it for any LTO-ed code. The issue is that there was a lot of disagreement on the semantics of the `leaf` attribute for `LTO` usage (https://reviews.llvm.org/D131628), and we were not able to get a clear answer from the `GCC` community. Therefore, we decided to go with the restricted semantics. 
> > 
> Thanks for the clarification. Can you add the note about it having no effect on functions defined within the unit to the description in D131628?
> 
> Revisiting the example you included, I would suggest something like the following wording (keep your original 2 sentences but modify the example description):
> 
> For example, the nocallback function may contain a call to another module. But if we merge its caller and callee module here, and not the module containing the nocallback function definition itself, the nocallback property will be violated (since the nocallback function will call back into the newly merged module containing both its caller and callee). This could happen if the module containing the nocallback function definition is native code, so it does not participate in the LTO link. Note if the nocallback function does participate in the LTO link, and thus ends up in the merged module containing its caller and callee, removing the attribute doesn't hurt as it has no effect on definitions in the same compilation unit.
> 
I incorporated your comment in the latest revision, and extended the description in D131628.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137360