[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

[walter erquinigo via Phabricator via lldb-commits]

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