[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

Thu Sep 3 06:39:12 PDT 2020

labath added a comment.

In D86670#2244876 <https://reviews.llvm.org/D86670#2244876>, @clayborg wrote:

> In D86670#2244092 <https://reviews.llvm.org/D86670#2244092>, @labath wrote:
>
>> I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.
>
> The idea was to iterate on the design of the trace feature using these patches and have the discussion here. The idea is one trace file can create N individual targets. Each target can be independent and the threads contained in each belong to each target.  "trace dump" when no args are supplied it could maybe only dump the currently selected target to start with? I am not sure what the right semantics are, but that shouldn't stop us with proceeding with these patches IMHO. It will be easy to modify as we evolve if we do want to have target groups, but I don't think it is required for these patches. The target group is a much higher level design that can affect many things and could be used in this case, but isn't required. If a trace plug-in needs to iterate over all of the targets this can be achieved by using the debugger to grab all targets and iterate over them, so no real group is required.
>
> So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

In D86670#2244880 <https://reviews.llvm.org/D86670#2244880>, @wallace wrote:

> I agree with that Greg said.

I don't think a review like this is good for a broader discussion because it obscures the woods by lots of individual trees. Like, I could write a page about what I don't like about the `Target::SetTrace` method, but I don't think that's the most important thing to sort out right now. Before going into the details of how trace dump options should be passed around, I think we should have some understanding and agreement about the feature. Like a short introduction to the problem, mentioning what is in scope for the project, what isn't; what are the new user-facing commands/apis being added, new concepts; and finally a very brief overview of new important new lldb internal classes. It doesn't have to be as polished as the memory tagging <http://lists.llvm.org/pipermail/lldb-dev/2020-August/016402.html> RFC (that was probably the best rfc lldb has ever had, and I feel guilty for not responding to it), something simpler will also do. I'm sure the two of you have talked about this a lot, and already have something to say about all of the points above, but I don't know how many other people do. And without that its hard to evaluate whether any particular patch is good or not, and reduces the comments to "i think this should be a unique pointer".

Even if noone responds to the rfc, it's not a waste because anyone can go back to it for details when reviewing any particular patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670