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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Second round of comments. I still see a lot of places with redundant checks for what appear to be operational invariants. I'd like to get those cleared up to make the code flow better.



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


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:621
+        LLDB_LOG(log, "{0} error in create m_code {1} m_string {2}",
+                 __FUNCTION__, error2.GetError(), error2.AsCString());
+      }
----------------
LLDB_LOG will already print the function name (same thing everywhere else you use `__FUNCTION__`), so you don't need to write it out yourself. Also the AsCString is unnecessary, as Status has a pretty-printer.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2534
+                                   size_t offset) {
+  TraceOptions trace_options;
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));
----------------
unused variable


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2627
+  if (!traceMonitor) {
+    Status error2(traceMonitor.takeError());
+    LLDB_LOG(log, "{0} error in create m_code {1} m_string {2}", __FUNCTION__,
----------------
As of r305462 you can write `error = traceMonitor.takeError()`, so you shouldn't need the extra variable.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2644
+  if (iter != m_processor_trace_monitor.end())
+    error = StopTrace(iter->second->GetTraceID(), thread);
+
----------------
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.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2678
+  Status error;
+  if (ProcessorTraceMonitor::GetProcessTraceID() == LLDB_INVALID_UID) {
+    error.SetError(ProcessNotBeingTraced, eErrorTypeGeneric);
----------------
This is dead code. The caller has already checked this condition. This should be at most an assertion.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:263
+  // is sufficient to obtain the actual ProcessorTrace instance.
+  ProcessorTraceMonitorSP LookupProcessorTraceInstance(lldb::user_id_t traceid,
+                                                       lldb::tid_t thread,
----------------
ErrorOr<T> is a better way to return a value *or* an error. Although, in this case, it sounds like you're really only interested in the found-or-not-found distinction, so you could just drop the Status argument and let a null return value signify an error.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:277
+
+  llvm::DenseMap<lldb::tid_t, ProcessorTraceMonitorSP>
+      m_processor_trace_monitor;
----------------
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).


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


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:158
+    LLDB_LOG(log, "ProcessorTrace failed to open Config file");
+    error.SetError(FileNotFound, eErrorTypePOSIX);
+    return error;
----------------
eErrorTypePOSIX is used for errno error values. Please don't try to pass your invented error codes as these.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:321
+  if (!pt_monitor_sp) {
+    LLDB_LOG(log, "NativeProcessLinux {0}", "Alloc failed");
+    error.SetError(AllocFailed, eErrorTypeGeneric);
----------------
dead code


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:412
+#ifndef PERF_ATTR_SIZE_VER5
+  LLDB_LOG(log, "ProcessorTrace {0} Not supported ", __FUNCTION__);
+  error.SetError(PerfEventNotSupported, eErrorTypeGeneric);
----------------
Just put llvm_unreachable here. There shouldn't be any way to construct this object if tracing is not supported.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:480
+  if (base == nullptr) {
+    LLDB_LOG(log, "ProcessorTrace {0} Null pointer ", __FUNCTION__);
+    error.SetError(NullPointer, eErrorTypeGeneric);
----------------
Dead code? The construction of the object shouldn't have succeeded if you failed to allocate the memory or initialize the trace.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:20
+
+namespace lldb_private {
+
----------------
All the other files in the folder are in an additional process_linux namespace. This file should go there as well.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:70
+  void *m_mmap_data;
+  void *m_mmap_aux;
+  void *m_mmap_base;
----------------
Make this MutableArrayRef<uint8_t>. Then you don't need the ugly cast when calling readcyclicbuffer, nor the `GetAuxBufferSize` function (as it just becomes `m_mmap_aux.size()`). Same for `m_mmap_data`.


================
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;
+
----------------
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).


================
Comment at: unittests/Linux/ProcessorTraceTest.cpp:1
+//===-- ProcessorTraceTest.cpp -------------------------------- -*- C++ -*-===//
+//
----------------
You seem to have forgotten to delete this.


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


https://reviews.llvm.org/D33674