This is an archive of the discontinued LLVM Phabricator instance.

[Linker] Remove nocallback attribute while linking
ClosedPublic

Authored by gulfem on Nov 3 2022, 2:25 PM.

Details

Summary

GCC's leaf attribute is lowered to LLVM IR nocallback attribute.
Clang conservatively treats this function attribute as a hint on the
module level, and removes it while linking modules. More context can
be found in https://reviews.llvm.org/D131628.

Diff Detail

Event Timeline

gulfem created this revision.Nov 3 2022, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 2:25 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
gulfem requested review of this revision.Nov 3 2022, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 2:25 PM
mysterymath added inline comments.Nov 3 2022, 3:13 PM
llvm/lib/Linker/LinkModules.cpp
546

We'll also need to do this with inaccessiblememonly, inaccessiblemem_or_argmemonly, and the inaccessiblemem option for the memory() attribute.

mysterymath added inline comments.Nov 3 2022, 3:26 PM
llvm/lib/Linker/LinkModules.cpp
546

Scratch this, it looks like the FunctionAttrs pass rewrites these based on the results of the bitcode link.

I somewhat assume we could remove the attribute whenever we actually move a function (lazy linking) rather than for all in the module.
Other than that I'm happy with dropping nocallback *and* inacc* (see https://reviews.llvm.org/D131628#3724307 for the latter).

llvm/lib/Linker/LinkModules.cpp
546

We cannot rely on a pass to clean things up. We need to drop inacc* here too, as shown here: https://reviews.llvm.org/D131628#3724307

This change needs a test case. It's usually best to just pre-commit the test case and rebase this change on top of it, so that the effect on the newly added test becomes clear. You normally don't need to open a Phabricator review for the added test.

gulfem updated this revision to Diff 473378.Nov 4 2022, 5:12 PM
gulfem marked an inline comment as not done.

Added a test case and addressed feedback

gulfem added a comment.Nov 4 2022, 5:29 PM

This change needs a test case. It's usually best to just pre-commit the test case and rebase this change on top of it, so that the effect on the newly added test becomes clear. You normally don't need to open a Phabricator review for the added test.

Done!

I somewhat assume we could remove the attribute whenever we actually move a function (lazy linking) rather than for all in the module.
Other than that I'm happy with dropping nocallback *and* inacc* (see https://reviews.llvm.org/D131628#3724307 for the latter).

