Skip to content

[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.

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 6 commits into from
Dec 30, 2024
Merged

[LLDB][Minidump] Make workaround for the Dynamic loader issue #120166

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d5dfb15
Make workaround for the Dynamic loader issue
Jlalond Dec 17, 2024
49bc821
Reset and go with Pavel's plan of a stream section instead of a flag
Jlalond Dec 20, 2024
66de146
Fix reversed logic, add the new stream type to the parsers ENUM_TO_ST…
Jlalond Dec 20, 2024
2214e6c
Support empty stream, remove magic bytes
Jlalond Dec 23, 2024
e3bc772
Run GCF
Jlalond Dec 23, 2024
dbec309
Remove extra return
Jlalond Dec 30, 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
15 changes: 12 additions & 3 deletions lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
// First set the offset on the file, and on the bytes saved
m_saved_data_size = HEADER_SIZE;
// We know we will have at least Misc, SystemInfo, Modules, and ThreadList
// (corresponding memory list for stacks) And an additional memory list for
// non-stacks.
// (corresponding memory list for stacks), an additional memory list for
// non-stacks, and a stream to mark this minidump was generated by LLDB.
lldb_private::Target &target = m_process_sp->GetTarget();
m_expected_directories = 6;
// Check if OS is linux and reserve directory space for all linux specific
Expand Down Expand Up @@ -89,8 +89,11 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
"Failed to fill in header and directory "
"sections. Written / Expected (%" PRIx64 " / %" PRIx64 ")",
new_offset, m_saved_data_size);

return error;
if (error.Fail())
return error;
Copy link
Collaborator

@labath labath Dec 26, 2024

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)

Copy link
Contributor Author

@Jlalond Jlalond Dec 30, 2024

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.

Copy link
Collaborator

@labath labath Jan 7, 2025

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.

Copy link
Contributor Author

@Jlalond Jlalond Jan 7, 2025

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.

Copy link
Collaborator

@clayborg clayborg Jan 7, 2025

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:

lldb_private::StructuredData::ObjectSP Process::GetLoadedDynamicLibrariesInfos()

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.

Copy link
Collaborator

@clayborg clayborg Jan 7, 2025

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.

Copy link
Contributor Author

@Jlalond Jlalond Jan 7, 2025

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.

Copy link
Collaborator

@clayborg clayborg Jan 7, 2025

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.

Copy link
Collaborator

@clayborg clayborg Jan 7, 2025

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

Copy link
Contributor Author

@Jlalond Jlalond Jan 7, 2025

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.


return AddLLDBGeneratedStream();
}

