[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

The big picture here is fine.

I see a couple opportunities in the details, but I won't block on them.



================
Comment at: include/lldb/Core/Module.h:779
+  //------------------------------------------------------------------
+  void SetUUID(const lldb_private::UUID &uuid);
+
----------------
Was there no SetUUID before because the UUID was considered immutable?  I wonder if that's worth keeping.  Is the UUID always known at construction time?


================
Comment at: source/Core/Module.cpp:341
+  m_uuid = uuid;
+  m_did_parse_uuid = true;
+}
----------------
I wonder if the lazy UUID lookup in Module::GetUUID should call this to avoid code duplication.  I know it's not a lot of code, but it involves a mutex and an atomic flag, so it might be nice to always have this update in just one place.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:99
 
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+  auto cv_record =
----------------
I wonder if this should return an `Expected<UUID>`.  The function has multiple ways it can fail, in which case the error info is lost and we silently return an empty UUID.


https://reviews.llvm.org/D46292