[lldb] Don't clear a Module's UnwindTable when adding a SymbolFile (#86603) #8479
Conversation
This file contains hidden or 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
…lvm#86603) Fixing a crash in lldb when `symbols.auto-download` setting is enabled. When doing a backtrace, this feature has lldb search for a SymbolFile for stack frames when we are backtracing, and add them either synchoronously or asynchronously, depending on the specific setting used. Module::SetSymbolFileFileSpec clears the Module's UnwindTable, once we find a new SymbolFile. We may be adding a source of unwind information that we did not have when lldb was working only with the executable binary. What happens in practice is that we're using a reference to the Module's UnwindTable, and then the other thread getting the SymbolFile clears it and now the first thread is referring to freed memory and we can crash. When built with address sanitizer, it crashes much more reliably. Given that unwind information used for exception handling -- eh_frame, compact unwind -- is present in executable binaries, the only thing we're likely to *add* would be DWARF's `debug_frame` if that was also available. The actual value of re-creating the UnwindTable when we have added a SymbolFile is not large. I also tried fixing this by changing the Module to have a shared_ptr to the UnwindTable, so we could have two different UnwindTable's in use simultaneously for a brief period. This would be fine TODAY, but it introduces a very subtle bug that someone will have a heck of a time figuring out in the future. In the end, I believe the safest approach is to sacrifice the possible marginal gain of reconstructing the UnwindTable once a SymbolFile has been added, to sidestep this whole problem area. Also, in `Module::GetUnwindTable()`, call `DownloadSymbolFileAsync` before we create the UnwindTable for the first time, in case the symbol file is fetched synchronously, we will have it for that possible marginal gain. (cherry picked from commit 2f63718)
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test windows |
In
commit 2f63718
Author: Jason Molenda <[email protected]>
Date: Tue Mar 26 09:07:15 2024 -0700
[lldb] Don't clear a Module's UnwindTable when adding a SymbolFile
(llvm#86603)
I stopped clearing a Module's UnwindTable when we add a SymbolFile to
avoid the memory management problems with adding a symbol file
asynchronously while the UnwindTable is being accessed on another
thread. This broke the target-symbols-add-unwind.test shell test on
Linux which removes the DWARF debub_frame section from a binary, loads
it, then loads the unstripped binary with the DWARF debug_frame section
and checks that the UnwindPlans for a function include debug_frame.
I originally decided that I was willing to sacrifice the possiblity of
additional unwind sources from a symbol file because we rely on assembly
emulation so heavily, they're rarely critical. But there are targets
where we we don't have emluation and rely on things like DWARF
debug_frame a lot more, so this probably wasn't a good choice.
This patch adds a new UnwindTable::Update method which looks for any new
sources of unwind information and adds it to the UnwindTable, and calls
that after a new SymbolFile has been added to a Module.
(cherry picked from commit 6a0ec8e)
|
@swift-ci test |
|
@swift-ci test windows |
jasonmolenda
deleted the
cp/dont-reset-unwindtable-when-adding-symbol-file-6.0
branch
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.
Fixing a crash in lldb when
symbols.auto-downloadsetting is enabled. When doing a backtrace, this feature has lldb search for a SymbolFile for stack frames when we are backtracing, and add them either synchoronously or asynchronously, depending on the specific setting used.Module::SetSymbolFileFileSpec clears the Module's UnwindTable, once we find a new SymbolFile. We may be adding a source of unwind information that we did not have when lldb was working only with the executable binary.
What happens in practice is that we're using a reference to the Module's UnwindTable, and then the other thread getting the SymbolFile clears it and now the first thread is referring to freed memory and we can crash. When built with address sanitizer, it crashes much more reliably.
Given that unwind information used for exception handling -- eh_frame, compact unwind -- is present in executable binaries, the only thing we're likely to add would be DWARF's
debug_frameif that was also available. The actual value of re-creating the UnwindTable when we have added a SymbolFile is not large.I also tried fixing this by changing the Module to have a shared_ptr to the UnwindTable, so we could have two different UnwindTable's in use simultaneously for a brief period. This would be fine TODAY, but it introduces a very subtle bug that someone will have a heck of a time figuring out in the future.
In the end, I believe the safest approach is to sacrifice the possible marginal gain of reconstructing the UnwindTable once a SymbolFile has been added, to sidestep this whole problem area.
Also, in
Module::GetUnwindTable(), callDownloadSymbolFileAsyncbefore we create the UnwindTable for the first time, in case the symbol file is fetched synchronously, we will have it for that possible marginal gain.(cherry picked from commit 2f63718)