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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Thanks for the feedback. I uploaded a new revision (incorporating some of the feedback, including an ELF test case)



================
Comment at: include/lldb/Core/Module.h:779
+  //------------------------------------------------------------------
+  void SetUUID(const lldb_private::UUID &uuid);
+
----------------
labath wrote:
> amccarth wrote:
> > 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?
> The UUID is not always know at construction time -- sometimes it can be, if you set it on the FileSpec you use to create the module, but you don't have to do that.
> 
> However, it is true that the UUID was constant during the lifetime of the Module, and this changes that, which is not ideal IMO.
> 
> It seems that in the only place you call this function, you already know the UUID. Would it be possible to do this work in the PlaceholderModule constructor (thereby avoiding any changes to the Module API) ?
Good point: I agree, it's a good idea to preserve the invariant that the module UUID does not change once it's set.

I'd not add more complexity & arguments to the placeholder module constructor though. Moreover, even if it's technically possible to reach into the protected data members from Module I'd like to avoid doing that since Module::m_uuid is closely tied to Module::m_mutex and Module::m_did_parse_uuid (the later now renamed to m_did_set_uuid).

I added a check and assert to prevent Module::SetUUID from overwriting an already set UUID. (Personally I'd would've liked to have the assert be a fastfail in all builds but I remember that is somewhat against the LLDB philosophy)




================
Comment at: source/Core/Module.cpp:341
+  m_uuid = uuid;
+  m_did_parse_uuid = true;
+}
----------------
amccarth wrote:
> 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.
Good point, but I'm afraid that would obscure the double-checked-locking in Module::GetUUID() so I'd rather not change it.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:99
 
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+  auto cv_record =
----------------
amccarth wrote:
> 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.
The goal is support the common CV records we expect to see from current toolchains and the contract for GetModuleUUID() is to either return a valid UUID or an empty one. 

Neither is an error: the CV record is optional and we also can't guarantee to handle every single CV record type.




https://reviews.llvm.org/D46292