[Lldb-commits] [PATCH] D106328: [trace][intel pt] Add TSC timestamps

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: JDevlieghere.

Man cases of having variables named "tsc" or "m_tsc" when it might be clear to have them named "enable_tsc" and "m_enable_tsc".



================
Comment at: lldb/source/Commands/Options.td:1071
+    Desc<"For each instruction, print the corresponding timestamp counter if available.">;
 }
 
----------------
You mentioned "--psb_period" in your description, but that isn't here. Just wanted to make sure that is on purpose. And if you do add it it should be --psb-period with - and not a _


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:58
+                                                IntelPTConfigFileType type) {
+  auto stream = llvm::MemoryBuffer::getFileAsStream(file);
+
----------------
Don't use auto here? More readable


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:87
+
+  if (buffer.trim().consumeInteger(getRadix(), value)) {
+    std::ostringstream error;
----------------
Is "buffer" binary or text? If text, then this is fine.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:104
+    return createStringError(inconvertibleErrorCode(), error.str().c_str());
+  }
+  return value;
----------------
Do you want an else clause here to error check "ZeroOne" in case you get something like 12 back?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:85
+  llvm::Error StartTrace(lldb::pid_t pid, lldb::tid_t tid, uint64_t buffer_size,
+                         bool tsc, llvm::Optional<size_t> psb_period);
 
----------------
rename "tsc" to "enable_tsc"?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h:21
 const size_t kProcessBufferSizeLimit = 5 * 1024 * 1024; // 500MB
+const bool kTSC = false;
+const llvm::Optional<size_t> kPSBPeriod = llvm::None;
----------------
"kDefaultTSCValue"? The name above doesn't really convey what this variable is


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h:22
+const bool kTSC = false;
+const llvm::Optional<size_t> kPSBPeriod = llvm::None;
 
----------------
kDefaultPSBPeriod?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106328/new/

https://reviews.llvm.org/D106328