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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:738-742
+    StartProcessorTracing(tid, m_pt_process_config, traceMonitor);
+    if (traceMonitor.get() != nullptr) {
+      traceMonitor->setUserID(m_pt_process_uid);
+      m_pt_traced_thread_group.insert(tid);
+    }
----------------
This function returns a `Status`.  Can't we assume that `traceMonitor` will be valid if and only if the returned `Status` is a success condition?  And if it's not a success condition, shouldn't you log the error?


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2517-2522
+  auto iter = m_processor_trace_monitor.begin();
+  for (; iter != m_processor_trace_monitor.end(); iter++) {
+    if (uid == iter->second->getUID() &&
+        (thread == iter->first || thread == LLDB_INVALID_THREAD_ID))
+      return iter->second;
+  }
----------------
Please use a range based for here.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2626
+  lldb::user_id_t uid = LLDB_INVALID_UID;
+  if (config.getType() == lldb::TraceType::eTraceTypeProcessorTrace) {
+    if (m_pt_process_uid == LLDB_INVALID_UID) {
----------------
Can you do an early return here if the condition is not true?


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2627
+  if (config.getType() == lldb::TraceType::eTraceTypeProcessorTrace) {
+    if (m_pt_process_uid == LLDB_INVALID_UID) {
+      m_pt_process_config = config;
----------------
And here


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2638-2642
+        llvm::StringRef intel_custom_params_key("intel-pt");
+        llvm::StringRef cpu_family_key("cpu_family");
+        llvm::StringRef cpu_model_key("cpu_model");
+        llvm::StringRef cpu_stepping_key("cpu_stepping");
+        llvm::StringRef cpu_vendor_intel_key("cpu_vendor_intel");
----------------
Nothing specifically wrong with this, but it's implicitly convertible, so if you like it you can simply just pass these strings into the `AddIntegerItem` function as string literals.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2717
+  m_processor_trace_monitor.insert(
+      std::pair<lldb::tid_t, ProcessorTraceMonitorSP>(thread, traceMonitor));
+
----------------
`std::make_pair(thread, traceMonitor)` might allow this to fit on one line.  `m_processor_trace_monitor.emplace(thread, traceMonitor);` almost definitely would.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2744
+  error = getCPUType(cpu_family, model, stepping, vendor_id);
+  if (error.Success()) {
+    StructuredData::Dictionary *intel_params = new StructuredData::Dictionary();
----------------
Early return here.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2745
+  if (error.Success()) {
+    StructuredData::Dictionary *intel_params = new StructuredData::Dictionary();
+
----------------
Who is responsible for freeing this memory?


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2794-2795
+
+  switch (trace_type) {
+  case lldb::TraceType::eTraceTypeProcessorTrace:
+    error =
----------------
Seems like this would be more straightforward to just say:

```
if (trace_type != eTraceTypeProcessorTrace)
  return NativeProcessProtocol::StartTrace(config, error);
```


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2870-2871
+                                                       lldb::tid_t thread) {
+  Status error;
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));
+
----------------
Bunch of opportunities in this function for early returning on error conditions.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2876-2877
+    bool uid_found = false;
+    for (; iter != m_processor_trace_monitor.end(); iter++) {
+      if (iter->second->getUID() == uid) {
+        // Stopping a trace instance for an individual thread
----------------
Range based for with an inverted conditional and early continue inside the loop.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2931-2938
+static uint64_t ComputeFloorLog2(uint64_t input) {
+  uint64_t prev = input;
+  while ((input & (input - 1)) != 0) {
+    input &= (input - 1);
+    prev = input;
+  }
+  return prev;
----------------
Delete and replace callsites with `llvm::Log2_64`


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2940-2943
+Status NativeProcessLinux::ProcessorTraceMonitor::StartTrace(
+    lldb::pid_t pid, lldb::tid_t tid, TraceOptions &config) {
+  Status error;
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));
----------------
What happens if you call this function twice in a row?  Or from different threads?  Is that something you care about?


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3009-3010
+  errno = 0;
+  m_mmap_base =
+      mmap(NULL, (metabufsize + page_size), PROT_WRITE, MAP_SHARED, m_fd, 0);
+  if (m_mmap_base == MAP_FAILED) {
----------------
Perhaps you can use `llvm::MemoryBuffer` here?  It mmaps internally


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3046
+    uint64_t &cpu_family, uint64_t &model, uint64_t &stepping,
+    std::string &vendor_id) {
+  Status error;
----------------
It looks to me like `vendor_id` param could be a `StringRef&`.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:156
+  // ---------------------------------------------------------------------
+  static size_t ReadCyclicBuffer(void *buf, size_t buf_size, void *cyc_buf,
+                                 size_t cyc_buf_size, size_t cyc_start,
----------------
labath wrote:
> How about ` void ReadCyclicBuffer(ArrayRef<uint8_t> buffer, size_t position, MutableArrayRef<uint8_t> &dest)`
Better yet, drop the `position` argument entirely.  `ReadCyclicBuffer(src, N, dest)` is equivalent to `ReadCyclicBuffer(src.drop_front(N), dest);`


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:319-320
+    int m_fd;
+    void *m_mmap_data;
+    void *m_mmap_aux;
+    void *m_mmap_base;
----------------
Instead of having 
```
void *m_mmap_data;
void *m_mmap_aux;
void *m_mmap_base;
```

and then doing some ugly casting every time someone calls `getAuxBufferSize` or `getDataBufferSize`, instead just write:

```
MutableArrayRef<uint8_t> m_mmap_data;
MutableArrayRef<uint8_t> m_mmap_aux;
```

and initializing these array refs up front using the size from the header.  This way you don't have to worry about anyone using the buffers incorrectly, and the `ReadPerfTraceAux(size_t offset)` function no longer needs to return a `Status`, but instead it can just return `MutableArrayRef<uint8_t>` since nothing can go wrong.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:397-398
+
+  std::unordered_map<lldb::tid_t, ProcessorTraceMonitorSP>
+      m_processor_trace_monitor;
+
----------------
Can you use an `llvm::DenseMap` here?  


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:400-402
+  // Set for tracking threads being traced under
+  // same process user id.
+  std::set<lldb::tid_t> m_pt_traced_thread_group;
----------------
And an `llvm::DenseSet` here?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1319-1320
 
-  for (size_t i = 0; i < buf.size(); ++i)
+  for (uint64_t i = 0; i < buf.size(); ++i)
     response.PutHex8(buf[i]);
 
----------------
```
for (const auto &I : buf)
  response.PutHex8(I);
```


https://reviews.llvm.org/D33674