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

[Jakob Johnson via Phabricator via lldb-commits]

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


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:51
+  int64_t m_time_shift;
+  int64_t m_time_zero;
+};
----------------
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?




================
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)} {}
----------------
wallace wrote:
> you can get rid of the constructors a
Need the 3 arg constructor to create an instance of this class in `IntelPTManager.cpp` and need the default constructor for when we create an uninitialized instance of the class before mapping the values from JSON in the `fromJSON` method.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:45
 
+std::shared_ptr<TscConverter> IntelPTManager::tsc_converter = nullptr;
+
----------------
wallace wrote:
> remove it from here
Without a definition of the static member outside of the class, a linker error occurs.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:238
 
+  static std::shared_ptr<TscConverter> tsc_converter;
+
----------------
wallace wrote:
> static llvm::Optional<TimestampCounterRateSP> g_tsc_rate; // by default this is None
Is the idea of wrapping the SP in an Optional here allows us to represent 3 different states?
1. `None`: we have not yet attempted to fetch/calculate the tsc rate.
2. `nullptr`: we tried to fetch the rate but one does not exist
3. `<non-null SP>`: the rate that was successfully fetched

Without the Optional, we cannot distinguish between the first two cases.

Please let me know if my understanding is correct.


================
Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:69
+  }
+  return ret;
+}
----------------
wallace wrote:
> return false; if you got here, then you have a kind that is not available
in this case should we also set the SP value to be nullptr? Otherwise that value will still be None which, from my understanding, indicates that the TscRate has not yet been fetched whereas nullptr indicates that the we have tried to fetch the rate but it does not exist.


================
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
----------------
wallace wrote:
> o.mapOptional is what you need here. That will return true if "tsc_conversion" is not available or if it's well-formed.
{F22265358}
>From looking at the source of map and mapOptional it appears that the only difference is that map sets Out to None whereas mapOptional leaves it unchanged, they both return true in the case that the property doesn't exist.

Given this, it's not immediately obvious to me which we should be using in this case - do we want the tsc_conversion to be set to None or left unchanged in the case that it's not present in the JSON?



================
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},
----------------
wallace wrote:
> here you should cast to int64_t
How should we handle serializing `m_time_zero` since it's `uint64_t` and the JSON library only supports int64_t for number types?






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120595