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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:390-391
+    // Try and find a module with a full UUID that matches. This function will
+    // add the module to the target if it finds one, so we might need to remove
+    // it if there is an error.
     lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, &error);
----------------
This comment (the part about removing in case of error) is no longer true. Please delete it.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:396-397
+      // the file given a basename. We then will look for a partial UUID match
+      // if we find any matches. This function will add the module to the
+      // target if it finds one, so we might need to remove it if there is an
+      // error.
----------------
Same here.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:401
+      basename_module_spec.GetUUID().Clear();
+      basename_module_spec.GetFileSpec().GetDirectory().Clear();
+      module_sp = GetTarget().GetSharedModule(basename_module_spec, &error);
----------------
What will happen if you just delete this line? Based on my experiments, I believe we should still end up looking through the exec-search-paths list. However, the test does not seem to be modifying this setting, so it's not clear to me how it is even finding the binary in the first place. What's the mechanism you're relying on for the lookup in the test?


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:407-408
+        const auto mod_bytes = module_sp->GetUUID().GetBytes();
+        // We have a match if either UUID is empty or as long as the actual
+        // UUID starts with the minidump UUID
+        const bool match = dmp_bytes.empty() || mod_bytes.empty() ||
----------------
These comments no longer make sense like this as the checking happens all at once. Please rephrase them. Maybe something like "We consider the module to be a match if the minidump UUID is a prefix of the module UUID, or if one of the uuid's is empty". ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60001/new/

https://reviews.llvm.org/D60001