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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
| Time | Test | |
|---|---|---|
| 60,020 ms | x64 debian > libFuzzer.libFuzzer::minimize_crash.test |
Event Timeline
| 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. | |
| llvm/lib/Linker/LinkModules.cpp | ||
|---|---|---|
| 546 | Scratch this, it looks like the FunctionAttrs pass rewrites these based on the results of the bitcode link. | |
Comment Actions
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 | |
Comment Actions
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.
Comment Actions
Done!
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.
| llvm/test/Linker/drop-attribute.ll | ||
|---|---|---|
| 14 | Checking the whole line should allow omitting the other two CHECK-NOT lines | |
Comment Actions
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?
Comment Actions
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.
Comment Actions
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).
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.
Comment Actions
- Drop memory(inaccessiblemem) attributes as well, and added test cases for them
- Added a test case for testing intrinsics
- Added a test case for a nocallback declaration without a definition being linked in
Comment Actions
I would like to make sure that we all agree on the following items:
- 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).
- A nocallback definition.
We are not going to allow nocallback on definitions, and I think we are all on the same page for 2.
- A nocallback declaration with a definition that is being linked in.
- 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.
Comment Actions
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. | |
Comment Actions
That appears to be how glibc uses it: https://github.com/bminor/glibc/blob/d055481ce39d03652ac60de5078889e15b6917ff/misc/sys/cdefs.h#L64
Comment Actions
- Remove dropping memory(inaccessiblemem) attribute
- Keep nocallback attribute in intrinsics
- Drop nocallback attribute on definitions
Comment Actions
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.
Comment Actions
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.
Comment Actions
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:
| |
Comment Actions
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.
Comment Actions
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.
Comment Actions
| 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? | |
| 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. |
| llvm/test/Linker/drop-attribute.ll | ||
|---|---|---|
| 39 | Yes, we would like to remove the attribute whether the definition is linked or not. | |
| 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. |
| 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"). 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. |
| 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. |
| 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 |
I wouldn't use the term "hint" here. Maybe something like this: