-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[LLDB][Minidump] Make workaround for the Dynamic loader issue #120166
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
Changes from 3 commits
d5dfb15
49bc821
66de146
2214e6c
e3bc772
dbec309
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -354,6 +354,39 @@ DataExtractor ProcessMinidump::GetAuxvData() { | |||||||
| GetAddressByteSize(), GetAddressByteSize()); | ||||||||
| } | ||||||||
|
|
||||||||
| bool ProcessMinidump::IsLLDBMinidump() { | ||||||||
| // If we've already checked, return the cached value | ||||||||
| if (m_is_lldb_generated.has_value()) | ||||||||
| return *m_is_lldb_generated; | ||||||||
|
|
||||||||
| // If the minidump doesn't have a LLDBGeneratedStream, it's not an LLDB | ||||||||
|
||||||||
| // We also check to see if the section was generated correctly, but not | ||||||||
| // enforcing an exact size so we can change it in the future without | ||||||||
| // impacting older generated Minidumps. | ||||||||
| llvm::ArrayRef<uint8_t> lldbStream = | ||||||||
| m_minidump_parser->GetStream(StreamType::LLDBGenerated); | ||||||||
| if (lldbStream.empty() || lldbStream.size() <= sizeof(StreamType)) { | ||||||||
|
||||||||
| if (lldbStream.empty() || lldbStream.size() <= sizeof(StreamType)) { | |
| if (lldbStream.size() < sizeof(StreamType)) { |
empty is subsumed by the size check, and I think you need a strict comparison here.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const uint32_t *lldbStreamType = | |
| reinterpret_cast<const uint32_t *>(lldbStream.data()); | |
| uint32_t lldbStreamType = llvm::support::endian::read<uint32_t, llvm::endianness::little>(lldbStream.data()); |
We only support little-endian minidumps right now, but it'd be nice if they could be read on a system of any endianness. I'm also not sure if this is guaranteed to be aligned.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,7 @@ enum class StreamType : uint32_t { | |
| #include "llvm/BinaryFormat/MinidumpConstants.def" | ||
| Unused = 0, | ||
| LastReserved = 0x0000ffff, | ||
| LLDBGenerated = 0x4C4C4442, // ASCII for 'LLDB' | ||
|
||
| }; | ||
|
|
||
| /// Specifies the location (and size) of various objects in the minidump file. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the extra return?
(It would be nice to at least have a test that verifies that the lldb stream is actually written)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have a test that depends on the dynamic loader, but it was passing in CI, even with this incorrect return which is unexpected. I tested locally by capturing a minidump with TLS variables on an older version of lldb, and verified there is no dyld or TLS variables when loaded with LLDB built on this patch. Capturing and loading on this patch ensured it worked. This does mean we're making a breaking change for behavior, where all older TLS minidumps will no longer work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately I think this patch is a workaround for the fact that the dynamic loader (posix) and process (minidump) classes can't agree on how to load the modules. The dynamic loader expect that it will be completely in charge of module loading, and this works fine in case of a live process, as it always contains enough information to determine the set of loaded modules. A minidump does not (always) contains this information, but (maybe for that same reason) it contains a explicit list of loaded modules (which is handled by the process class). When we start to use both sources, things get messy. I'm not sure what's the best way to handle this situation, but I think this patch approximates the desired behavior relatively well, and I think it is useful to be able to tell who generated a minidump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@labath I agree, I think it would be ideal to agree with Breakpad/Crashpad on a section about what program generated it, including version of that software. From Memory I know there is some brakepad relevant data, but a unified section would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change all dynamic loaders to always check with the process via:
This allows processes to tell the dynamic loaders where things are loaded in case they know, like in the case of minidumps. So we can probably make the POSIX dynamic loader a bit smarter when it comes to dealing with core files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing comes back from this call, then the dynamic loader will use its normal means to locate things, else, it should use the list that the process provides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clayborg Just so I follow, you're recommending we embed a structured data (really just Json) into a section? If so, I really like the proposal, because it allows us to have a fairly loose schema across Brakepad, Crashpad and LLDB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we should implement the
lldb_private::StructuredData::ObjectSP Process::GetLoadedDynamicLibrariesInfos()virtual method in ProcessMinidump and return a structured data that describes where everything is loaded. Then have the POSIX dynamic loader call this method on the process object and see if it returns anything. If it does, then we use this as the truth, else we discover the shared libraries using the existing code in the POSIX dynamic loader.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(since ProcessMinidump has the shared library load list embedded in a directory).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clayborg Got it, from the context of the conversation I thought you wanted that structure data embedded in the Corefile.
I think that makes sense instead of using this Flag stream.