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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for spelling that out. However, it still does not sound like a convincing use case to me. Why would the user start to trace just one thread, only to later change his mind and trace the whole process instead. I'm not saying that can't happen, but it seems like something that we should discourage rather than try to support. The messaging I'd give to the user is: "you should trace the whole process unless you have a good reason not to". In case of a single threaded application, it won't make a difference, and if that app suddenly turns multithreaded, it will be traced as well. If he happens to have a single-thread trace running and later chooses to have a full process traced, he can always cancel that one and initiate a full trace instead.

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

> Just to make things clear, I will explain a use case
>
> Suppose if we are debugging an application where the main thread spawns a second new thread ->
>
>   int main() {
>   int i = 0;   // user starts tracing on main thread -> gets traceid 1
>   .....  // Some statements
>   i++; // Here he starts tracing the whole process -> gets traceid 2.
>           // Now traceid=2 represents the tracing instance on the 
>           // whole process, meaning all new threads spawned in 
>           //  the process will be traced with this id. Although the 
>           //main thread is already being traced hence 
>           // will not be a part of traceid =2


Yes, and this is what makes it confusing to me. At this point trace 2 will not be tracing anything, even though in the comments and the code you use the terms like "process trace" and "trace all threads".

Another thing I find confusing is what would happen if the events were reversed:

1. Start tracing the whole process
2. Ok. Traceid = 1
3. Start tracing thread 47
4. Error. Thread 47 is already traced. (???)

So now tracing a single thread is forbidden if you already have a process trace running, but starting a process trace is ok even if there is a thread trace already. This seems  more complicated to grasp than "you cannot trace the full process as you already trace one of it's threads".



================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2644
+  if (iter != m_processor_trace_monitor.end())
+    error = StopTrace(iter->second->GetTraceID(), thread);
+
----------------
ravitheja wrote:
> labath wrote:
> > Should you delete the iterator from the map after this? In fact, the mere act of deleting the iterator should probably be what triggers the trace stop, in line with RAII principles.
> Yes, but the thing is i have to update the Set of threads 
> 
> ```m_pt_traced_thread_group
> ```
> which is tracking how many threads are being traced under the process trace id. The StopTrace calls StopProcessorTracingOnThread eventually which deletes the element from the various containers. I can directly call StopProcessorTracingOnThread instead of StopTrace.
That makes it slightly better, but I still find extremely verbose -- I have to read through 50 lines of error checks, only to execute two lines of code:
```
  m_pt_traced_thread_group.erase(thread);
  m_processor_trace_monitor.erase(iter);
```
It looks like there is some problem with abstractions there.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:158
+    LLDB_LOG(log, "ProcessorTrace failed to open Config file");
+    error.SetError(FileNotFound, eErrorTypePOSIX);
+    return error;
----------------
ravitheja wrote:
> labath wrote:
> > ravitheja wrote:
> > > labath wrote:
> > > > eErrorTypePOSIX is used for errno error values. Please don't try to pass your invented error codes as these.
> > > Yes I did not want to use eErrorTypePOSIX but when transitioning from Status to llvm::Error, the m_code is only retained for eErrorTypePOSIX else its not retained.
> > That's a good point. When I wrote the conversion function, there was no use case for this --  I think you're the first person who actually want's to use the error codes in some meaningful way.
> > 
> > What is your further plan for these error codes? Judging by the state of the D33035 you won't be able to use them to display the error messages to the user?
> > 
> > If you still want to preserve the error codes, we can definitely make this happen. Actually, llvm::Error makes this even easier, as it allows you to define new error categories in a distributed way. Frankly, I think the your use of the "generic" error category with custom error codes is a bit of a hack. I think the intended usage of the Status class was to define your own ErrorType enum value and tag your errors with that (but that does not scale well to many different sources of error codes).
> My plan is to perhaps implement a way to pass error strings along with the error packets, so that the tool in D33035 can directly use those strings. I guess then I can just use the eErrorTypeGeneric . 
> 
> So keeping that in mind I can just remove the error codes and replace them everywhere with the strings ? Although the tool will not work unless the error strings are transported .
> 
Ok, then let's drop the error enum, and use just strings for now(?) For what it's worth, at least it makes this consisted with the rest of the code base.


https://reviews.llvm.org/D33674