davidca added inline comments.
Herald added a project: All.
================
Comment at: lldb/docs/lldb-gdb-remote.txt:469
+//
+// tsc_rate: {
+// kind: "perf",
----------------
why is all this called tsc rate? there is also offset in this conversion. What
about something like tsc_conversion_params or tsc_conv
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+ public:
----------------
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?
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:47
+ virtual ~TimestampCounterRate() = default;
+ /// Convert TSC value to nanoseconds. This represents the number of
nanoseconds since
+ /// the TSC register's reset.
----------------
wallace wrote:
>
To be precise, it's number of nanoseconds since boot. You should also add that
this nanoseconds clock is sched_clock
https://www.kernel.org/doc/html/latest/timers/timekeeping.html
In fact, you may want to use sched_clock as the name for this clock, rather
than Nanos as in the next lines.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:78-80
+bool fromJSON(const llvm::json::Value &value, TimestampCounterRateSP
&tsc_converter, llvm::json::Path path);
+
+bool fromJSON(const llvm::json::Value &value, TraceIntelPTGetStateResponse
&packet, llvm::json::Path path);
----------------
should this two be static factory methods?
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257
+ IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+
----------------
wallace wrote:
> sadly you can't put this here. If you do this, then GetState() will only work
> correctly if StartTrace() was invoked first. What if in the future we have a
> different flow in which lldb-server uses an additional tracing function
> besides StartTrace() and that new function forgets to set the tsc_rate?
> This is called coupling and should be avoided.
>
> What you should do instead is to create a new static function called
> `llvm::Optional<TimestampCounterRateSP >GetTscRate()` that will perform the
> perf_event request or reuse a previously saved value. It should work
> regardless of when it's invoked.
1) this is not a Get* method. This is populating a global. Get* are inexpensive
methods that return a value.
2) if the TSC conversion parameters will be stored in a global, why not to use
the Singleton pattern?
3) TSC parameters change over time. I really think that should be stored within
IntelPTManager. Obtaining this parameters is a ~10 microseconds operation, so
it's fine if they need to be read next time an IntelPTManager is created.
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:211
static bool IsSupported();
+ static void GetPerfTscRate(perf_event_mmap_page &mmap_meta);
+
----------------
wallace wrote:
> don't forget to add documentation here
how is ti
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
