[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
Fri Feb 25 16:33:37 PST 2022
wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
you got the flow correct. I left comments regarding specific details, but good job overall!!
================
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".
----------------
don't forget to f
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:51
+ int64_t m_time_shift;
+ int64_t m_time_zero;
+};
----------------
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
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:55
+/// TSC to nanosecond conversion.
+struct TscConverter {
+ virtual ~TscConverter() = default;
----------------
TscToWallTimeConverter is a better name
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:55
+/// TSC to nanosecond conversion.
+struct TscConverter {
+ virtual ~TscConverter() = default;
----------------
wallace wrote:
> TscToWallTimeConverter is a better name
use a class and not a struct. A struct has all its member public and we don't want that
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:58
+ /// Convert TSC value to nanoseconds.
+ virtual uint64_t TscToNanos(uint64_t tsc) = 0;
+ virtual llvm::json::Value toJSON() = 0;
----------------
ToNanos() is also okay. Up to you
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:63
+/// Conversion based on values define in perf_event_mmap_page.
+struct PerfTscConverter : TscConverter {
+ PerfTscConverter() = default;
----------------
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:64-66
+ PerfTscConverter() = default;
+ PerfTscConverter(uint32_t time_mult, uint16_t time_shift, uint64_t time_zero) :
+ m_perf_conversion{time_mult, time_shift, static_cast<int64_t>(time_zero)} {}
----------------
you can get rid of the constructors a
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:70
+
+ PerfTscConversionValues m_perf_conversion;
+};
----------------
you don't need this struct. You should just put the 3 fields here.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:86
+ /// `nullptr` if no TscConverter exists.
+ std::shared_ptr<TscConverter> tsc_converter;
+};
----------------
it's weird for a bag of data to contain a field transmitted by json called coverter. Just call it TimestampCounterRate. Because that's what it is. Then each TimestampCounterRate subclass can have a ToNanos(uint64_t tsc) method that converts to that.
Your documentation should specific if this ToNanos method returns the since the start of the computer, or since the start of the program, or anything else
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:89
+
+bool fromJSON(const llvm::json::Value &value, std::shared_ptr<TscConverter> &tsc_converter, llvm::json::Path path);
+
----------------
Create a typedef in this file called TimestampCounterRateSP for readability. You can use tsc_rate as your variable names
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:45
+std::shared_ptr<TscConverter> IntelPTManager::tsc_converter = nullptr;
+
----------------
remove it from here
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257-259
+ if (m_mmap_meta->cap_user_time_zero) {
+ IntelPTManager::tsc_converter = std::make_shared<PerfTscConverter>(PerfTscConverter(m_mmap_meta->time_mult, m_mmap_meta->time_shift, m_mmap_meta->time_zero));
+ }
----------------
notice the case is which g_tsc_rate is not None but nullptr. That means that the computation was performed but no rate was found, so the computation won't happen again. We have to make the code in that way because it's possible that checking for some rates is not a no-op. E.g. you can run a little 10ms program and calculate the difference in TSC to get an estimated rate.
Besides that, put all of this in a new function
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:238
+ static std::shared_ptr<TscConverter> tsc_converter;
+
----------------
static llvm::Optional<TimestampCounterRateSP> g_tsc_rate; // by default this is None
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:22
#include "llvm/ADT/None.h"
+#include <memory>
----------------
jj10306 wrote:
>
memory should be defined as a new include block between lines 9 and 11. That's just the pattern we follow
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:214
}
+ if (!m_tsc_converter && state->tsc_converter)
+ m_tsc_converter = state->tsc_converter;
----------------
just replace the converter if you got a new one from the state. IT's cheap
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:187
llvm::Optional<std::string> m_live_refresh_error;
+ /// TSC to nano converter. nullptr if no conversion exists.
+ std::shared_ptr<TscConverter> m_tsc_converter;
----------------
this is null by default, no need to mention it
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:188
+ /// TSC to nano converter. nullptr if no conversion exists.
+ std::shared_ptr<TscConverter> m_tsc_converter;
};
----------------
jj10306 wrote:
> Not sure `TraceIntelPT` is the correct spot to cache the converter on the client side. It will depend on how this new logic is integrated with the decoders, but curious to get your initial feedback on this.
this is the right place to cache it, right next to m_cpu_info
================
Comment at: lldb/source/Target/Trace.cpp:191-202
+ std::unique_ptr<TraceGetStateResponse> live_process_state = DoRefreshLiveProcessState(GetLiveProcessState());
- for (const TraceThreadState &thread_state :
- live_process_state->tracedThreads) {
- for (const TraceBinaryData &item : thread_state.binaryData)
- m_live_thread_data[thread_state.tid][item.kind] = item.size;
- }
+ if (live_process_state) {
+ for (const TraceThreadState &thread_state :
+ live_process_state->tracedThreads) {
+ for (const TraceBinaryData &item : thread_state.binaryData)
+ m_live_thread_data[thread_state.tid][item.kind] = item.size;
----------------
you can combine these blocks
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:56
+
+ bool ret = o.map("kind", tsc_converter_kind);
+
----------------
fail fast whenever you can
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:59
+ if (tsc_converter_kind == "perf") {
+ std::shared_ptr<PerfTscConverter> perf_tsc_converter_sp = std::make_shared<PerfTscConverter>(PerfTscConverter());
+ ret = ret && o.map("time_mult", perf_tsc_converter_sp->m_perf_conversion.m_time_mult) && o.map("time_shift", perf_tsc_converter_sp->m_perf_conversion.m_time_shift) && o.map("time_zero", perf_tsc_converter_sp->m_perf_conversion.m_time_zero);
----------------
you don't need to pass PerfTscConverter. make_shared should be smart enough to create the object by itself
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:59-60
+ if (tsc_converter_kind == "perf") {
+ std::shared_ptr<PerfTscConverter> perf_tsc_converter_sp = std::make_shared<PerfTscConverter>(PerfTscConverter());
+ ret = ret && o.map("time_mult", perf_tsc_converter_sp->m_perf_conversion.m_time_mult) && o.map("time_shift", perf_tsc_converter_sp->m_perf_conversion.m_time_shift) && o.map("time_zero", perf_tsc_converter_sp->m_perf_conversion.m_time_zero);
+ // should we check ret here?
----------------
wallace wrote:
> you don't need to pass PerfTscConverter. make_shared should be smart enough to create the object by itself
if you do this, then you can fail fast and do the int conversions here, without polluting your class
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:66-68
+ } else {
+ tsc_converter = nullptr;
+ }
----------------
delete this
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:69
+ }
+ return ret;
+}
----------------
return false; if you got here, then you have a kind that is not available
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:74
+ ObjectMapper o(value, path);
+ bool base_bool = o && fromJSON(value, (TraceGetStateResponse &)packet, path) && o.map("tsc_conversion", packet.tsc_converter);
+ // call perftscconverter
----------------
o.mapOptional is what you need here. That will return true if "tsc_conversion" is not available or if it's well-formed.
================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:84-86
+ {"time_mult",m_perf_conversion.m_time_mult},
+ {"time_shift", m_perf_conversion.m_time_shift},
+ {"time_zero", m_perf_conversion.m_time_zero},
----------------
here you should cast to int64_t
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120595/new/
https://reviews.llvm.org/D120595
More information about the lldb-commits
mailing list