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

[Ravitheja Addepally via Phabricator via lldb-commits]

ravitheja added inline comments.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:611
+
+    if (ProcessorTraceMonitor::GetProcessTraceID() != LLDB_INVALID_UID) {
+      auto traceMonitor = ProcessorTraceMonitor::Create(
----------------
labath wrote:
> Every call to AddThread is followed by this block. Is there a reason we cannot just move it inside the AddThread function?
Yes that could be done.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:282
+  // same process user id.
+  llvm::DenseSet<lldb::tid_t> m_pt_traced_thread_group;
 };
----------------
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.


================
Comment at: unittests/Process/Linux/ProcessorTraceTest.cpp:41
+  bytes_read = ReadCylicBufferWrapper(
+      nullptr, sizeof(smaller_buffer), cyclic_buffer, sizeof(cyclic_buffer), 3,
+      0);
----------------
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 ?


https://reviews.llvm.org/D33674