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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 marked 10 inline comments as done.
jj10306 added inline comments.


================
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;
+
----------------
wallace wrote:
> 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
sgtm - do you think the `GetTimestampCounter` API's name should remain the same or change it to something like `GetTsc`? This file is trace agnostic so tying it to TSC could be confusing since, afaiu, TSC is intel specific, but I noticed the docs of that method mention "TSC", so curious to hear your thoughts.


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:46
+// TODO: update after naming convention is agreed upon
+class TimestampCounterRate {
+  public:
----------------
wallace wrote:
> This class is generic, so better not enforce the meaning of the zero tsc. Each trace plugin might be opinionated regarding this timestamp 
Good point. Given that this class is generic, would it make more sense to move it from this trace specific file `TraceIntelPTGDBRemotePackets.h` to `TraceGDBRemotePackets.h` since that file is trace agnostic?


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+  public:
----------------
jj10306 wrote:
> wallace wrote:
> > davidca wrote:
> > > nit, Intel documentation and kernel refers this as TSC, not TimestampCounter, in order to differentiate this awful name from everywhere else that "Timestamp" and "Counter" is used. Perhaps embrace TSC to make it easier to google info?
> > Ahh!! Then let's do what David says. Let's call it tsc everywhere
> Which do you think is a better name to replace `TimestampCounterRate` with - `TscConversionParams` (as mentioned in the previous comment, "tsc_conversion_params" will be the new key in the TraceGetState packet schema, so this would be consistent with that) or `TscConversioninfo` or `TscConverter` (since the function of the class is to provide functionality to convert Tsc to time)? 
> @davidca @wallace wdyt?
Bumping this discussion related to naming


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

https://reviews.llvm.org/D120595