[llvm][EmbedBitcodePass] Prevent modifying the module with ThinLTO #139999
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@llvm/pr-subscribers-llvm-transforms Author: Paul Kirth (ilovepi) ChangesSince ThinLTOBitcodeWriterPass handles many things for CFI and WPD, like The only non-invasive way to do that is to clone the module when Fixes #139440 Full diff: https://github.com/llvm/llvm-project/pull/139999.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp b/llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
index 73f567734a91b..7a378c61cb899 100644
--- a/llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
+++ b/llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp
@@ -16,6 +16,7 @@
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/Triple.h"
#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
+#include "llvm/Transforms/Utils/Cloning.h"
#include "llvm/Transforms/Utils/ModuleUtils.h"
#include <string>
@@ -33,8 +34,11 @@ PreservedAnalyses EmbedBitcodePass::run(Module &M, ModuleAnalysisManager &AM) {
std::string Data;
raw_string_ostream OS(Data);
+ // Clone the module with with Thin LTO, since ThinLTOBitcodeWriterPass changes
+ // vtable linkage that would break the non-lto object code for FatLTO.
if (IsThinLTO)
- ThinLTOBitcodeWriterPass(OS, /*ThinLinkOS=*/nullptr).run(M, AM);
+ ThinLTOBitcodeWriterPass(OS, /*ThinLinkOS=*/nullptr)
+ .run(*llvm::CloneModule(M), AM);
else
BitcodeWriterPass(OS, /*ShouldPreserveUseListOrder=*/false, EmitLTOSummary)
.run(M, AM);
diff --git a/llvm/test/Transforms/EmbedBitcode/embed-wpd.ll b/llvm/test/Transforms/EmbedBitcode/embed-wpd.ll
index f1f7712f54039..54931be42b4eb 100644
--- a/llvm/test/Transforms/EmbedBitcode/embed-wpd.ll
+++ b/llvm/test/Transforms/EmbedBitcode/embed-wpd.ll
@@ -1,12 +1,13 @@
; RUN: opt --mtriple x86_64-unknown-linux-gnu < %s -passes="embed-bitcode<thinlto>" -S | FileCheck %s
-; CHECK-NOT: $_ZTV3Foo = comdat any
+; CHECK: $_ZTV3Foo = comdat any
$_ZTV3Foo = comdat any
$_ZTI3Foo = comdat any
-; CHECK: @_ZTV3Foo = external hidden unnamed_addr constant { [5 x ptr] }, align 8
-; CHECK: @_ZTI3Foo = linkonce_odr hidden constant { ptr, ptr, ptr } { ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv120__si_class_type_infoE, i64 2), ptr @_ZTS3Foo, ptr @_ZTISt13runtime_error }, comdat, align 8
+;; ThinLTOBitcodeWriter will remove the vtable for Foo, and make it an external symbol
+; CHECK: @_ZTV3Foo = linkonce_odr hidden unnamed_addr constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr @_ZTI3Foo, ptr @_ZN3FooD2Ev, ptr @_ZN3FooD0Ev, ptr @_ZNKSt13runtime_error4whatEv] }, comdat, align 8, !type !0, !type !1, !type !2, !type !3, !type !4, !type !5
+; CHECK-NOT: @foo = external unnamed_addr constant { [5 x ptr] }, align 8
; CHECK: @llvm.embedded.object = private constant {{.*}}, section ".llvm.lto", align 1
; CHECK: @llvm.compiler.used = appending global [1 x ptr] [ptr @llvm.embedded.object], section "llvm.metadata"
@_ZTV3Foo = linkonce_odr hidden unnamed_addr constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr @_ZTI3Foo, ptr @_ZN3FooD2Ev, ptr @_ZN3FooD0Ev, ptr @_ZNKSt13runtime_error4whatEv] }, comdat, align 8, !type !0, !type !1, !type !2, !type !3, !type !4, !type !5
|
|
I thought we switched away from cloning to avoid inefficiency. I'll take a look through previous patches though. |
|
nuts, I forgot about #72180, I guess I'll need to think about this some more. |
|
oh #139223 may avoid the issue w/ BlockAddress |
|
Well, with my patch the program in initial bug report from #70703 is passing w/ this patch, and the rest of the test suite seems OK ... maybe its fine to clone now? |
|
@ilovepi Does it also work on the release branch? I'd mainly see the clone module approach as something easily backportable for the release branch, but I assume for main we'll want a different solution? |
|
I'm testing the release branch now. I believe that the issue w/ cloning was solved before we cut the release, and the patch above just added regression tests, so I expect it to work. I'm all ears if you can think of a better way to handle this on main. The main problem I saw as I looked was that there isn't a good way to figure out what ThinLTOBitcodeWriter changed in the module (e.g. did it remove a comdat, change a linkage, etc.). We want those things to happen for the ThinLTO bitcode, but its awfully problematic for the rest of the compile. I guess we could add some metadata to the instructions we changed and then clean up after, but that also seems to not be great. |
|
The test suite seems just as happy with the FatLTO fix as it does with ThinLTO on the release branch. I see the same three tests failing, and the reported test case passes as well. For completeness these are the failing tests. This was consistent w/ FatLTO + ThinLTO, and ThinLTO alone on built from releases/20.x with the call to CloneModule(). |
|
@nikic I spent some time over the weekend looking at this, I think if we want to avoid cloning the module, we'd need to record enough metadata to recover the old state. For the Vtable I think that's not too much data, but IDK about the rest. That said, that whole approach seems awfully invasive to the pass and is likely to be both buggy and brittle. I just don't see a good way for us to roll back those changes to the VTable otherwise... Should we just land this for now, and try to come up with something better/different as a follow up? |
There was a problem hiding this comment.
Okay, let's go with this for now.
Compile-time impact of cloning the module is about 0.2% when building clang with fat LTO: https://llvm-compile-time-tracker.com/compare.php?from=11a01e851a06188ae946ace1140f866d7a667221&to=46e037d763e7997a83ce78c9a602248fd67f0d44&stat=instructions:u
ilovepi
force-pushed
the
users/ilovepi/fatlto-wpd-fix
branch
from
0bc0cc2 to
5205f4a
Compare
ilovepi
force-pushed
the
users/ilovepi/fatlto-wpd-precommit
branch
from
d287d80 to
4a3148f
Compare
Merge activity
|
ilovepi
force-pushed
the
users/ilovepi/fatlto-wpd-precommit
branch
10 times, most recently
from
be9716d to
a5f8547
Compare
ilovepi
force-pushed
the
users/ilovepi/fatlto-wpd-precommit
branch
12 times, most recently
from
8b47987 to
f51dbd6
Compare
ilovepi
force-pushed
the
users/ilovepi/fatlto-wpd-fix
branch
from
5205f4a to
7373f7c
Compare
ilovepi
force-pushed
the
users/ilovepi/fatlto-wpd-precommit
branch
3 times, most recently
from
c1261cc to
1c9cca5
Compare
Since ThinLTOBitcodeWriterPass handles many things for CFI and WPD, like updating vtable linkage, we need to prevent those changes from persisting in the non-LTO object code we will compile under FatLTO. The only non-invasive way to do that is to clone the module when serializing the module in ThinLTOBitcodeWriterPass. We may be able to avoid cloning in the future with additional infrastructure to restore the IR to its original state. Fixes #139440
ilovepi
force-pushed
the
users/ilovepi/fatlto-wpd-fix
branch
from
7373f7c to
23e9886
Compare
…lvm#139999) Since ThinLTOBitcodeWriterPass handles many things for CFI and WPD, like updating vtable linkage, we need to prevent those changes from persisting in the non-LTO object code we will compile under FatLTO. The only non-invasive way to do that is to clone the module when serializing the module in ThinLTOBitcodeWriterPass. We may be able to avoid cloning in the future with additional infrastructure to restore the IR to its original state. Fixes llvm#139440
…lvm#139999) Since ThinLTOBitcodeWriterPass handles many things for CFI and WPD, like updating vtable linkage, we need to prevent those changes from persisting in the non-LTO object code we will compile under FatLTO. The only non-invasive way to do that is to clone the module when serializing the module in ThinLTOBitcodeWriterPass. We may be able to avoid cloning in the future with additional infrastructure to restore the IR to its original state. Fixes llvm#139440
Since ThinLTOBitcodeWriterPass handles many things for CFI and WPD, like
updating vtable linkage, we need to prevent those changes from
persisting in the non-LTO object code we will compile under FatLTO.
The only non-invasive way to do that is to clone the module when
serializing the module in ThinLTOBitcodeWriterPass. We may be able to
avoid cloning in the future with additional infrastructure to restore
the IR to its original state.
Fixes #139440