[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 inline comments.


================
Comment at: include/lldb/Core/Module.h:779
+  //------------------------------------------------------------------
+  void SetUUID(const lldb_private::UUID &uuid);
+
----------------
labath wrote:
> lemo wrote:
> > 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)
> > 
> > 
> Guarding the invariant with an assert is better than nothing, but even better is arranging stuff so that the invariant cannot be broken in the first place. And this feels like the sort of thing that should be possible to make safe by design.
> 
> How about adding a special (protected?) Module constructor, which takes a UUID argument and uses it to initialize the m_uuid field. This way, it is impossible by design to change the UUID mid-flight, and you still don't have to reach into the protected Module fields from the other class (we could even make them private if we want to).
Trying to make sure that the invariant can't be invalidated is a high bar considering that most Module state is protected. But I agree with the sentiment and here's a new iteration:

1. Made Module::SetUUID() protected
    (note that the m_uuid change is conditional, not just protected by an lldbassert)
2. PlaceholderModule constructor is where SetUUID is called from

I'm hesitant to add another Module constructor: that class already has 2 public + 1 private constructors (even using a delegating constructor, the addition of a new protected constructor questionable). But if you still prefer that option better I'll make the change.




https://reviews.llvm.org/D46292