[Lldb-commits] [PATCH] D60001: Allow partial UUID matching in Minidump core file plug-in

[Pavel Labath via Phabricator via lldb-commits]

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