Skip to content

[RemoveDIs] Don't convert debug-info in bitcode-loading just now #80865

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.

Sign up for GitHub

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 2 commits into from
Feb 7, 2024
Merged

[RemoveDIs] Don't convert debug-info in bitcode-loading just now #80865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eec3c25
[RemoveDIs] Don't convert debug-info in bitcode-loading just now
jmorse Feb 6, 2024
89e1800
Strip out more undesired bitcode conversion
jmorse Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions llvm/lib/Bitcode/Reader/BitcodeReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6632,9 +6632,6 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
if (Error Err = materializeMetadata())
return Err;

bool NewDebugInfoRequested = F->IsNewDbgInfoFormat;
F->IsNewDbgInfoFormat = false;

// Move the bit stream to the saved position of the deferred function body.
if (Error JumpFailed = Stream.JumpToBit(DFII->second))
return JumpFailed;
Expand Down Expand Up @@ -6710,14 +6707,6 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
// Look for functions that rely on old function attribute behavior.
UpgradeFunctionAttributes(*F);

// If we've materialized a function set up in "new" debug-info mode, the
// contents just loaded will still be in dbg.value mode. Switch to the new
// mode now. NB: we can add more complicated logic here in the future to
// correctly identify when we do and don't need to autoupgrade.
if (NewDebugInfoRequested) {
F->convertToNewDbgValues();
}

// Bring in any functions that this function forward-referenced via
// blockaddresses.
return materializeForwardReferencedFunctions();
Expand Down Expand Up @@ -8042,14 +8031,6 @@ BitcodeModule::getModuleImpl(LLVMContext &Context, bool MaterializeAll,
return std::move(Err);
}

// If we are operating in a "new debug-info" context, upgrade the debug-info
// in the loaded module. This is a transitional approach as we enable "new"
// debug-info in LLVM, which will eventually be pushed down into the
// autoupgrade path once the bitcode-encoding is finalised. Non-materialised
// functions will be upgraded in the materialize method.
if (UseNewDbgInfoFormat && !M->IsNewDbgInfoFormat)
M->convertToNewDbgValues();

return std::move(M);
}

Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Linker/IRMover.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1775,6 +1775,8 @@ IRMover::IRMover(Module &M) : Composite(M) {
Error IRMover::move(std::unique_ptr<Module> Src,
ArrayRef<GlobalValue *> ValuesToLink,
LazyCallback AddLazyFor, bool IsPerformingImport) {
if (getModule().IsNewDbgInfoFormat)
Src->convertToNewDbgValues();
Comment on lines +1778 to +1779
Copy link
Contributor

Choose a reason for hiding this comment

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

Not familiar with the context of this function, but presumably Src will never be in the new format already because it's just been read from bitcode?

Copy link
Member Author

Choose a reason for hiding this comment

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

"Probably yes" -- with this patch it's only the pass manager that's going to create modules in new-debug-info mode, so any other module created Should(TM) be in dbg.value format. This seems to be the case in the llvm-lit test suite.

I'm 99.5% confident of this; but we've also got a bunch of assertions for the IsNewDbgInfoFormat flag to detect codepaths where this isn't the case. If it turns out to be wrong we'll hopefully break-fast-break-hard.

IRLinker TheIRLinker(Composite, SharedMDs, IdentifiedStructTypes,
std::move(Src), ValuesToLink, std::move(AddLazyFor),
IsPerformingImport);
Expand Down
3 changes: 3 additions & 0 deletions llvm/test/ThinLTO/X86/crash_debuginfo.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
; RUN: opt -passes=function-import,inline -summary-file %t-index.thinlto.bc %t-dst.bc -o %t.out
; RUN: llvm-nm -U %t.out | FileCheck %s --implicit-check-not=_bar

; Ensure we can do the same thing in RemoveDIs mode.
; RUN: opt -passes=function-import,inline -summary-file %t-index.thinlto.bc %t-dst.bc -o %t.out --try-experimental-debuginfo-iterators

; Verify that we import bar and inline it. It use to crash importing due to ODR type uniquing
; CHECK: _foo

Expand Down