[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

Fri Dec 21 06:48:49 PST 2018

labath added a comment.

Some interesting ideas here. I like some of them, but I have reservations about others. I'm going to go through them one by one.

In D55142#1333077 <https://reviews.llvm.org/D55142#1333077>, @zturner wrote:

> I thought about what my "ideal" design would look like.  I'm not suggesting anyone should actually go off and do this, but since we're brainstorming design anyway, it doesn't hurt to consider what an optimal one should look like (and for the record, there may be things I haven't considered that cuase this design to not work or be suboptimal or not work at all).  Just throwing it out there.
>
> 1. Rename `ObjectFile` to `ModuleFile` to indicate that a) it makes no sense without a Module, and b) it doesn't make sense for symbol-only files.

This is the one I have most problems with, actually, as it goes against my "ideal" design. I would actually like to have ObjectFile (or whatever it's called) be completely independent of the Module class. The motivation for this is, unsurprisingly, lldb-server, which needs to have access to very lightweight info about an object file (on the level of: UUID, architecture, maybe entry point, etc.), but nothing like the fancy interface the Module class provides. So ideally, I'd like for the ObjectFile to live on a completely different level, so that it can be used in lldb-server without pulling in Module and everything that goes with it. So, I don't agree with your (a) goal here. The (b) goal seems completely reasonable though.

> 
> 
> 2. Rename `SymbolVendor` to `SymbolFileLocator`.  Mostly cosmetic, but more clearly differentiates the responsibilities of the two classes.  (e.g. wouldn't you expect SymbolFiles to be able to "vend" symbols)?

+1000. It has always bugged me that SymbolVendor has all these methods that don't do anything and just forward their arguments to the SymbolFile (after performing a dozen null checks). I think it would be better if the symbol file interface was called directly. If some SymbolVendor/SymbolLocator ever wants to take an existing SymbolFile and alter its behavior slightly, it can always use the composition pattern and return it's own delegating SymbolFile object instead.

> 
> 
> 3. `SymbolFileLocator` returns a `unique_ptr<SymbolFile>`.  Nothing more.  It has no other methods.

That seems to be the logical consequence of the previous item.

> 
> 
> 4. `Target` contains a `vector<unique_ptr<SymbolFileLocator>>` as well as methods `AddSymbolFileLocator(unique_ptr<SymbolFileLocator>)` and `unique_ptr<SymbolFile> LocateSymboleFile(Module&)`.  The former gives things like the Platform or Process the ability to customize the behavior.  The latter, when called, will iterate through the list of `SymbolFileLocators`, trying to find one that works.

I am not sure if `Target` is a good receptacle for this functionality, because one of lldb's goals (which seems completely reasonable to me) is to be able to reuse the same Module instance if the module happens to be loaded in multiple targets. If the target is responsible for locating the symbol files, then this opens the door for one target to unwittingly alter the symbols of another target. One way to get out of this would be to say that targets share a Module only if they (independently) choose the same symbol file. This might enable us to also solve the current problem, where "target symbols add" executed in one debugger, alters the behavior of another debugger instance. Another would be to store the list of SymbolLocators in a more global place. This would actually be pretty similar to what we have now.

> 
> 
> 5. `Module` contains a `unique_ptr<SymbolFile>`.  When `GetSymbolFile()` is called for the first time, it calls `Target::LocateSymbolFile(*this)` and updates its member variable.

Nothing to argue about here.

> 
> 
> 6. `ModuleFile` is updated to support "capabilities" much like `SymbolFile` is today.  One of these capabilities can be "has unwind info".  Likewise, "has unwind info is added to `SymbolFile` capabilities as well.  `Module` can then select the best provider of unwind info using this approach.

I don't believe a single bit is enough to describe the suitability of unwind info, as the choice of unwind strategy may vary from function to function and depend on other circumstances too (e.g., whether we are stopped at a call site). Instead, I'd try to have much more granular capabilities. E.g., one of those capabilities might be "has function boundaries". Then if either entity provides the information about function boundaries, the Module can create an InstructionEmulationUnwinder, which uses these boundaries to create emulated unwind plans. And even this capability might not be just a "bit", but rather a pooling of function boundary information gathered from various sources.

> And finally
> 
> 7. `SymbolFile` now stores a variable `m_module` instead of `m_obj_file` since this is usually what the `SymbolFile` requires anyway, and it also eliminates the dependency on `SymbolFile` being backed by an `ObjectFile`.  Note that in this case, a `SymbolFileBreakpad` for example would have its `m_module` variable set to the actual module that its providing symbols for.

Sounds good.

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

https://reviews.llvm.org/D55142