labath added a reviewer: markmentovai.
labath added a comment.
+mark, in case he has any thoughts about this. It's unfortunate that we need to
add workarounds like this, but it seems that's what the situation is.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py:155-156
+ self.verify_module(modules[0],
+ "libuuidmatch.so",
+ "7295E17C-6668-9E05-CBB5-DEE5003865D5-5267C116")
+
----------------
The yamlization of minidumps is unfortunately not ready yet, but it should be
possible to check in the so files in yaml form. Could you please try that out.
You should be able to remove almost everything from the yaml file except the
.build-id section.
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:394
+ if (error.Fail()) {
+ GetTarget().GetImages().Remove(module_sp);
+ module_sp.reset();
----------------
Under what circumstances can `GetSharedModule` fail, and still produce a valid
module_sp ?
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:397
+ }
+ if (!module_sp) {
+ // Try and find a module without specifying the UUID and only looking for
----------------
I am wondering if there's any way to narrow down the scope of this so we don't
do this second check for every module out there. If I understood the
description correctly, it should be sufficient to do this for linux minidumps
with PDB70 uuid records. Since PDBs aren't a thing on linux, and later breakpad
versions used used ElfBuildId records, this should be a pretty exact match for
the situation we want to capture. WDYT?
(Getting the platform of the minidump should be easy, for the UUID record type,
we might need a small refactor of the MinidumpParser class.)
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:416-435
+ if (!dmp_bytes.empty()) {
+ const auto mod_bytes = module_sp->GetUUID().GetBytes();
+ // We have a valid UUID in the minidump, now check if the module
+ // has a UUID. If the module doesn't have a UUID then we can call
+ // this a mtach
+ if (!mod_bytes.empty()) {
+ // We have a valid UUID in the module and in the minidump.
----------------
I don't find this piecewise checking helps readability. What would really help
IMO is if the booleans were inverted, so we check for the case when we *have* a
match, instead of when we haven't.
Maybe something like
```
bool module_matches = dmp_bytes.empty() || mod_bytes.empty() ||
(dmp_bytes.size() < mod_bytes.size() && mod_bytes.take_front(dmp_bytes.size())
== dmp_bytes);
if (!module_matches) { GetTarget().GetImages().Remove(module_sp);
module_sp.reset(); }
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60001/new/
https://reviews.llvm.org/D60001
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
