[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D33674#783861, @ravitheja wrote:

> Although a bit confusing, there is more flexibility for the user.I must also point out that the trace buffer available is not unlimited and there can be situations where a user might simultaneously want to trace newly spawned threads with a smaller buffer and trace an individual thread with perhaps a bigger buffer size.  Tracing all threads is definitely important coz the user might not want to separately start tracing on each new thread. Now the current design might be a bit confusing but I am willing to document it well with examples and uses cases.


I agree that having the ability to trace new threads from the moment they spawn is important. The confusion arises from the fact that we are treating "trace all threads" and "trace new threads" as a single concept. I think the cleanest solution for me would be to split those up, so that we would have three operations: (1) trace a specific thread, (2) trace new threads, (3) trace all threads. Then (1) and (2) can be combined in arbitrary ways, and (3) is mutually exclusive with anything else. It could be I am over-designing this though. I'd like to hear the opinion of anyone else that is reading this.



================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:277
+
+  llvm::DenseMap<lldb::tid_t, ProcessorTraceMonitorSP>
+      m_processor_trace_monitor;
----------------
ravitheja wrote:
> labath wrote:
> > I'd like to downgrade these to unique pointers to ProcessTraceMonitor. There's no reason for these to ever outlive or escape the process instance, so it's natural to say they are strongly owned by it. In other places where you use ProcessorTraceMonitorSP you can just use regular pointers or references (depending on whether they can be null or not).
> Hi, I don't see the advantage of changing to unique pointers ? coz when the process dies they will be destroyed anyhow, plus using shared pointers makes it easier for functions operating with the ProcessTraceMonitor to work.
It makes it clear that the Process is the owner of these objects (and not for example "sharing" them with anyone else). Plus you should use the simplest tool that gets the job done and unique_ptr is definitely simpler. So I'd reverse the question: If there is no need for using shared_ptr, why do it?

I disagree with the statement that it makes it harder for the functions to work. Please provide an example.


https://reviews.llvm.org/D33674