There is a recent change that drops argmemonly, inaccessiblememonly and inaccessiblemem_or_argmemonly attributes (https://reviews.llvm.org/D135780). So, we do not need to apply this change to them anymore.

gulfem added a comment.Nov 4 2022, 5:50 PM

I somewhat assume we could remove the attribute whenever we actually move a function (lazy linking) rather than for all in the module.

Done!

arichardson added inline comments.
llvm/test/Linker/drop-attribute.ll
14

Checking the whole line should allow omitting the other two CHECK-NOT lines

nikic added a subscriber: nikic.Nov 5 2022, 1:49 AM

I don't think this implementation is correct, but I'm also not entirely sure in which cases you want to drop the attribute and in which you want to retain it. I'd suggest to have the following test cases:

  • A nocallback intrinsic, where the attribute should never be dropped.
  • A nocallback definition.
  • A nocallback declaration with a definition that is being linked in.
  • A nocallback declaration without a definition being linked in.

Additionally, how does this interact with inference? Say we originally had a call to a nocallback declaration in a function, then inference determines that that function is nocallback as well (that would be legal, right?) In that case we might have to clear that nocallback attribute as well. So I think at point we have to drop all (non-intrinsic) nocallback attributes in the module?

mysterymath added a comment.EditedNov 7 2022, 9:41 AM

I don't think this implementation is correct, but I'm also not entirely sure in which cases you want to drop the attribute and in which you want to retain it. I'd suggest to have the following test cases:

  • A nocallback intrinsic, where the attribute should never be dropped.

Is it safe to keep nocallback on intrinsics? It's legal to LTO-in the implementations of various compiler intrinsics, so it seems possible that a call to an intrinsic declared nocallback might be implemented via a call to a a function in the current LTO unit.

With the per-translation-unit semantics for nocallback, you'd only need to guarantee that you don't use the intrinsic in the translation unit that defines its implementation. With the LTO-unit semantics, you couldn't use the intrinsic anywhere in the whole program, which is impossibly restrictive, so instead, you'd be forced to keep the definition out of LTO. This scenario caused me to change my tune somewhat about the ideal semantics for nocallback.

I somewhat assume we could remove the attribute whenever we actually move a function (lazy linking) rather than for all in the module.
Other than that I'm happy with dropping nocallback *and* inacc* (see https://reviews.llvm.org/D131628#3724307 for the latter).

There is a recent change that drops argmemonly, inaccessiblememonly and inaccessiblemem_or_argmemonly attributes (https://reviews.llvm.org/D135780). So, we do not need to apply this change to them anymore.

But it just replaces inaccessiblememonly with memoru(inaccessiblememonly), right? It doesn't address the underlying problem, which is that the linking potentially invalidates memory(inaccessiblememonly).

I don't think this implementation is correct, but I'm also not entirely sure in which cases you want to drop the attribute and in which you want to retain it. I'd suggest to have the following test cases:

  • A nocallback intrinsic, where the attribute should never be dropped.
  • A nocallback definition.
  • A nocallback declaration with a definition that is being linked in.
  • A nocallback declaration without a definition being linked in.

Additionally, how does this interact with inference? Say we originally had a call to a nocallback declaration in a function, then inference determines that that function is nocallback as well (that would be legal, right?) In that case we might have to clear that nocallback attribute as well. So I think at point we have to drop all (non-intrinsic) nocallback attributes in the module?

Definitions should never be nocallback, IMHO. I thought the proposed lang-ref change explicitly mentioned that. Which leaves three cases above to be dealt with:
nocallback intrinsic and nocallback declaration w/ and w/o definition being linked in.
We should not touch intrinsics, here, as noted.
For the declaration case, we always need to drop it because it is either a definition now (for which nocallback is meaningless) or we might have imported code that is called by the (former) nocallback declaration.

gulfem updated this revision to Diff 482700.Dec 13 2022, 8:44 PM
  1. Drop memory(inaccessiblemem) attributes as well, and added test cases for them
  2. Added a test case for testing intrinsics
  3. Added a test case for a nocallback declaration without a definition being linked in
gulfem marked an inline comment as done.Dec 13 2022, 8:52 PM

I would like to make sure that we all agree on the following items:

  1. A nocallback intrinsic, where the attribute should never be dropped.

Why is the case? @nikic and @jdoerfert can you explain the reason behind this? Why should we drop nocallback for user declared functions, but keep it for intrinsics?
We need to clarify 1).

  1. A nocallback definition.

We are not going to allow nocallback on definitions, and I think we are all on the same page for 2.

  1. A nocallback declaration with a definition that is being linked in.
  2. A nocallback declaration without a definition being linked in.

Whether the definition is being linked in or not, we will drop the nocallback attribute. I think, we are all on the same page for 3 and 4 as well.

Drop memory(inaccessiblemem) attributes as well, and added test cases for them

I'd personally recommend to keep inaccessiblemem out of this patch. Yes, it has a similar issue as nocallback, but it's also a much higher impact change (fixing it properly basically means declaring that every single call can have a side-effect after linking) and that case definitely needs to deal with inference and cannot be limited to declarations. I think once we have a solution for nocallback, we should tackle inaccessiblemem as a second step.

A nocallback intrinsic, where the attribute should never be dropped.

Why is the case? @nikic and @jdoerfert can you explain the reason behind this? Why should we drop nocallback for user declared functions, but keep it for intrinsics?
We need to clarify 1).

Intrinsic attributes are ... intrinsic to the intrinsic :) They must be legal regardless of where and how the intrinsic is used. If the intrinsic would not be nocallback in some circumstance, it cannot be nocallback at all. (This doesn't apply to call-site attributes, which raises the question of whether nocallback can be used as a call-site attribute -- if it can, then your patch would have to clear the attribute at call-sites as well, not just declarations. If it can't, then we have no problem.)

A nocallback definition.

We are not going to allow nocallback on definitions, and I think we are all on the same page for 2.

