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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
