Skip to content

[llvm][EmbedBitcodePass] Prevent modifying the module with ThinLTO #139999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 29, 2025

Conversation

ilovepi
Copy link
Contributor

@ilovepi ilovepi commented May 15, 2025

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 ilovepi requested review from nikic and pcc May 15, 2025 04:07
Copy link
Contributor Author

ilovepi commented May 15, 2025

@llvmbot
Copy link
Member

llvmbot commented May 15, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Paul Kirth (ilovepi)

Changes

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


Full diff: https://github.com/llvm/llvm-project/pull/139999.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/EmbedBitcodePass.cpp (+5-1)
  • (modified) llvm/test/Transforms/EmbedBitcode/embed-wpd.ll (+4-3)
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

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we switch away from cloning because it breaks blockaddress somehow?

@ilovepi
Copy link
Contributor Author

ilovepi commented May 15, 2025

I thought we switched away from cloning to avoid inefficiency. I'll take a look through previous patches though.

@ilovepi
Copy link
Contributor Author

ilovepi commented May 15, 2025

nuts, I forgot about #72180, I guess I'll need to think about this some more.

@ilovepi
Copy link
Contributor Author

ilovepi commented May 15, 2025

oh #139223 may avoid the issue w/ BlockAddress

@ilovepi
Copy link
Contributor Author

ilovepi commented May 15, 2025

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?

@nikic
Copy link
Contributor

nikic commented May 15, 2025

@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?

Copy link
Contributor Author

ilovepi commented May 16, 2025

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.

Copy link
Contributor Author

ilovepi commented May 16, 2025

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().

 test-suite :: MultiSource/UnitTests/Float/rounding/rounding.test
  test-suite :: SingleSource/Regression/C/gcc-c-torture/execute/GCC-C-execute-pr17377.test
  test-suite :: SingleSource/UnitTests/Float/classify.test

@ilovepi
Copy link
Contributor Author

ilovepi commented May 20, 2025

@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?

Copy link
Contributor

@pcc pcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloning makes sense to me at least to begin with. Since fat LTO is a seldomly used feature, it may be better overall to be a bit less efficient in order to reduce the overall maintenance burden.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ilovepi force-pushed the users/ilovepi/fatlto-wpd-fix branch from 0bc0cc2 to 5205f4a Compare May 29, 2025 17:22
@ilovepi ilovepi force-pushed the users/ilovepi/fatlto-wpd-precommit branch from d287d80 to 4a3148f Compare May 29, 2025 17:22
Copy link
Contributor Author

ilovepi commented May 29, 2025

Merge activity

@ilovepi ilovepi force-pushed the users/ilovepi/fatlto-wpd-precommit branch 10 times, most recently from be9716d to a5f8547 Compare May 29, 2025 18:00
@ilovepi ilovepi force-pushed the users/ilovepi/fatlto-wpd-precommit branch 9 times, most recently from 8b47987 to f51dbd6 Compare May 29, 2025 20:20
@ilovepi ilovepi force-pushed the users/ilovepi/fatlto-wpd-fix branch from 5205f4a to 7373f7c Compare May 29, 2025 20:20
@ilovepi ilovepi force-pushed the users/ilovepi/fatlto-wpd-precommit branch 3 times, most recently from c1261cc to 1c9cca5 Compare May 29, 2025 20:37
Base automatically changed from users/ilovepi/fatlto-wpd-precommit to main May 29, 2025 20:39
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 ilovepi force-pushed the users/ilovepi/fatlto-wpd-fix branch from 7373f7c to 23e9886 Compare May 29, 2025 20:39
@ilovepi ilovepi merged commit 55c7d5c into main May 29, 2025
6 of 9 checks passed
@ilovepi ilovepi deleted the users/ilovepi/fatlto-wpd-fix branch May 29, 2025 20:42
google-yfyang pushed a commit to google-yfyang/llvm-project that referenced this pull request May 29, 2025
…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
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025
…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
@tstellar
Copy link
Collaborator

@ilovepi I've bisected a crash down to this commit. Attached is a reproducer:
proactor_test-f405d8.cpp.txt
proactor_test-f405d8.sh.txt

@ilovepi
Copy link
Contributor Author

ilovepi commented Jun 26, 2025

@ilovepi I've bisected a crash down to this commit. Attached is a reproducer: proactor_test-f405d8.cpp.txt proactor_test-f405d8.sh.txt

Sorry, looks like I missed the notification. I'm a bit puzzled as to how cloning the module could break things if the block address issue is fixed... I'll start looking. Should we revert? the link error we were fixing seems preferable to a crash...

@PiJoules
Copy link
Contributor

Reverting since we also ran into another crash bisected to this

; Function Attrs: optsize
declare noundef i32 @_ZN8perfetto4base9CloseFileEi(i32 noundef %0) local_unnamed_addr #2

Unknown constant!
UNREACHABLE executed at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2998!

client_impl-ef56d7.sh.txt
client_impl-ef56d7.cpp.txt

PiJoules pushed a commit that referenced this pull request Jun 27, 2025
…inLTO" (#145987)

Reverts #139999

This has a reported crash in
#139999 (comment)

This PR was intended to fix an error when linking, which is
unfortunately preferable to crashing clang. For now, we'll revert and
investigate the problem.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jun 27, 2025
…ule with ThinLTO" (#145987)

Reverts llvm/llvm-project#139999

This has a reported crash in
llvm/llvm-project#139999 (comment)

This PR was intended to fix an error when linking, which is
unfortunately preferable to crashing clang. For now, we'll revert and
investigate the problem.
@nikic
Copy link
Contributor

nikic commented Jun 27, 2025

Both of those test cases reference a function inside DITemplateValueParameter metadata. Unfortunately, I'm not able to reproduce this issue when running through opt instead of clang.

@nikic
Copy link
Contributor

nikic commented Jun 27, 2025

Added support for running the fat LTO pipeline in opt in #146048, though this does not help reproducing this issue.

Here is a reduction:

// RUN: clang -g -flto=thin -ffat-lto-objects %s
template <void CloseFunction()>
struct ScopedResource {
  ~ScopedResource() { CloseFunction(); }
};
 
void CloseFile();
 
struct Client {
  void CreateInstance(ScopedResource<CloseFile>);
  Client();
};
 
struct ClientImpl {
  ClientImpl(ScopedResource<CloseFile>, int *);
};
 
int CreateInstance_task_runner;
void Client::CreateInstance(ScopedResource<CloseFile> conn_args) {
  ClientImpl(conn_args, &CreateInstance_task_runner);
}

@nikic
Copy link
Contributor

nikic commented Jun 27, 2025

As one would expect, what ultimately causes the crash is a failure to remap a metadata reference of the declaration of the one from the new module. But I'm having a hard time understanding what exactly the root cause for that is. What I see is that we do correctly remap one of the transitive references from the DISubprogram, but not the one in the declaration DISubprogram. The difference between running clang and opt is that in the latter case the type referenced by DISubprogram and its declaration DISubprogram is the same, while in the former case it is different.

rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
…inLTO" (llvm#145987)

Reverts llvm#139999

This has a reported crash in
llvm#139999 (comment)

This PR was intended to fix an error when linking, which is
unfortunately preferable to crashing clang. For now, we'll revert and
investigate the problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-fwhole-program-vtables breaks in combination with -ffat-lto-objects
6 participants