I would be okay with this limitation, but if it exists, we need to enforce it. We need to add a check to the IR verifier that the attribute does not occur on definitions, and we need to make sure that Clang does not produce invalid IR. This means either diagnosing if the attribute would be placed on a definition or silently dropping it. (For the purposes of this patch, we probably don't strongly care about having the limitation -- we could also omit the declaration check and clear the attribute on all functions. I think the question of call-site attributes is more problematic.)

On that note, does someone have an example of how __attribute__((nocallback)) is getting used in the wild? Do people use the preprocessor to add the nocallback attribute in the header only for external users of the library, but omit it when including the header in the implementation (as no callback is likely not valid there?)

Whether the definition is being linked in or not, we will drop the nocallback attribute. I think, we are all on the same page for 3 and 4 as well.

Yes, I think this is right.

llvm/lib/Linker/LinkModules.cpp
390

I don't think we should include it in this patch, but if we do, this is not the right way: What you want to do is always add inaccessiblemem as an accessible location. You would have to do something like this:

F->setMemoryEffects(F->getMemoryEffects() | MemoryEffects::inaccessibleMemOnly());
392

You don't need to check hasFnAttribute() first.

On that note, does someone have an example of how __attribute__((nocallback)) is getting used in the wild? Do people use the preprocessor to add the nocallback attribute in the header only for external users of the library, but omit it when including the header in the implementation (as no callback is likely not valid there?)

That appears to be how glibc uses it: https://github.com/bminor/glibc/blob/d055481ce39d03652ac60de5078889e15b6917ff/misc/sys/cdefs.h#L64

gulfem updated this revision to Diff 483016.Dec 14 2022, 3:40 PM
  1. Remove dropping memory(inaccessiblemem) attribute
  2. Keep nocallback attribute in intrinsics
  3. Drop nocallback attribute on definitions
gulfem added a comment.EditedDec 14 2022, 3:54 PM

Drop memory(inaccessiblemem) attributes as well, and added test cases for them

I'd personally recommend to keep inaccessiblemem out of this patch. Yes, it has a similar issue as nocallback, but it's also a much higher impact change (fixing it properly basically means declaring that every single call can have a side-effect after linking) and that case definitely needs to deal with inference and cannot be limited to declarations. I think once we have a solution for nocallback, we should tackle inaccessiblemem as a second step.

I agree with that. Our first goal is to agree on the nocallback semantics, add the description into LLVM Language Reference Manual.

A nocallback intrinsic, where the attribute should never be dropped.

Why is the case? @nikic and @jdoerfert can you explain the reason behind this? Why should we drop nocallback for user declared functions, but keep it for intrinsics?
We need to clarify 1).

Intrinsic attributes are ... intrinsic to the intrinsic :) They must be legal regardless of where and how the intrinsic is used. If the intrinsic would not be nocallback in some circumstance, it cannot be nocallback at all. (This doesn't apply to call-site attributes, which raises the question of whether nocallback can be used as a call-site attribute -- if it can, then your patch would have to clear the attribute at call-sites as well, not just declarations. If it can't, then we have no problem.)

I'm ok to keep nocallback on intrinsics. Is there any chance that an existing intrinsic violates the restricted module-level nocallback semantics?
nocallback is a function attribute, not a call site attribute.

A nocallback definition.

We are not going to allow nocallback on definitions, and I think we are all on the same page for 2.

I would be okay with this limitation, but if it exists, we need to enforce it. We need to add a check to the IR verifier that the attribute does not occur on definitions, and we need to make sure that Clang does not produce invalid IR. This means either diagnosing if the attribute would be placed on a definition or silently dropping it. (For the purposes of this patch, we probably don't strongly care about having the limitation -- we could also omit the declaration check and clear the attribute on all functions. I think the question of call-site attributes is more problematic.)

In the recent revision, I dropped nocallback attribute from the definitions as well.

On that note, does someone have an example of how __attribute__((nocallback)) is getting used in the wild? Do people use the preprocessor to add the nocallback attribute in the header only for external users of the library, but omit it when including the header in the implementation (as no callback is likely not valid there?)

Whether the definition is being linked in or not, we will drop the nocallback attribute. I think, we are all on the same page for 3 and 4 as well.

Yes, I think this is right.

A nocallback intrinsic, where the attribute should never be dropped.

Why is the case? @nikic and @jdoerfert can you explain the reason behind this? Why should we drop nocallback for user declared functions, but keep it for intrinsics?
We need to clarify 1).

