wallace added inline comments.
================
Comment at: lldb/docs/lldb-gdb-remote.txt:455
+// }],
+// "tsc_conversion"?: Optional<{
+// "kind": <string>,
----------------
The ? After the key name makes Optional<> redundant. Just keep the ?
================
Comment at: lldb/docs/lldb-gdb-remote.txt:457
+// "kind": <string>,
+// Timestamp counter conversion rate kind, e.g. perf.
+// The kind strings can be found in the overriden toJSON method
----------------
perf-linux might be a better name
================
Comment at: lldb/docs/lldb-gdb-remote.txt:458
+// Timestamp counter conversion rate kind, e.g. perf.
+// The kind strings can be found in the overriden toJSON method
+// of each TimestampCounterRate subclass. The kind determines
----------------
All the supported kinds must be present below in the documentation, so no need
to mention c++ classes here
================
Comment at: lldb/include/lldb/Target/TraceCursor.h:193
+ // TODO: add docs and rename once naming convention is agreed upon
+ virtual llvm::Optional<uint64_t> GetNanos() = 0;
+
----------------
What about just naming it GetTimestamp and return std::chrono::nanoseconds?
That way we use c++ time conversion libraries and we don't include the time
granularity in the name
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:46
+// TODO: update after naming convention is agreed upon
+class TimestampCounterRate {
+ public:
----------------
This class is generic, so better not enforce the meaning of the zero tsc. Each
trace plugin might be opinionated regarding this timestamp
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:64
+ m_time_mult(time_mult), m_time_shift(time_shift), m_time_zero(time_zero)
{}
+ uint64_t ToNanos(uint64_t tsc) override;
+ llvm::json::Value toJSON() override;
----------------
Return a std:: Chrono type. You might want rename this to ToWallTime
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:53
+
+} // namespace resource_handle
+
----------------
Nice
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:223
+ /// \a The timestamp counter rate if one exists, \a nullptr if no
conversion exists.
+ static TimestampCounterRateSP GetTscRate(lldb::pid_t pid);
+
----------------
Now that we don't have caching, try to move this method to Process Linux, as an
static, so that others can use that method more easily.
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:253
+ /// Server-side cache of timestamp counter rate
+ static llvm::Optional<TimestampCounterRateSP> g_tsc_rate;
+
----------------
We don't need caching anymore. Just calculate it every time we request the
state, following the information that David gave that the rate might change
over time.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120595/new/
https://reviews.llvm.org/D120595
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
