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

[Walter via lldb-commits]

> - 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.

> - 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.

Il giorno gio 1 ott 2020 alle ore 07:12 Pavel Labath via Phabricator <
reviews at reviews.llvm.org> ha scritto:

> labath added a comment.
>
> I have two high-level remarks/questions about this:
>
> - 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.
> - 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?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D85705/new/
>
> https://reviews.llvm.org/D85705
>
>

-- 
- Walter Erquínigo Pezo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20201001/58e448db/attachment.html>