Intrinsic attributes are ... intrinsic to the intrinsic :) They must be legal regardless of where and how the intrinsic is used. If the intrinsic would not be nocallback in some circumstance, it cannot be nocallback at all. (This doesn't apply to call-site attributes, which raises the question of whether nocallback can be used as a call-site attribute -- if it can, then your patch would have to clear the attribute at call-sites as well, not just declarations. If it can't, then we have no problem.)

I'm ok to keep nocallback on intrinsics. Is there any chance that an existing intrinsic violates the restricted module-level nocallback semantics?

I don't think so.

nocallback is a function attribute, not a call site attribute.

Unless there is a verifier check to the contrary, all function attributes are also accepted as call-site attributes by default.

I'm fine with this in terms of semantics. Probably someone familiar with IR linking should also take a look at this to check that this is a reasonable place to do the adjustment.

llvm/lib/Linker/LinkModules.cpp
383

I wouldn't use the term "hint" here. Maybe something like this:

Drop nocallback attribute while linking, because the function might call an imported function, in which case it now calls back into the module.

gulfem updated this revision to Diff 483387.Dec 15 2022, 4:38 PM

Drop a nocallback attribute when it is on a call-site

gulfem marked an inline comment as done.Dec 15 2022, 4:40 PM

I'm fine with this in terms of semantics. Probably someone familiar with IR linking should also take a look at this to check that this is a reasonable place to do the adjustment.

A nocallback intrinsic, where the attribute should never be dropped.

Why is the case? @nikic and @jdoerfert can you explain the reason behind this? Why should we drop nocallback for user declared functions, but keep it for intrinsics?
We need to clarify 1).

Intrinsic attributes are ... intrinsic to the intrinsic :) They must be legal regardless of where and how the intrinsic is used. If the intrinsic would not be nocallback in some circumstance, it cannot be nocallback at all. (This doesn't apply to call-site attributes, which raises the question of whether nocallback can be used as a call-site attribute -- if it can, then your patch would have to clear the attribute at call-sites as well, not just declarations. If it can't, then we have no problem.)

I'm ok to keep nocallback on intrinsics. Is there any chance that an existing intrinsic violates the restricted module-level nocallback semantics?

I don't think so.

nocallback is a function attribute, not a call site attribute.

Unless there is a verifier check to the contrary, all function attributes are also accepted as call-site attributes by default.

I added support for dropping nocallback when it is added on a call-site.

I haven't read through the details of this issue yet, but implementing the fix in the ModuleLinker will not fix ThinLTO in any cases, or LTO when called from lld or gold. The ModuleLinker is legacy code that is invoked afaik only in a few cases such as by the legacy LTO (still used by ld64) and the llvm-lto tool, as well as the llvm-link tool. It is now built on top of the IRMover (see uses of a variable of this type in LinkModules.cpp). ThinLTO uses IRMover directly (see FunctionImport.cpp), as does the newer LTO API (see lib/LTO/LTO.cpp) utilized by lld and gold. The new LTO API can be tested via the llvm-lto2 tool.

The IRMover itself is built up the IRLinker (also in IRMover.cpp), and I suspect that is where the attibute fixup should be done, to handle all of these cases in one place. In fact, the comments at the declaration for IRLinker::copyGlobalValueProto in IRMover.cpp say that attribute fixup should be done there, so that seems like the right place.

gulfem updated this revision to Diff 483696.Dec 16 2022, 4:48 PM

Move it into IRMover.

gulfem added a comment.EditedDec 16 2022, 4:54 PM

