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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

First batch of comments from me, I think I will have some more once I gain more insight into this. The main one is about the constructor/initialize, destructor/destroy duality, which we should abolish. The rest is mostly stylistic.



================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:618
+                            traceMonitor);
+      if (traceMonitor.get() != nullptr) {
+        traceMonitor->setUserID(m_pt_process_uid);
----------------
`if(traceMonitor)`


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:619
+      if (traceMonitor.get() != nullptr) {
+        traceMonitor->setUserID(m_pt_process_uid);
+        m_pt_traced_thread_group.insert(thread_sp->GetID());
----------------
As far as, I can tell, every call to `StartProcessorTracing` is followed by setUserId. Can we move the logic of setting the id inside the `StartProcessorTracing` function


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2483
+template <typename T>
+static Status ReadFromFile(const char *filename, T &read,
+                          std::ios_base::fmtflags mode = (std::ios_base::hex |
----------------
Please use llvm file API for this (see how `getProcFile` works -- it's just a very thin wrapper around the llvm functions)


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2510
+  if (thread == LLDB_INVALID_THREAD_ID && uid == m_pt_process_uid) {
+    if (log)
+      log->Printf("NativeProcessLinux %s_thread not specified: %" PRIu64,
----------------
Please use LLDB_LOG (here and everywhere else in this patch)


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2847
+
+Status NativeProcessLinux::StopProcessorTracingOnProcess(lldb::user_id_t uid) {
+  Status ret_error;
----------------
You are  calling this function with uid == m_pt_process_uid, which means this parameter is redundant, and leads to redundant sanity checks. Please remove it.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2965
+
+  config.setTraceBufferSize(bufsize);
+  config.setMetaDataBufferSize(metabufsize);
----------------
You are modifying the config, but the caller is ignoring these modifications. If you don't need these,  we could remove the modifications, make the config argument `const` and end up with a much cleaner interface.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3056
+
+  auto BufferOrError = getProcFile("cpuinfo");
+  if (!BufferOrError) {
----------------
I don't see a getProcFile overload with this signature (although I am not opposed to adding one). Are you sure this compiles?


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3064
+    std::tie(Line, Rest) = Rest.split('\n');
+    {
+      SmallVector<StringRef, 2> columns;
----------------
Is this scope necessary? I find it just confuses the reader...


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3070
+        continue; // continue searching
+
+      if (log)
----------------
add: `columns[1] = columns[1].trim();`
Then you can skip calling trim everywhere else.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3075
+
+      if ((columns[0].find("cpu family") != std::string::npos) &&
+          columns[1].trim(" ").getAsInteger(10, cpu_family))
----------------
columns[0].contains(foo)


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3100
+          (stepping != static_cast<uint64_t>(-1)) && (!vendor_id.empty()))
+        break; // we are done
+    }
----------------
return here. Then you can avoid duplicating the check outside the loop.


================
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,
----------------
How about ` void ReadCyclicBuffer(ArrayRef<uint8_t> buffer, size_t position, MutableArrayRef<uint8_t> &dest)`


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:160
+  enum PTErrorCode {
+    FileNotFound = 0x23,
+    ThreadNotTraced,
----------------
This seems suspicious. How did you come to choose this number?


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:317
+  // ---------------------------------------------------------------------
+  class ProcessorTraceMonitor {
+    int m_fd;
----------------
Please move this into a separate file. NativeProcessLinux.cpp is big enough as it is.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:331
+  public:
+    ProcessorTraceMonitor()
+        : m_fd(-1), m_mmap_data(nullptr), m_mmap_aux(nullptr),
----------------
This would be much cleaner, if we could avoid creating an "invalid trace" object. The way that's usually done is to have a factory function, returning say llvm::Expected<std::unique_ptr<T>> (or even llvm::Expected<T> if you are able to make your type movable easily). The factory function then does all the operations that can fail and returns an error in this case. Once it has everything ready, it just calls the constructor, forwarding all the necessary arguments. The constructor can perform any additional non-fallable initialization, if necessary.

This way all the code inside and around the object can assume that once it has an object of this type, it is a valid object. We don't have many examples of this pattern, but you can for example look at MinidumpParser::Create or the code in (D32930) for guidance. I am also working on a patch to switch NativeProcess creation to use this pattern.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:348
+
+    void setUserID(lldb::user_id_t uid) { m_uid = uid; }
+
----------------
I believe we decided to call this "trace ID"


================
Comment at: unittests/CMakeLists.txt:77
+endif()
+add_subdirectory(Linux)
----------------
Please put this under unittests/Process/Linux (to match unittests/Process/gdb-remote)


================
Comment at: unittests/Linux/CMakeLists.txt:1
+include_directories(${LLDB_SOURCE_DIR}/source/Plugins/Process/Linux)
+
----------------
Delete this. Just include the files as "Plugins/Process/Linux/XXX.h"


https://reviews.llvm.org/D33674