[PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Pavel Labath via llvm-commits]

On 01/10/2020 20:57, Walter wrote:
>> - I am surprised that it was not necessary to create a special process
> plugin for this purpose. I have a feeling one will be necessary sooner
> or later because of the need to customize the step/continue/etc. flows.
> Currently, this will probably produce very bizarre if one tries to
> execute those commands. The reason I'm bringing this up is because of
> the `Target::GetTrace` method being added in the next patch. If there is
> a trace-specific process class, then it might make sense for this class
> to hold the trace object instead of adding the GetTrace method on every
> Target object out there even though most targets will have that null. I
> don't know if that will really be the case, but I think it's something
> worth keeping in mind as you work on the subsequent patches.
> 
> Very good remark. Probably we'll end up doing that when we start
> implementing reverse debugging. The tricky thing we'll need to solve in
> the future is seamlessly transition between a trace and a live process.
> For example, when reverse debugging a live process, you might be stopped
> at a breakpoint in the trace, then you do "continue", it reaches the end
> of the trace, and then it resumes the live process. I still haven't
> decided what we'll propose for this, but probably we'll have to change a
> lot of the current code to make that happen nicely.

Yes, that will be interesting. Even more interesting will be the
question of how to communicate to the user the fact that even though we
have "unwound" the PC to the previous location, variables and memory
still contain the "live" values. Particularly, once we support rr-style
reverse debugging which will "unwind" memory as well..

> 
>> - I am puzzled by the TraceXXX vs. TraceXXXSettingsParser duality. The
> two classes are very tightly coupled, so it's not clear to me what is
> the advantage of separating it out this way (one could just move all the
> relevant methods directly into the Trace class. What's the reason for
> this design?
> 
> Well, there's definitely coupling, but there are two reasons. One is
> that the Trace of a live process won't need any parsing. Parsing is only
> used when loading a trace from a json file. That makes parsing one of
> the two main ways to create a Trace. And besides, I think that the
> single responsibility principle is good to follow. The Parser class does
> the parsing, and the Trace class holds an actual trace.

Yeah, I'm all for the single responsibility principle, but the way it is
implemented gives me pause. Like, if the Parser is supposed to "create"
the Trace, then why does the code do it the other way around
(Trace::CreateParser)? For one, that means that it will be possible to
"parse" (by calling CreateParser()->...) any trace that you can get your
hands on, even that of a live process. Ideally, the Parser would be the
one creating a Trace object (in it's Parse function, or something), and
after the Trace is created, it should no longer be possible to obtain
the parser. All of this could be encapsulated in the "create_callback"
that the plugin registers, and so the fact that there is a parser class
involved would be completely unknown to the rest of the code. And then
there could be a different create_callback which would create a Trace
object suitable for tracing a live process...

pl