[ThinLTO] Do not duplicate import a function that is actually defined in the current module #110064
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the current module. Doing so could cause a bug where the linker tries to remap a function "reimported" from the current module when materializing it, causing a lookup assert in the type mappings.
|
@llvm/pr-subscribers-lto Author: William Junda Huang (huangjd) ChangesDoing so could cause a bug where the linker tries to remap a function "reimported" from the current module when materializing it, causing a lookup assert in the type mappings. Full diff: https://github.com/llvm/llvm-project/pull/110064.diff 1 Files Affected:
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 3a6c2678cd157f..5bd05d86a949c3 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -595,11 +595,15 @@ Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
if (!SGV)
return nullptr;
+ // If SGV is from dest, it is already materialized when dest was loaded.
+ if (SGV->getParent() == &DstM)
+ return nullptr;
+
// When linking a global from other modules than source & dest, skip
// materializing it because it would be mapped later when its containing
// module is linked. Linking it now would potentially pull in many types that
// may not be mapped properly.
- if (SGV->getParent() != &DstM && SGV->getParent() != SrcM.get())
+ if (SGV->getParent() != SrcM.get())
return nullptr;
Expected<Constant *> NewProto = linkGlobalValueProto(SGV, ForIndirectSymbol);
|
|
This patch is to fix a bug encountered while bootstrapping clang itself, I was not able to create a minimum reproducing case. The failing case happens in a thinLTO step involving a lot of files so it is unrealistic to make it a test case. |
|
Added a test case. It is very contrived, and I think in real life only when using thinLTO + function pointer template parameter in source code has a potential to trigger it, and building clang itself is one example |
|
It also possibly reveals another bug (not related, won't cause immediate trouble). If there is a template with function pointer parameter, and there's an instantiation of the template but the function used is not defined, then the optimizer could do away the metadata info for the template parameter, so we will ended up non-identical metadata type nodes with the same name in different modules. If |
The non-LTO behavior if Alternatively, GCC never produces references to these pointer non-type-template parameters anyway, because doing so can lead to different behavior when building with and without debug info (failure to link is the least problematic one - more problematic would be if the linker pulled in new object files to satisfy the reference, then had global ctors in them that ran - changing program behavior). I will say, ideally, if the function is optimized away, rather than using 0 for the value in the DWARF, we should, ideally, use the "tombstone" value (which should be -1/maxint/etc, ideally) though that does require a bit of a special case in the linker. That'll make it clear that this value is different from a null pointer which is a valid value for a non-type template parameter of pointer type. But don't feel like you have to solve that problem - this is pretty niche and, again, GCC provides no value due to these complications, so we're not missing out on some important opportunity. |
teresajohnson
approved these changes
Oct 10, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/6576 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/6574 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/6700 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/6684 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/12/builds/7520 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/6778 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/5038 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/3140 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/54/builds/3017 Here is the relevant piece of the build log for the reference |
| ; | ||
| ; RUN: opt -module-summary -o %t1.bc %s | ||
| ; RUN: opt -module-summary -o %t2.bc %S/Inputs/ditemplatevalueparameter-remap.ll | ||
| ; RUN: ld.lld --plugin-opt=thinlto-index-only -shared %t1.bc %t2.bc |
There was a problem hiding this comment.
I see you are getting a lot of build bot failures. I missed the fact that your test uses lld. You should use llvm-lto2 instead
There was a problem hiding this comment.
I fixed the test case and this is the new pr #111933
Is there a way to "dry run" a patch on all llvm build bots before actually submitting something?
There was a problem hiding this comment.
Is there a way to "dry run" a patch on all llvm build bots before actually submitting something?
Unfortunately not.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/187/builds/1803 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/125/builds/2714 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/4584 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/4893 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/39/builds/2176 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/1427 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/6955 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/9608 Here is the relevant piece of the build log for the reference |
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
… in the current module (llvm#110064) Doing so could cause a bug where the linker tries to remap a function "reimported" from the current module when materializing it, causing a lookup assert in the type mappings.
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
… defined in the current module" (llvm#111919) Reverts llvm#110064
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 17, 2024
… in the current module (llvm#110064) Doing so could cause a bug where the linker tries to remap a function "reimported" from the current module when materializing it, causing a lookup assert in the type mappings.
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 17, 2024
… defined in the current module" (llvm#111919) Reverts llvm#110064
Closed
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
… in the current module llvm#110064 (llvm#111933) Trying to land llvm#110064 again after fixing test case
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Doing so could cause a bug where the linker tries to remap a function "reimported" from the current module when materializing it, causing a lookup assert in the type mappings.