I haven't read through the details of this issue yet, but implementing the fix in the ModuleLinker will not fix ThinLTO in any cases, or LTO when called from lld or gold. The ModuleLinker is legacy code that is invoked afaik only in a few cases such as by the legacy LTO (still used by ld64) and the llvm-lto tool, as well as the llvm-link tool. It is now built on top of the IRMover (see uses of a variable of this type in LinkModules.cpp). ThinLTO uses IRMover directly (see FunctionImport.cpp), as does the newer LTO API (see lib/LTO/LTO.cpp) utilized by lld and gold. The new LTO API can be tested via the llvm-lto2 tool.

The IRMover itself is built up the IRLinker (also in IRMover.cpp), and I suspect that is where the attibute fixup should be done, to handle all of these cases in one place. In fact, the comments at the declaration for IRLinker::copyGlobalValueProto in IRMover.cpp say that attribute fixup should be done there, so that seems like the right place.

Thanks for the suggestion @tejohnson, and I moved it into IRMover. I did not directly add it to copyGlobalValueProto() because we need to remove the attribute from the call sites as well. So, I added it after function body is linked in linkGlobalValueBody() (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Linker/IRMover.cpp#L637).
Please let me know if there are any concerns.

Few comments below. Also, there is another patch pending (D139209) that also performs attribute updates here, and I am thinking these should be combined into the same helper. Can you work with @TimNN on a uniform location for these attribute updates?

llvm/lib/Linker/IRMover.cpp
530 ↗(On Diff #483696)

Probably make the comment more generic, since I envision that this function will be expanded to do other attribute updates. Maybe call it "updateAttributes"?

1531 ↗(On Diff #483696)

Since I envision this function will be expanded for other attribute updates, suggest moving this comment down into the body, where it is removing this attribute.

1536 ↗(On Diff #483696)

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.

llvm/test/Linker/drop-attribute.ll
39

Is dropping the attribute required in this situation?

46

Move this CHECK up above the declaration to be consistent with the location of the other CHECKs. Also, to confirm that the nocallback is dropped, I think you need to add {{$}} at the end?

gulfem updated this revision to Diff 484352.Dec 20 2022, 12:21 PM

Addressed tejohnson@'s feedback

gulfem marked 3 inline comments as done.Dec 20 2022, 12:27 PM
gulfem added inline comments.
llvm/lib/Linker/IRMover.cpp
1536 ↗(On Diff #483696)

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.

gulfem added inline comments.Dec 20 2022, 12:33 PM
llvm/test/Linker/drop-attribute.ll
39

Yes, we would like to remove the attribute whether the definition is linked or not.

tejohnson added inline comments.Dec 21 2022, 11:06 AM
llvm/lib/Linker/IRMover.cpp
1536 ↗(On Diff #483696)

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.

gulfem added inline comments.Dec 21 2022, 6:30 PM
llvm/lib/Linker/IRMover.cpp
1536 ↗(On Diff #483696)

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.

tejohnson added inline comments.Dec 22 2022, 6:39 AM
llvm/lib/Linker/IRMover.cpp
1536 ↗(On Diff #483696)

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.

gulfem updated this revision to Diff 484915.Dec 22 2022, 11:41 AM

Updated the comment

This revision is now accepted and ready to land.Dec 22 2022, 11:47 AM
gulfem updated this revision to Diff 484924.Dec 22 2022, 12:04 PM

Replace compilation unit with module for consistency in the comments.

gulfem marked an inline comment as done.Dec 22 2022, 12:06 PM
gulfem added inline comments.
llvm/lib/Linker/IRMover.cpp
1536 ↗(On Diff #483696)

I incorporated your comment in the latest revision, and extended the description in D131628.

This revision was landed with ongoing or failed builds.Dec 22 2022, 1:10 PM
This revision was automatically updated to reflect the committed changes.
gulfem marked an inline comment as done.
tejohnson added inline comments.
llvm/lib/Linker/IRMover.cpp
1558 ↗(On Diff #484944)

Should this be CallBase (so it handles InvokeInst as well)? I missed during the review, but it was pointed out by @arsenm here: https://reviews.llvm.org/D139209#inline-1368889

gulfem marked an inline comment as done.Jan 13 2023, 5:10 PM
gulfem added inline comments.
llvm/lib/Linker/IRMover.cpp
1558 ↗(On Diff #484944)

Thanks for pointing that out, and and I uploaded D141740.