Status MinidumpFileBuilder::AddDirectory(StreamType type,
Expand Down Expand Up @@ -126,6 +129,12 @@ Status MinidumpFileBuilder::AddDirectory(StreamType type,
return error;
}

Status MinidumpFileBuilder::AddLLDBGeneratedStream() {
Status error;
StreamType type = StreamType::LLDBGenerated;
return AddDirectory(type, 0);
}

Status MinidumpFileBuilder::AddSystemInfo() {
Status error;
const llvm::Triple &target_triple =
Expand Down
1 change: 1 addition & 0 deletions lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class MinidumpFileBuilder {
void DeleteFile() noexcept;

private:
lldb_private::Status AddLLDBGeneratedStream();
// Add data to the end of the buffer, if the buffer exceeds the flush level,
// trigger a flush.
lldb_private::Status AddData(const void *data, uint64_t size);
Expand Down
6 changes: 6 additions & 0 deletions lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ llvm::ArrayRef<uint8_t> MinidumpParser::GetStream(StreamType stream_type) {
return m_file->getRawStream(stream_type).value_or(llvm::ArrayRef<uint8_t>());
}

std::optional<llvm::ArrayRef<uint8_t>>
MinidumpParser::GetRawStream(StreamType stream_type) {
return m_file->getRawStream(stream_type);
}

UUID MinidumpParser::GetModuleUUID(const minidump::Module *module) {
auto cv_record =
GetData().slice(module->CvRecord.RVA, module->CvRecord.DataSize);
Expand Down Expand Up @@ -651,6 +656,7 @@ MinidumpParser::GetStreamTypeAsString(StreamType stream_type) {
ENUM_TO_CSTR(FacebookAbortReason);
ENUM_TO_CSTR(FacebookThreadName);
ENUM_TO_CSTR(FacebookLogcat);
ENUM_TO_CSTR(LLDBGenerated);
}
return "unknown stream type";
}
Expand Down
1 change: 1 addition & 0 deletions lldb/source/Plugins/Process/minidump/MinidumpParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class MinidumpParser {
llvm::ArrayRef<uint8_t> GetData();

llvm::ArrayRef<uint8_t> GetStream(StreamType stream_type);
std::optional<llvm::ArrayRef<uint8_t>> GetRawStream(StreamType stream_type);

UUID GetModuleUUID(const minidump::Module *module);

Expand Down
16 changes: 16 additions & 0 deletions lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,22 @@ DataExtractor ProcessMinidump::GetAuxvData() {
GetAddressByteSize(), GetAddressByteSize());
}

bool ProcessMinidump::IsLLDBMinidump() {
std::optional<llvm::ArrayRef<uint8_t>> lldb_generated_section =
m_minidump_parser->GetRawStream(StreamType::LLDBGenerated);
return lldb_generated_section.has_value();
}

DynamicLoader *ProcessMinidump::GetDynamicLoader() {
// This is a workaround for the dynamic loader not playing nice in issue
// #119598. The specific reason we use the dynamic loader is to get the TLS
// info sections, which we can assume are not being written to the minidump
// unless it's an LLDB generate minidump.
if (IsLLDBMinidump())
return PostMortemProcess::GetDynamicLoader();
return nullptr;
}

void ProcessMinidump::BuildMemoryRegions() {
if (m_memory_regions)
return;
Expand Down
7 changes: 5 additions & 2 deletions lldb/source/Plugins/Process/minidump/ProcessMinidump.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ class ProcessMinidump : public PostMortemProcess {

Status DoLoadCore() override;

DynamicLoader *GetDynamicLoader() override;

// Returns AUXV structure found in the core file
lldb_private::DataExtractor GetAuxvData() override;

Expand All @@ -74,8 +76,8 @@ class ProcessMinidump : public PostMortemProcess {

ArchSpec GetArchitecture();

Status GetMemoryRegions(
lldb_private::MemoryRegionInfos &region_list) override;
Status
GetMemoryRegions(lldb_private::MemoryRegionInfos &region_list) override;

bool GetProcessInfo(ProcessInstanceInfo &info) override;

Expand Down Expand Up @@ -113,6 +115,7 @@ class ProcessMinidump : public PostMortemProcess {
std::optional<MemoryRegionInfos> m_memory_regions;

void BuildMemoryRegions();
bool IsLLDBMinidump();
};

} // namespace minidump
Expand Down
4 changes: 4 additions & 0 deletions llvm/include/llvm/BinaryFormat/MinidumpConstants.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ HANDLE_MDMP_STREAM_TYPE(0xFACECCCC, FacebookAppStateLog)
HANDLE_MDMP_STREAM_TYPE(0xFACEDEAD, FacebookAbortReason)
HANDLE_MDMP_STREAM_TYPE(0xFACEE000, FacebookThreadName)

// LLDB specific stream types
// Ascii for 'LLDB'
HANDLE_MDMP_STREAM_TYPE(0x4C4C4442, LLDBGenerated)

HANDLE_MDMP_ARCH(0x0000, X86) // PROCESSOR_ARCHITECTURE_INTEL
HANDLE_MDMP_ARCH(0x0001, MIPS) // PROCESSOR_ARCHITECTURE_MIPS
HANDLE_MDMP_ARCH(0x0002, Alpha) // PROCESSOR_ARCHITECTURE_ALPHA
Expand Down
Loading