[Lldb-commits] [PATCH] D88769: [trace] Scaffold "thread trace dump instructions"

Tue Oct 6 06:22:08 PDT 2020

labath added a comment.

In D88769#2314164 <https://reviews.llvm.org/D88769#2314164>, @labath wrote:

> In D88769#2312482 <https://reviews.llvm.org/D88769#2312482>, @wallace wrote:
>
>> - Comments on the architecture of classes are in that diff
>
> Where would that be? I couldn't find anything that would answer my question from the previous comment?
>
> The reason I want to know that is that if each `TraceXXX` "plugin" is supposed to have/use/need a `ProcessTraceXXX` plugin, then I'd rather organize them such that they are closer together. OTOH, if ProcessTrace is supposed to work with all kinds of traces, then the current design makes perfect sense (though I am having trouble imagining how would that work).

I've now read the description of D88841 <https://reviews.llvm.org/D88841>, which kind of answers that. The reason I am not sure of this design is that I fear this will require a fairly big API surface between `ProcessTrace` and `Trace` classes in order to implement the more complicated commands like reverse-step. It also adds a bunch of dependency restrictions:

- `Trace` class (being a core class) should not know anything about ProcessTrace (a plugin) -- this bit could be removed if we make ProcessTrace a non-plugin
- `ProcessTrace` (being a generic plugin serving multiple traces) should not know anything about any specific trace plugin

OTOH, if ProcessTraceXXX is an internal affair of the TraceXXX plugin, then the two classes can freely cooperate (within reason), and can use any interfaces they see fit. And the TraceYYY plugin can use a completely different interface to communicate with its process class.

However, I don't think your design is bad, if you can pull it off, so feel free to carry on like you were planning. I'd just encourage you to, as you're working on it, to think about whether things can be made simpler with an different organization of things.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769