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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like the direction this is going in, but I see places for more cleanup. The main three items are:

- making destruction cleaner
- avoiding global variables
- making ReadCyclicBuffer readable

the rest are just general nits.



================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2551
+    if (!process_config) {
+      error = process_config.takeError();
+      return error;
----------------
`return process_config.takeError()` is two lines shorter


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2568
+lldb::user_id_t
+NativeProcessLinux::StartTracingAllThreads(const TraceOptions &config,
+                                           Status &error) {
----------------
This is still called trace*ALL*threads


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2640
+  if (iter == m_processor_trace_monitor.end())
+  {
+    error.SetErrorString("tracing not active for this thread");
----------------
the formatting of the bracket is wrong


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2726
+
+  LLDB_LOG(log, "UID - {0} , Thread -{1}", traceid, thread);
+
----------------
s/UID/traceid/


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:114
+  Status StopTrace(lldb::user_id_t traceid,
+                   lldb::tid_t thread = LLDB_INVALID_THREAD_ID) override;
+
----------------
having default argument values on virtual functions is dangerous. Please remove these. AFAIK, noone is going to be calling these through a NativeProcessLinux pointer anyway.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:33
+
+uint64_t ProcessorTraceMonitor::GetAuxBufferSize() const {
+  return m_aux.size();
----------------
Are these functions still useful?


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:129
+    LLDB_LOG(log, "failed to open Config file");
+    error.SetErrorString("file not found");
+    return error;
----------------
You can consider `return ret.getError()`, which will probably give you a more detailed error message.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:148
+  if (error.Fail()) {
+    LLDB_LOG(log, "Status in custom config {0}", error.AsCString());
+
----------------
drop AsCString()


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:243
+    LLDB_LOG(log, "{0}:{1}:{2}:{3}", cpu_family, model, stepping,
+             vendor_id.c_str());
+
----------------
drop c_str()


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:307
+
+Status ProcessorTraceMonitor::Destroy() {
+  Status error;
----------------
I'd like this work to be done in the destructor. Just like nobody should see a constructed-but-not-yet-initialized object, so the destroyed-but-not-destructed state should not exist. Then you don't need to worry about setting the state of the object to some sane values.

In fact, if you utilize std::unique_ptr capabilities, then you may not even need a destructor at all. Just define a class
```
class munmap_delete {
  size_t m_length;

public:
  munmap_delete(size_t length) : m_length(length) {}
  void operator()(void *ptr) { munmap(ptr, m_length); }
};
```

and then you can just declare  `
`std::unique_ptr<perf_event_mmap_page, munmap_delete> mmap_base(mmap_result, munmap_delete(mmap_size))`

This way, the allocation and deallocation code are close together, and you don't need to write yourself `// we allocated extra page` reminders.

Since we store m_aux in a ArrayRef already, this would require duplicating the pointer in the unique_ptr member. I am fine with that, but if you want to avoid it you can just have a getAux() member function which sythesizes the arrayref on demand.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:358
+#else
+  if (buffer.empty()) {
+    LLDB_LOG(log, "Empty buffer ");
----------------
ReadCyclicBuffer will handle the case of a zero-sized buffer gracefully, right? You don't need to check the size at every level.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:438
+  }
+  buffer = llvm::MutableArrayRef<uint8_t>(buffer.data(),
+                                          (buffer.size() - bytes_remaining));
----------------
`buffer = buffer.drop_back(bytes_remaining) ` ?


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:475
+
+  if (dst.size() >= size_to_read) {
+    if (offset >= firstpart.size()) {
----------------
I think there are waaaaaay too many nested conditionals here. Let's try this implementation for a change:
```
SmallVector<ArrayRef<uint8_t>, 2> parts{ src.slice(src_cyc_index), src.drop_back(src_cyc_index)};
if (offset >= parts[0].size()) {
  offset -= parts[0].size();
  parts.drop_front();
}
parts[0] = parts[0].drop_front(offset);
```
That takes care of offset. Now on to the copy.
```
dst = dst.take_front(size_to_read);
auto next = dst.begin();
for(auto part: parts) {
  size_t chunk_size = std::min(size_to_read, part.size());
  next = std::copy_n(part.begin(), chunk_size, next);
  size_to_read -= chunk_size;
}
```
Much easier to reason about and less than half the size of the original implementation.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:79
+  // Trace id of trace instance corresponding to the process.
+  static lldb::user_id_t m_pt_process_traceid;
+
----------------
labath wrote:
> Instead of global variables (we may want to debug more than one process eventually), we could have a member variable `std::pair<user_id_t, TraceOptions> m_process_trace;` (you can have a special class for that if you don't like .first and .second everywhere).
I guess I wasn't too clear about this, but I meant a member variable in the Process class. This solves the "multiple process" issue, but creates a couple of other ones. All the call sites are now more complicated, and you have to worry about managing the lifetime of the entries in this map. If this was a Process member variable, those would be handled automatically (and it makes more sense, since it is the process class that owns these traces instances).


https://reviews.llvm.org/D33674