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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
