[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces
walter erquinigo via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Feb 28 20:54:19 PST 2022
wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
in terms of functionality, everything is right, but you need to do some improvements
================
Comment at: lldb/docs/lldb-gdb-remote.txt:454-479
+// }],
+// ... other parameters specific to the provided tracing type
// }
//
// NOTES
// - "traceThreads" includes all thread traced by both "process tracing" and
// "thread tracing".
----------------
you have to improve the documentation and formatting following the existing documentation's format
================
Comment at: lldb/include/lldb/Target/Trace.h:306
+ /// \param[in] json_string
+ /// String representation of the jLLDBTraceGetState response.
+ ///
----------------
JSON string representing the jLLDBTraceGetState response. It might be invalid.
================
Comment at: lldb/include/lldb/Target/Trace.h:310
+ /// Unique pointer to the packet response, nullptr if response parsing failed.
+ virtual std::unique_ptr<TraceGetStateResponse>
+ DoRefreshLiveProcessState(llvm::Expected<std::string> json_string) = 0;
----------------
you don't need to return a unique_ptr for this. There's nothing that you want to forbid copies of. Just return
llvm::Optional<TraceGetStateResponse>
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:14
+
/// See docs/lldb-gdb-remote.txt for more information.
----------------
undo this
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:43
+
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
----------------
TSC is not a very well known concept, so better be very explanatory in the documentation
Class that can be extended to represent different CPU Time Stamp Counter (TSC) frequency rates, which can be used to convert TSCs to common units of time, such as nanoseconds. More on TSCs can be found here https://en.wikipedia.org/wiki/Time_Stamp_Counter.
================
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.
----------------
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:53
+
+typedef std::shared_ptr<TimestampCounterRate> TimestampCounterRateSP;
+
----------------
modern c++ uses the `using` keyword.
using TimestampCounterRateSP = std::shared_ptr<TimestampCounterRate>;
use the version you want
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:55-56
+
+/// TSC to nanoseconds conversion values defined in struct perf_event_mmap_page.
+/// See https://man7.org/linux/man-pages/man2/perf_event_open.2.html for more information.
+class PerfTimestampCounterRate : public TimestampCounterRate {
----------------
You have to make the documentation very friendly to newcomers.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:59
+ public:
+ PerfTimestampCounterRate() = default;
+ PerfTimestampCounterRate(uint32_t time_mult, uint16_t time_shift, uint64_t time_zero) :
----------------
do you need this?
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:61
+ PerfTimestampCounterRate(uint32_t time_mult, uint16_t time_shift, uint64_t time_zero) :
+ m_time_mult(time_mult), m_time_shift(time_shift), m_time_zero(static_cast<int64_t>(time_zero)) {}
+ uint64_t ToNanos(uint64_t tsc) override;
----------------
why do you do static_cast<int64_t>?
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:66-68
+ uint32_t m_time_mult;
+ uint16_t m_time_shift;
+ uint64_t m_time_zero;
----------------
having the correct types here is the right thing to do
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:72
+/// jLLDBTraceGetState gdb-remote packet
+/// Contains additional information related to TSC -> nanosecond conversion.
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
----------------
The response contains the frequency rate. Whether it's used to convert to nanos or not is a different topic.
Just delete this line.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:74
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
+ /// `nullptr` if no tsc conversion rate exists.
+ llvm::Optional<TimestampCounterRateSP> tsc_rate;
----------------
avoid using nullptr when you have an Optional.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:51
+ int64_t m_time_shift;
+ int64_t m_time_zero;
+};
----------------
jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > What is the suggested way to serialize a u64 into JSON? I thought of two approaches:
> > > 1. Store two separate u32
> > > 2. store the integer as a string
> > >
> > > @wallace wdyt?
> > you don't need to use int64_t here. You can use the real types here and do the conversion in the fromJson and toJson methods
> > What is the suggested way to serialize a u64 into JSON? I thought of two approaches:
> > 1. Store two separate u32
> > 2. store the integer as a string
> >
> > @wallace wdyt?
>
>
i'll respond below
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257
+ IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+
----------------
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.
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:650
+
+ state.tsc_rate = IntelPTManager::g_tsc_rate;
+
----------------
only set it if not None
if (Optional<...> tsc_rate_sp = IntelPTManager::GetTSCRate())
state.tsc_rate = tsc_rate_sp;
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:698
+
+void IntelPTManager::GetPerfTscRate(perf_event_mmap_page &mmap_meta) {
+ if (!IntelPTManager::g_tsc_rate) {
----------------
GetTscRate is a better name, because eventually we might want to include other fallback mechanisms
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:45
+std::shared_ptr<TscConverter> IntelPTManager::tsc_converter = nullptr;
+
----------------
jj10306 wrote:
> wallace wrote:
> > remove it from here
> Without a definition of the static member outside of the class, a linker error occurs.
makes sense
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:211
static bool IsSupported();
+ static void GetPerfTscRate(perf_event_mmap_page &mmap_meta);
+
----------------
don't forget to add documentation here
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:194
+ m_live_refresh_error = toString(json_string.takeError());
+ return nullptr;
+ }
----------------
return None
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:204
m_live_refresh_error = toString(state.takeError());
- return;
+ return nullptr;
}
----------------
None
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:213
}
+ m_tsc_rate = state->tsc_rate;
+
----------------
make sure that everything works when lldb-server doesn't have support for the perf zero cap
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:187
llvm::Optional<std::string> m_live_refresh_error;
+ /// TSC to nano conversion rate. `nullptr` if no conversion exists.
+ llvm::Optional<TimestampCounterRateSP> m_tsc_rate;
----------------
use the same documentation I suggest above for the state object
================
Comment at: lldb/source/Target/Trace.cpp:191
- Expected<std::string> json_string = GetLiveProcessState();
- if (!json_string) {
- DoRefreshLiveProcessState(json_string.takeError());
- return;
- }
- Expected<TraceGetStateResponse> live_process_state =
- json::parse<TraceGetStateResponse>(*json_string, "TraceGetStateResponse");
- if (!live_process_state) {
- DoRefreshLiveProcessState(live_process_state.takeError());
- return;
- }
+ if (std::unique_ptr<TraceGetStateResponse> live_process_state = DoRefreshLiveProcessState(GetLiveProcessState())) {
+ for (const TraceThreadState &thread_state :
----------------
you are not storing live_process_state elsewhere. Just use an optional. That's all you need
================
Comment at: lldb/source/Target/Trace.cpp:191
- Expected<std::string> json_string = GetLiveProcessState();
- if (!json_string) {
- DoRefreshLiveProcessState(json_string.takeError());
- return;
- }
- Expected<TraceGetStateResponse> live_process_state =
- json::parse<TraceGetStateResponse>(*json_string, "TraceGetStateResponse");
- if (!live_process_state) {
- DoRefreshLiveProcessState(live_process_state.takeError());
- return;
- }
+ if (std::unique_ptr<TraceGetStateResponse> live_process_state = DoRefreshLiveProcessState(GetLiveProcessState())) {
+ for (const TraceThreadState &thread_state :
----------------
wallace wrote:
> you are not storing live_process_state elsewhere. Just use an optional. That's all you need
you can fail faster to keep everything cleaner
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:60
+ if (tsc_rate_kind == "perf") {
+ int a, b, c;
+ if (!o.map("a", a) || !o.map("b", b) || !o.map("c", c))
----------------
int64_t, and give proper names
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:65
+ }
+ return false;
+}
----------------
path.report("unsupported TSC rate kind");
make sure to write a test with malformed data
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:70
+ ObjectMapper o(value, path);
+ bool base_bool = o && fromJSON(value, (TraceGetStateResponse &)packet, path) && o.mapOptional("tsc_rate", packet.tsc_rate);
+ return base_bool;
----------------
just return the bool
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:80
+ {"time_shift", (int64_t) m_time_shift},
+ {"time_zero", (int64_t) m_time_zero}, // TODO: handle the potential lossy conversion correctly
+ });
----------------
this kind of casting is all fine. Look at this code
```
#include <iostream>
#include <climits>
using namespace std;
int main() {
uint64_t x = ULONG_MAX; // 0xFFFFFFFF;
cout << x << endl;
cout << (int64_t)x << endl;
cout << (uint64_t)(int64_t)x << endl;
}
```
and the output is
```
18446744073709551615
-1
18446744073709551615
```
The same works for ULONG_MAX - 1, which is equivalent to -2.
So that means that there's no conversion loss
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:86-91
+ auto tsc_rate = packet.tsc_rate;
+ // is there a better way to check that it's not None nor nullptr?
+ if (tsc_rate && *tsc_rate) {
+ json::Value tsc_converter_json = tsc_rate.getValue()->toJSON();
+ base.getAsObject()->try_emplace("tsc_rate", std::move(tsc_converter_json));
+ }
----------------
the code is simpler is you make strict use of the shared pointer only for valid cases
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120595/new/
https://reviews.llvm.org/D120595
More information about the lldb-commits
mailing list