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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:282
+  // same process user id.
+  llvm::DenseSet<lldb::tid_t> m_pt_traced_thread_group;
 };
----------------
ravitheja wrote:
> labath wrote:
> > I am confused about the purpose of this member variable. As far as I can tell, it will always contain *either* all of the threads in the process *or* none of them. Is there a situation where this is not the case?
> Yes there can be situations like that, for e.g if a user starts tracing on an individual thread followed by tracing the whole process (meaning in two separate start trace calls) then this Set will contain the thread ids of all the threads except the one on which individual tracing was started. This can be extended further.
Interesting... Is that actually useful for something? I find the semantic of "trace all threads in the process except those that happen to be traced already" a bit confusing. 

If you have a use for it in mind then fine, but otherwise, I'd like go for the simpler option of failing the request to trace the whole process if some individual threads are traced already. "you can either trace the whole process *OR* any number of individual threads" sounds much easier to understand. Otherwise, you'd have to think about corner cases like "What if the individual thread trace is stopped but the process trace request remains?" Do you then automatically start tracing the remaining thread based on the previous "whole process trace" request, and so on...


================
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:
> > 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).


================
Comment at: unittests/Process/Linux/ProcessorTraceTest.cpp:41
+  bytes_read = ReadCylicBufferWrapper(
+      nullptr, sizeof(smaller_buffer), cyclic_buffer, sizeof(cyclic_buffer), 3,
+      0);
----------------
ravitheja wrote:
> labath wrote:
> > I don't understand why you're testing these. Why would anyone create an ArrayRef pointing to null? If you get handed an array ref, you should be free to assume it points to valid data.
> Well I see that ArrayRef can be constructed with a nullptr and a positive size, like 
> 
> ```
> ArrayRef(nullptr, 5);
> ```
> hence I added the that check . If u want I can remove these checks ?
Yes, please. That was never the intention of array ref -- it should be used in a way that in always refers to valid memory. Making that assumption will allow you to cut the size of your code in half (both the test and the implementation)


https://reviews.llvm.org/D33674