wallace added a comment.
Thanks @davidca and @labath for chiming in.
@jj10306, please rename IntelPTManager to IntelPTCollector for clarity.
================
Comment at: lldb/docs/lldb-gdb-remote.txt:469
+//
+// tsc_rate: {
+// kind: "perf",
----------------
davidca wrote:
> why is all this called tsc rate? there is also offset in this conversion.
> What about something like tsc_conversion_params or tsc_conv
tsc_conversion_params seems like a good idea
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+ public:
----------------
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
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:74
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
+ /// `nullptr` if no tsc conversion rate exists.
+ llvm::Optional<TimestampCounterRateSP> tsc_rate;
----------------
labath wrote:
> wallace wrote:
> > avoid using nullptr when you have an Optional.
> Ideally this would be a plain `TimestampCounterRateSP` variable and nullptr
> would denote emptyness. Since (shared) pointers already have a natural empty
> state, it's best to use that. Adding an extra Optional layer just creates
> ambiguity.
Good idea
================
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);
----------------
davidca wrote:
> should this two be static factory methods?
That's just the pattern most json conversion utils follow. It helps with some
templating stuff
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257
+ IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+
----------------
davidca wrote:
> 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.
Can tsc frequency rates change even in modern cpus?. In this case, it's no
brainer we need to calculate this number all the time.
I also agree with point number 1.
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:211
static bool IsSupported();
+ static void GetPerfTscRate(perf_event_mmap_page &mmap_meta);
+
----------------
davidca wrote:
> wallace wrote:
> > don't forget to add documentation here
> how is ti
@davidca, i think you didn't finish your comment
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
