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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: wallace, davidca.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Begin implementing Tsc to timestamp conversion for IntelPT traces:

- Add TscConverter and stub out first implementor (PerfTscConverter)
- Add tsc_converter to TraceIntelPT and update DoRefreshLiveProcess to 
accomodate different trace implementation's TraceGetState responses

This is a work in progress, creating diff now to get feedback on initial 
changes before proceeding with decoder and schema changes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120595

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Target/Trace.cpp
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp

Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "lldb/Utility/TraceIntelPTGDBRemotePackets.h"
+#include "lldb/Utility/TraceGDBRemotePackets.h"
 
 using namespace llvm;
 using namespace llvm::json;
@@ -43,4 +44,55 @@
   return base;
 }
 
+uint64_t PerfTscConverter::TscToNanos(uint64_t tsc) {
+  // conversion logic here
+  return 1;
+}
+
+bool fromJSON(const llvm::json::Value &value, std::shared_ptr &tsc_converter, llvm::json::Path path) {
+  std::string tsc_converter_kind;
+  ObjectMapper o(value, path);
+
+  bool ret = o.map("kind", tsc_converter_kind);
+
+  if (tsc_converter_kind == "perf") {
+std::shared_ptr perf_tsc_converter_sp = std::make_shared(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?
+tsc_converter = perf_tsc_converter_sp;
+  } else if (tsc_converter_kind == "freq") {
+// TODO
+tsc_converter = nullptr;
+  } else {
+tsc_converter = nullptr;
+  }
+  return ret;
+}
+
+bool fromJSON(const json::Value &value, TraceIntelPTGetStateResponse &packet, Path path) {
+  ObjectMapper o(value, path);
+  bool base_bool = o && fromJSON(value, (TraceGetStateResponse &)packet, path) && o.map("tsc_conversion", packet.tsc_converter);
+  // call perftscconverter
+  // get an instance of perftscconverter and wrap it in optionaltoJSON();
+base.getAsObject()->try_emplace("tsc_conversion", std::move(tsc_converter_json));
+  }
+  return base;
+}
 } // namespace lldb_private
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -18,6 +18,7 @@
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/Stream.h"
+#include "lldb/Utility/TraceGDBRemotePackets.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -187,28 +188,19 @@
   m_stop_id = new_stop_id;
   m_live_thread_data.clear();
 
-  Expected json_string = GetLiveProcessState();
-  if (!json_string) {
-DoRefreshLiveProcessState(json_string.takeError());
-return;
-  }
-  Expected live_process_state =
-  json::parse(*json_string, "TraceGetStateResponse");
-  if (!live_process_state) {
-DoRefreshLiveProcessState(live_process_state.takeError());
-return;
-  }
+  std::unique_ptr 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;
+}
 
-  for (const TraceBinaryData &item : live_process_state->processBinaryData)
-m_live_process_data[item.kind] = item.size;
+for (const TraceBinaryData &item : live_process_state->processBinaryData)
+  m_live_process_data[item.kind] = item.size;
+  }
 
-  DoRefreshLiveProcessState(std::move(live_process_state));
 }
 
 uint32_t Trace::GetStopID() {
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===
--- lldb/sou

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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:51
+  int64_t m_time_shift;
+  int64_t m_time_zero;
+};

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/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:22
 #include "llvm/ADT/None.h"
+#include 
 





Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:188
+  /// TSC to nano converter. nullptr if no conversion exists.
+  std::shared_ptr m_tsc_converter;
 };

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.



Comment at: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp:75-76
+  bool base_bool = o && fromJSON(value, (TraceGetStateResponse &)packet, path) 
&& o.map("tsc_conversion", packet.tsc_converter);
+  // call perftscconverter
+  // get an instance of perftscconverter and wrap it in optionalhttps://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

[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(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 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 tsc_converter;
+

wallace wrote:
> static llvm::Optional 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. ``: 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

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 411862.
jj10306 marked 3 inline comments as done.
jj10306 edited the summary of this revision.
jj10306 added a comment.

Address comments, still need to handle uint64_t JSON serialization correctly. I 
plan to proceed with decoder and schema changes now that initial feedback has 
been received and addressed.


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

https://reviews.llvm.org/D120595

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Target/Trace.cpp
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp

Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "lldb/Utility/TraceIntelPTGDBRemotePackets.h"
+#include "lldb/Utility/TraceGDBRemotePackets.h"
 
 using namespace llvm;
 using namespace llvm::json;
@@ -43,4 +44,51 @@
   return base;
 }
 
+uint64_t PerfTimestampCounterRate::ToNanos(uint64_t tsc) {
+  // conversion logic here
+  return 1;
+}
+
+bool fromJSON(const llvm::json::Value &value, TimestampCounterRateSP &tsc_rate, llvm::json::Path path) {
+  std::string tsc_rate_kind;
+  ObjectMapper o(value, path);
+
+  if(!o.map("kind", tsc_rate_kind))
+return false;
+
+  if (tsc_rate_kind == "perf") {
+int a, b, c;
+if (!o.map("a", a) || !o.map("b", b) || !o.map("c", c))
+  return false;
+tsc_rate = std::make_shared((uint32_t)a, (uint16_t)b, (uint64_t)c);
+  }
+  return false;
+}
+
+bool fromJSON(const json::Value &value, TraceIntelPTGetStateResponse &packet, Path path) {
+  ObjectMapper o(value, path);
+  bool base_bool = o && fromJSON(value, (TraceGetStateResponse &)packet, path) && o.mapOptional("tsc_rate", packet.tsc_rate);
+  return base_bool;
+
+}
+
+llvm::json::Value PerfTimestampCounterRate::toJSON() {
+  return json::Value(json::Object{
+{"kind", "perf"},
+{"time_mult", (int64_t) m_time_mult},
+{"time_shift", (int64_t) m_time_shift},
+{"time_zero", (int64_t) m_time_zero}, // TODO: handle the potential lossy conversion correctly
+  });
+}
+
+json::Value toJSON(const TraceIntelPTGetStateResponse &packet) {
+  json::Value base = toJSON((const TraceGetStateResponse &)packet);
+  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));
+  }
+  return base;
+}
 } // namespace lldb_private
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -18,6 +18,7 @@
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/Stream.h"
+#include "lldb/Utility/TraceGDBRemotePackets.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -187,28 +188,17 @@
   m_stop_id = new_stop_id;
   m_live_thread_data.clear();
 
-  Expected json_string = GetLiveProcessState();
-  if (!json_string) {
-DoRefreshLiveProcessState(json_string.takeError());
-return;
-  }
-  Expected live_process_state =
-  json::parse(*json_string, "TraceGetStateResponse");
-  if (!live_process_state) {
-DoRefreshLiveProcessState(live_process_state.takeError());
-return;
-  }
+  if (std::unique_ptr 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;
+}
 
-  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;
+for (const TraceBinaryData &item : live_process_state->processBinaryData)
+  m_live_process_data[item.kind] = item.size;
   }
 
-  for (const TraceBinaryData &item : live_process_state->processBinaryData)
-m_live_process_data[item.kind] = item.size;
-
-  DoRefreshLiveProcessState(std::move(live_process_state));
 }
 
 uint32_t Trace::GetStopID() {
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT

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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 marked 41 inline comments as done.
jj10306 added a comment.

Addressed comments locally, waiting to post update until the couple minor 
discussions with @wallace and @davidca in the comments are complete.




Comment at: lldb/docs/lldb-gdb-remote.txt:469
+//
+//tsc_rate: {
+//  kind: "perf",

wallace wrote:
> 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
Agreed, making that change!



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+  public:

wallace wrote:
> 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
Which do you think is a better name to replace `TimestampCounterRate` with - 
`TscConversionParams` (as mentioned in the previous comment, 
"tsc_conversion_params" will be the new key in the TraceGetState packet schema, 
so this would be consistent with that) or `TscConversioninfo` or `TscConverter` 
(since the function of the class is to provide functionality to convert Tsc to 
time)? 
@davidca @wallace wdyt?



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.

davidca wrote:
> 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.
Chapter 17.7 of [[ 
https://www.intel.com/content/www/us/en/architecture-and-technology/64-ia-32-architectures-software-developer-system-programming-manual-325384.html
 | Intel's software developer manual (volume 3) ]] states the following:
"The time-stamp counter (as implemented in the P6 family, Pentium, Pentium M, 
Pentium 4, Intel Xeon, Intel Core
Solo and Intel Core Duo processors and later processors) is a 64-bit counter 
that is **set to 0 following a RESET of
the processor**."

Does a reset only occur when the system is rebooted? I mentioned the reset in 
the documentation here to be very specific, but if reset is always the same as 
boot then I agree it makes sense to change it to "nanoseconds since boot". 

I'll add in the documentation that the nanoseconds being referred to here are 
from the the sched_clock and add the link you shared for reference.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:74
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
+  /// `nullptr` if no tsc conversion rate exists.
+  llvm::Optional 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.
Agreed, will revert back to the plain SP.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257
 
+  IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+

wallace wrote:
> 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::OptionalGetTscRate()` 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 

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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 413985.
jj10306 marked 2 inline comments as done.
jj10306 edited the summary of this revision.
jj10306 added a comment.

Addressed most of the previous diff's comments (other than the ones from 
@wallace and @davidca that were explicitly mentioned in my previous comment). 
The naming of the conversion values is currently inconsistent, but I will 
update the naming and make it consistent across all the new changes once the 
name is agreed upon.

- Add client and server side caching of conversion values
- Update `DecodedThread` to allow for access to the conversion values from the 
`TraceCursor`
- Add `GetNanos` (to be renamed) API to `TraceCursor` to allow for conversion 
of a trace instruction's TSC value to nanoseconds
- Update `trace schema` and `trace save` commands to accommodate schema changes
- Add `resource_handle` namespace to allow for reuse of file descriptor and 
munmap resource handles
- Rename IntelPTManager to IntelPTCollector


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

https://reviews.llvm.org/D120595

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp

Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "lldb/Utility/TraceIntelPTGDBRemotePackets.h"
+#include "lldb/Utility/TraceGDBRemotePackets.h"
 
 using namespace llvm;
 using namespace llvm::json;
@@ -43,4 +44,51 @@
   return base;
 }
 
+uint64_t PerfTimestampCounterRate::ToNanos(uint64_t tsc) {
+  // See 'time_zero' section of https://man7.org/linux/man-pages/man2/perf_event_open.2.html
+uint64_t quot = tsc >> m_time_shift;
+uint64_t rem_flag = (((uint64_t)1 << m_time_shift) - 1);
+uint64_t rem  = tsc & rem_flag;
+return m_time_zero + quot * m_time_mult + ((rem * m_time_mult) >> m_time_shift);
+}
+
+bool fromJSON(const llvm::json::Value &value, TimestampCounterRateSP &tsc_rate, llvm::json::Path path) {
+  std::string tsc_rate_kind;
+  ObjectMapper o(value, path);
+
+  if(!o.map("kind", tsc_rate_kind))
+return false;
+
+  if (tsc_rate_kind == "perf") {
+int64_t time_mult, time_shift, time_zero;
+if (!o.map("time_mult", time_mult) || !o.map("time_shift", time_shift) || !o.map("time_zero", time_zero))
+  return false;
+tsc_rate = std::make_shared((uint32_t)time_mult, (uint16_t)time_shift, (uint64_t)time_zero);
+return true;
+  }
+  path.report("Unsupported TSC rate kind");
+  return false;
+}
+
+bool fromJSON(const json::Value &value, TraceIntelPTGetStateResponse &packet, Path path) {
+  ObjectMapper o(value, path);
+  return o && fromJSON(value, (TraceGetStateResponse &)packet, path) && o.mapOptional("tsc_rate", packet.tsc_rate);
+}
+
+llvm::json::Value PerfTimestampCounterRate::toJSON() {
+  return json::Value(json::Object{
+{"kind", "perf"},
+{"time_mult", (int64_t) m_time_mult},
+{"time_shift", (int64_t) m_time_shift},
+{"time_zero", (int64_t) m_time_zero},
+  });
+}
+
+json::Value toJSON(const TraceIntelPTGetStateResponse &packet) {
+  json::Value base = toJSON((const TraceGetStateResponse &)packet);
+  if (packet.tsc_rate)
+base.getAsObject()->try_emplace("tsc_rate", packet.tsc_rate->toJSON());
+
+  return base;
+}
 } // namespace lldb_private
Index: lldb/source/Target/TraceInstructionDumper.cpp
===
--- lldb/source/Target/TraceInstructionDumper.cpp
+++ lldb/source/Target/TraceInstructionDumper.cpp
@@ -185,6 +185,10 @@
 
   if (Optional timestamp = m_cursor_up->GetTimestampC

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

[Jakob Johnson via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Target/TraceCursor.h:193
+  // TODO: add docs and rename once naming convention is agreed upon
+  virtual llvm::Optional GetNanos() = 0;
+

wallace wrote:
> What about just naming it GetTimestamp and return std::chrono::nanoseconds? 
> That way we use c++ time conversion libraries and we don't include the time 
> granularity in the name
sgtm - do you think the `GetTimestampCounter` API's name should remain the same 
or change it to something like `GetTsc`? This file is trace agnostic so tying 
it to TSC could be confusing since, afaiu, TSC is intel specific, but I noticed 
the docs of that method mention "TSC", so curious to hear your thoughts.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:46
+// TODO: update after naming convention is agreed upon
+class TimestampCounterRate {
+  public:

wallace wrote:
> This class is generic, so better not enforce the meaning of the zero tsc. 
> Each trace plugin might be opinionated regarding this timestamp 
Good point. Given that this class is generic, would it make more sense to move 
it from this trace specific file `TraceIntelPTGDBRemotePackets.h` to 
`TraceGDBRemotePackets.h` since that file is trace agnostic?



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+  public:

jj10306 wrote:
> wallace wrote:
> > 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
> Which do you think is a better name to replace `TimestampCounterRate` with - 
> `TscConversionParams` (as mentioned in the previous comment, 
> "tsc_conversion_params" will be the new key in the TraceGetState packet 
> schema, so this would be consistent with that) or `TscConversioninfo` or 
> `TscConverter` (since the function of the class is to provide functionality 
> to convert Tsc to time)? 
> @davidca @wallace wdyt?
Bumping this discussion related to naming


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

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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

Due to the size and expanding scope of this diff, I'm going to split this into 
several smaller diffs.


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

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: wallace, davidca.
Herald added a subscriber: mgorny.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Minor refactor and renaming:

- Rename `IntelPTManager` class and files to `IntelPTCollector`
- Change `TraceCursor::GetTimestampCounter` API to general trace counter API, 
`GetCounter`

First of a series of smaller diffs that split https://reviews.llvm.org/D120595 
up.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121711

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Plugins/Process/Linux/CMakeLists.txt
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTCollector.h
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/unittests/Process/Linux/CMakeLists.txt
  lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
  lldb/unittests/Process/Linux/IntelPTManagerTests.cpp

Index: lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
===
--- lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
+++ lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
@@ -1,4 +1,4 @@
-//===-- IntelPTManagerTests.cpp ---===//
+//===-- IntelPTCollectorTests.cpp ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -8,7 +8,7 @@
 
 #include "gtest/gtest.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "llvm/ADT/ArrayRef.h"
 
 
Index: lldb/unittests/Process/Linux/CMakeLists.txt
===
--- lldb/unittests/Process/Linux/CMakeLists.txt
+++ lldb/unittests/Process/Linux/CMakeLists.txt
@@ -1,5 +1,5 @@
 add_lldb_unittest(TraceIntelPTTests
-  IntelPTManagerTests.cpp
+  IntelPTCollectorTests.cpp
 
   LINK_LIBS
 lldbPluginProcessLinux
Index: lldb/source/Target/TraceInstructionDumper.cpp
===
--- lldb/source/Target/TraceInstructionDumper.cpp
+++ lldb/source/Target/TraceInstructionDumper.cpp
@@ -183,7 +183,7 @@
 if (m_show_tsc) {
   s.Printf("[tsc=");
 
-  if (Optional timestamp = m_cursor_up->GetTimestampCounter())
+  if (Optional timestamp = m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
 s.Printf("0x%016" PRIx64, *timestamp);
   else
 s.Printf("unavailable");
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -28,7 +28,7 @@
 
   lldb::addr_t GetLoadAddress() override;
 
-  llvm::Optional GetTimestampCounter() override;
+  llvm::Optional GetCounter(lldb::TraceCounter counter_type) override;
 
   lldb::TraceInstructionControlFlowType
   GetInstructionControlFlowType() override;
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -85,8 +85,11 @@
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
-Optional TraceCursorIntelPT::GetTimestampCounter() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  switch (counter_type) {
+case lldb::eTraceCounterTSC:
+  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  }
 }
 
 TraceInstructionControlFlowType
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -20,7 +20,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-types.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "NativeThreadLinux.h"
 #include "Plugins/Process/POSIX/NativeProcessELF.h"
 #include "Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h"
@@ -241,7 +241,7 @@
   Status PopulateMemoryRegionCache();
 
   /// Manages Intel PT process and thread traces.
-  IntelPTManager m_intel_pt_manager;
+ 

[Lldb-commits] [PATCH] D128484: [NFC][lldb][trace] Rename trace session to trace bundle

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

lgtm - thanks for doing this renaming 🙂




Comment at: lldb/include/lldb/Core/PluginManager.h:346
 
-  static TraceCreateInstanceForSessionFile
+  static TraceCreateInstanceForBundle
   GetTraceCreateCallback(llvm::StringRef plugin_name);

nit:  `FromBundle` makes more sense than `ForBundle` imo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128484

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128477: [trace] Add a flag to the decoder to output the instruction type

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.

Thanks for working on this 🙂
Looks good overall, just left some minor comments.




Comment at: lldb/include/lldb/Target/TraceCursor.h:64
 /// Low level traversal:
-///   Unlike the \a TraceCursor::Next() API, which uses a given granularity and
-///   direction to advance the cursor, the \a TraceCursor::Seek() method can be
+///   Unlike the \a TraceCursor::Next() API, which direction to advance
+//the cursor, the \a TraceCursor::Seek() method can be

For some reason I can't make inline suggestions, so I'll just put my suggested 
edit below:

s/which direction/which uses the current direction of the trace



Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:105-107
+  void PrintInstructionType();
+
   void DumpInstructionDisassembly(const InstructionSymbolInfo &insn);

Don't forget to rebase onto mainline, @wallace's recent diff 
(https://reviews.llvm.org/D128316) refactored some of this code, so that will 
effect where some of these new changes should live.



Comment at: lldb/source/Commands/Options.td:1132
+  def thread_trace_dump_instructions_show_kind : Option<"kind", "k">, Group<1>,
+Desc<"For each instruction, print the instruction type">;
   def thread_trace_dump_instructions_show_tsc : Option<"tsc", "t">, Group<1>,

nit: add a period at the end of the description (:



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:46-50
 if (m_tsc_range && !m_tsc_range->InRange(m_pos))
   m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();
 
-if (!m_ignore_errors && IsError())
-  return true;
-if (GetInstructionControlFlowType() & m_granularity)
-  return true;
+return true;
   }

Apologies again bc I can't "Suggest Edits" inline, so leaving my suggestion in 
a code block below:

IIUC, we previously had a while loop here because the possibility of "skipping" 
instructions due to ignoring errors or not matching the granularity. Since this 
diff removes those two things, this logic can now be simplified since the trace 
cursor can no longer skip the instructions - below is roughly what I'm thinking:
```
bool TraceCursorIntelPT::Next() {
  auto canMoveOne = [&]() {
if (IsForwards())
  return m_pos + 1 < GetInternalInstructionSize();
return m_pos > 0;
  };

  if (!canMoveOne())
return false;

  m_pos += IsForwards() ? 1 : -1;
  if (m_tsc_range && !m_tsc_range->InRange(m_pos))
m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();

  return true;
}
```
@wallace wdyt?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128477

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128543: [trace] Improve the TraceCursor iteration API

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

will take a complete look over the weekend, but wanted to point out the 
conflict with @persona0220's diff asap




Comment at: lldb/include/lldb/Target/TraceCursor.h:93-98
-  /// Set the granularity to use in the \a TraceCursor::Next() method.
-  void SetGranularity(lldb::TraceInstructionControlFlowType granularity);
-
-  /// Set the "ignore errors" flag to use in the \a TraceCursor::Next() method.
-  void SetIgnoreErrors(bool ignore_errors);
-

@persona0220 did this in https://reviews.llvm.org/D128477 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128543

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128543: [trace] Improve the TraceCursor iteration API

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

lgtm, thanks for making the cursor traversal much cleaner




Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:228-231
-  // We insert a fake error signaling an empty trace if needed becasue the
-  // TraceCursor requires non-empty traces.
-  if (m_instruction_ips.empty())
-AppendError(createStringError(inconvertibleErrorCode(), "empty trace"));

nice, the new approach is much cleaner



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:38
+void TraceCursorIntelPT::Next() {
+  m_pos += IsForwards() ? 1 : -1;
 

should only do this increment or decrement if `HasValue()` is true? otherwise 
(in theory) this value could wrap around if it's incremented/decremented too 
many times?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:40
 
+  {
+// We try to go to a neighbor tsc range that might contain the current pos

why is this new scope introduced here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128543

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128576: [trace] Make events first class items in the trace cursor and rework errors

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

review- part 1




Comment at: lldb/include/lldb/Target/Trace.h:171
+  /// information failed to load, an \a llvm::Error is returned.
+  virtual llvm::Expected
+  CreateNewCursor(Thread &thread) = 0;

Do we want to keep the cursor as a UP or take this refactor before creating the 
public bindings to make it a SP? IMO a UP might make sense because I can't 
really think of many cases where you would want multiple users of the same 
cursor.
wdyt?



Comment at: lldb/include/lldb/Target/TraceCursor.h:44-46
+/// The cursor can also point at events in the trace, which aren't errors
+/// nor instructions. An example of an event could be a context switch in
+/// between two instructions.

do you want to include pointers to the methods you can use to check/get events 
similar to what you did for errors above?



Comment at: lldb/include/lldb/Target/TraceCursor.h:232-246
+  virtual const char *GetError() const = 0;
 
-  /// Get the corresponding error message if the cursor points to an error in
-  /// the trace.
-  ///
   /// \return
-  /// \b nullptr if the cursor is not pointing to an error in
-  /// the trace. Otherwise return the actual error message.
-  virtual const char *GetError() = 0;
+  /// Whether the cursor points to an event or not.
+  virtual bool IsEvent() const = 0;
 
   /// \return

For the getters, what are your thoughts on returning an optional instead of 
using designated value/variants (ie nullptr, LLDB_INVALID_INSTRUCTION, 
eTraceEventNone`) to indicate that the current item does not match what `Get` 
method is being used?

Along the same lines, but a slightly bigger change:
With the introduction of `Events` as first class citizens of the trace cursor 
API, it may make sense to introduce the notion of of a trace "item" or 
something similar that encapsulates (instructions, events, errors, etc). I'm 
thinking of a tagged union type structure (ie a Rust enum)  that enforces the 
correctness of the access to the underlying item.
wdyt?



Comment at: lldb/include/lldb/Target/TraceCursor.h:258
+  virtual llvm::Optional
+  GetCounter(lldb::TraceCounter counter_type) const = 0;
 

Now that we are introducing the notion of an `Event`? wdyt about combining 
Events and Counters since a counter value in a trace really is just a type of 
event? In this case, `Counter` could just become a variant of `TraceEvent`. 
This may make more sense because even in the case of TSCs in an IntelPT trace 
because, strictly speaking, these aren't associated with instructions, correct? 
Instead the TSC values are emitted with PSBs and then we "attribute" these 
values to the nearest instruction, right?



Comment at: lldb/include/lldb/Target/TraceDumper.h:59
+  /// Helper struct that holds all the information we know about a trace item
+  struct TraceItem {
 lldb::user_id_t id;

oh looks like you already introduced a `TraceItem` structure!
Similar to my comment above related to introducing a trace item type structure 
for the trace cursor, would it make sense to have a separate type for each of 
the different "items". With the way the TraceItem struct is currently, a lot of 
this data is mutually exclusive, hence the many optional fields, correct?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:145-147
   /// \return
-  /// The control flow categories, or \b 0 if the instruction is an error.
+  /// The control flow categories, or an undefined vlaue if the item is not
+  /// an instruction.

Why not use an optional here instead?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:236-239
+  enum class ItemKind : uint64_t {
+Instruction = 0,
+Error = LLDB_INVALID_ADDRESS - 1,
+Event = LLDB_INVALID_ADDRESS - 2,

Continuing the theme of my comments related to the notion of a `TraceItem` - 
would it be possible to unite the notion of a trace item? currently there is an 
Item enum here, a item struct in the Dumper and I feel there is a need for a 
Item at the trace cursor level as well.
wdyt?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:241
+  };
+  ItemKind LowestNonInstructionItemKind = ItemKind::Event;
+

why is this needed?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:253-258
+  /// The low level storage of all instruction addresses, error and events. 
Each
+  /// trace item has an index in this vector and this index is used throughout
+  /// the code. As an storage optimization, values of \b ItemKind::Error
+  /// indicate that this item is an error, \b ItemKind::Event if the item is an
+  /// event, and otherwise 

[Lldb-commits] [PATCH] D128576: [trace] Make events first class items in the trace cursor and rework errors

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

This is awesome work - the code is much more understandable, so thanks for 
doing this!

lgtm overall, just left a couple final minor comments. I think we can continue 
to iterate/improve this design, so we can discuss those improvements for future 
diffs. 
Also, let me know what you think about how to support the two different types 
of disable events - this doesn't need to be done in this diff but it would be 
extremely useful as we begin to explore kernel tracing since this will most 
likely require us to understand any differences between user only, kernel only, 
or user + kernel tracing.




Comment at: lldb/include/lldb/lldb-enumerations.h:1162-1167
+// Enum used to identify which kind of item a \a TraceCursor is pointing at
+enum TraceItemKind {
+  eTraceItemKindError = 0,
+  eTraceItemKindEvent,
+  eTraceItemKindInstruction,
 };

nice



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:51-52
 
-lldb::addr_t DecodedThread::GetInstructionLoadAddress(size_t insn_index) const 
{
-  return m_instruction_ips[insn_index];
+lldb::addr_t DecodedThread::GetInstructionLoadAddress(size_t item_index) const 
{
+  return m_items[item_index].data.instruction.load_address;
 }

with this current implementation, it's the caller's responsibility to check the 
Item's type before calling this otherwise they will get a garbage load address 
if the item is actually an event for example.
Would it be better to enforce the validity of the Get call at this lowest level 
of storage by checking that the type matches what is expected and return None 
or something like LLDB_INVALID_ADDRESS if there's a mismatch?
same idea applies below for the other getters



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:84-86
+  m_items.emplace_back();
+  size_t index = m_items.size() - 1;
+  m_items[index].kind = kind;

nit: consider using `emplace` since this returns a ref to the item. or just 
create a constructor for the TraceItemKind that you can pass the kind to so you 
can set the kind in the emplace call and then there's no need for a reference 
to the newly inserted object

another suggestion would be to just make this method return a reference to the 
newly created TraceItemStorage since all the callers end up using the index to 
get access to the newly added item, so returning the ref would remove the need 
for those "redundant" lookups



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:245
+/// The TraceItemKind for each trace item encoded as uint8_t.
+uint8_t kind;
+union {

why not use the enum as the type?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:264
+
+  /// Most of the trace data is stored here.
+  std::vector m_items;

why is this "most"?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:265
+  /// Most of the trace data is stored here.
+  std::vector m_items;
 

nice, this is much more clear now (:



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:159-165
   case ptev_disabled:
 // The CPU paused tracing the program, e.g. due to ip filtering.
   case ptev_async_disabled:
 // The kernel or user code paused tracing the program, e.g.
 // a breakpoint or a ioctl invocation pausing the trace, or a
 // context switch happened.
+m_decoded_thread.AppendEvent(lldb::eTraceEventDisabled);

oooh, having a separate event for these two disabled cases would be provide 
great insight for us when trying to understand how tracing is enabled/disabled 
for user vs kernel mode (ie is it disabled by the hardware due to CPL filtering 
(ptev_disabled) or by the kernel/user (ptev_asyn_disabled)) - this is super 
cool and will be very useful!

not sure how we could cleanly expose this though since these different types of 
disables are pretty intelpt specific, but the notion of the traceevent is 
exposed at the top-level trace-agnostic tracecursor, right?
wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128576

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129340: [trace][intel pt] Create a CPU change event and expose it in the dumper

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

looks good overall, just a couple minor suggestions and questions




Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288
 
+decoded_thread.NotifyCPU(execution.thread_execution.cpu_id);
+

Does this mean our trace will always start with the CPU change event or would 
it be possible that we won't know the CPU id until the first context switch 
occurs?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:81-88
   return m_tsc_range->GetTsc();
 else
   return llvm::None;
   }
 }
 
+Optional TraceCursorIntelPT::GetCPU() const {

afaiu, we use pretty much the same data structures to keep track of TSCs and 
CPU Ids. Is there a reason that the TSC structures live on the trace cursor 
whereas the CPU id structures live on the decoded thread itself?
Wondering if it would make more sense to move the TSC structures off of the 
trace cursor and be consistent with how it's done for CPU Ids.
wdyt?



Comment at: lldb/source/Target/TraceDumper.cpp:137-141
   m_s << "(event) " << TraceCursor::EventKindToString(*item.event);
+  if (*item.event == eTraceEventCPUChanged) {
+m_s.Format(" [new CPU={0}]",
+   item.cpu_id ? std::to_string(*item.cpu_id) : "unavailable");
+  }

What about making an `EventToString` that abstracts away the details of how to 
print an event? Basically the same as `EventKindToString` but have it take in 
an item instead of event kind.
Otherwise, we will have to continue adding logic here for each new event/any 
modifications to existing events



Comment at: lldb/source/Target/TraceDumper.cpp:204-206
+m_j.attribute("event", TraceCursor::EventKindToString(*item.event));
+if (item.event == eTraceEventCPUChanged)
+  m_j.attribute("cpuId", item.cpu_id);

similar suggestions as above about potentially moving all of this logic into a 
single function, maybe in this case it could be `EventToJson` or something 
similar and it takes in a ref to the json as well as the item


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129340

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129257: [trace][intel pt] Add a cgroup filter

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

looks good overall, just a couple questions from my end




Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:87-90
+std::string slice = line.substr(line.find_first_of("/"));
+if (slice.empty())
+  return None;
+std::string cgroup_file = formatv("/sys/fs/cgroup/{0}", slice);

isn't the cgroup_file path going to have two slashes since slice starts with a 
slash?
{F23780471}
in the case of the image above, wouldn't cgroup_file be 
"/sys/fs/cgroup//foo.slice/bar.service" instead of 
"/sys/fs/cgroup/foo.slice/bar.service"




Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp:122
 } else {
-  m_unattributed_intelpt_subtraces++;
+  m_unattributed_psb_blocks++;
 }

Help me understand this. To me this seems like it is counting all of the PSBs 
that don't belong to **the current thread**, whereas I would expect this to 
only count the PSBs that don't belong to **any thread**?
So based on my understanding we would be severely overcounting the number of 
unattributed PSB, but I think I'm just misunderstanding how this code flows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129257

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

lgtm, two minor comments




Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:318
+should_copy = true;
+} else if (perf_record.IsErrorRecord()) {
+  should_copy = true;

why do we want the error record?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:323-325
+  for (size_t i = 0; i < perf_record.size; i++) {
+out_data.push_back(data[offset + i]);
+  }

consider using `std::copy` or `.insert` here instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129613: [trace][intel pt] Add a nice parser for the trace size

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added inline comments.
This revision is now accepted and ready to land.



Comment at: 
lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp:174
+
+  std::map multipliers = {{"mib", 1024 * 1024},
+ {"mb", 1024 * 1024},

nit



Comment at: 
lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp:175-181
+ {"mb", 1024 * 1024},
+ {"m", 1024 * 1024},
+ {"kib", 1024},
+ {"kb", 1024},
+ {"k", 1024},
+ {"b", 1},
+ {"", 1}};

nit: this may be overkill but consider using constants here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129613

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129249: [trace][intel pt] Measure the time it takes to decode a thread in per-cpu mode

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129249

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129340: [trace][intel pt] Create a CPU change event and expose it in the dumper

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

thanks for following up on those questions, lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129340

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129257: [trace][intel pt] Add a cgroup filter

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

thanks for answering those questions, lgtm




Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:87-90
+std::string slice = line.substr(line.find_first_of("/"));
+if (slice.empty())
+  return None;
+std::string cgroup_file = formatv("/sys/fs/cgroup/{0}", slice);

wallace wrote:
> jj10306 wrote:
> > isn't the cgroup_file path going to have two slashes since slice starts 
> > with a slash?
> > {F23780471}
> > in the case of the image above, wouldn't cgroup_file be 
> > "/sys/fs/cgroup//foo.slice/bar.service" instead of 
> > "/sys/fs/cgroup/foo.slice/bar.service"
> > 
> I think that's fine for system paths. You can have multiple consecutive  
> and the system collapses them
TIL



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp:122
 } else {
-  m_unattributed_intelpt_subtraces++;
+  m_unattributed_psb_blocks++;
 }

wallace wrote:
> jj10306 wrote:
> > Help me understand this. To me this seems like it is counting all of the 
> > PSBs that don't belong to **the current thread**, whereas I would expect 
> > this to only count the PSBs that don't belong to **any thread**?
> > So based on my understanding we would be severely overcounting the number 
> > of unattributed PSB, but I think I'm just misunderstanding how this code 
> > flows.
> the current code is correct, I'll explain:
> 
> - on_new_thread_execution is executed on all threads of the same CPU in 
> chronological order
> - as on_new_thread_execution is being invoked repeatedly, the `it` iterator 
> is being traversed and is always moving forwards
> - when on_new_thread_execution is invoked, the `it` iterator will look for 
> psb blocks that happened before the given execution. These blocks do not 
> belong to any thread execution. 
> 
> Graphically, we have
> 
> exec 1   ---exec 2 ---exec 3
> PSB1PSB2PSB3 PSB4   PSB5PSB6
> 
> when on_new_thread_execution is invoked for exec2, `it` will be pointing at 
> PSB3, which is the first PSB after exec 1. PSB3 comes before exec 2, so it'll 
> be unattributed, then it will move to PSB4 and so on
makes sense, thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129257

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:183
 
-DecodedThread::TscRange::TscRange(std::map::const_iterator 
it,
+DecodedThread::TscRange::TscRange(std::map::const_iterator it,
   const DecodedThread &decoded_thread)

What's purpose does the `TscRange` class serve? Specifically, why can't we take 
the same approach we take for CPU events where we just store a map and the 
chronologically last CPU ID on the `DecodedThread`:
```
// The cpu information is stored as a map. It maps `instruction index -> CPU`
// A CPU is associated with the next instructions that follow until the next 
cpu is seen.
std::map m_cpus;
/// This is the chronologically last CPU ID.
llvm::Optional m_last_cpu = llvm::None;
```
we currently store this same information for TSCs but then we have this whole 
additional `TscRange` notion that lives on a `TraceCursorIntelPT`

This is basically an extension of the conversation we had inline on 
(https://reviews.llvm.org/D129340) about eventually moving TSCs to be the same 
as CPUs. Perhaps now that we are making this change to treat TSCs like events 
(just like CPU changes) it would be a good time to revisit the design and make 
it simpler/consistent with CPU change events, if possible.

tl;dr iiuc the TSC lookup for an instruction is basically identical to the CPU 
ID lookup for an instruction, so why should the way we do the lookup be 
different for TSCs vs CPU IDs



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:191-197
+  TSC tsc_duration =
+  next_it == m_decoded_thread->m_timestamps.end()
+  ? (m_end_index + 1 - it->first) // for the last range, we assume each
+  // instruction took 1 TSC
+  : next_it->second - it->second;
+  m_tsc_duration_per_instruction =
+  static_cast(tsc_duration) / (m_end_index - m_it->first + 1);

I found this a little difficult to reason about.
Perhaps consistent use of the different iterator variables, minor renaming and 
docs may make this a little more understandable. Feel free to take none or any 
of the above suggestions 🙂



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:198-205
+  // duration_per_instruction will be greater than 1 if the PSBs are too spread
+  // out, e.g. when we missed instructions or there was a context switch in
+  // between. In this case, we change it to 1 to have a more real number. If
+  // duration_per_instruction is less than 1, then that means that multiple
+  // instructions were executed in the same tsc.
+  if (m_tsc_duration_per_instruction > 1)
+m_tsc_duration_per_instruction = 1;

Doesn't this check essentially make it so `m_tsc_duration_per_instruction` is 
either 0 or 1?
Where is the heuristic that every instruction should take <= 1 TSC to execute 
coming from?

From a visualization perspective,  how should we display instructions in a 
TscRange where `m_tsc_duration_per_instruction` is 0? Doesn't this force us to 
stack those instructions since will all have the same interpolated timestamp?




Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:275
   /// first one. Otherwise, this map will be empty.
-  std::map m_timestamps;
+  std::map m_timestamps;
   /// This is the chronologically last TSC that has been added.

Now that we are introducing timestamps (ie wall clock from TSCs), should we 
rename this to make it clear that these are TSC values and not timestamps?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130054

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

AFAICT this doesn't handle the TSCs we get for CPU change events through the 
context switch traces, is that correct?
This will be very useful to have for visualization purposes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130054

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288
 
+decoded_thread.NotifyTsc(execution.thread_execution.GetLowestKnownTSC());
 decoded_thread.NotifyCPU(execution.thread_execution.cpu_id);

if we have both a start and end TSC, shouldn't we emit the start here (like 
you're doing) and then after decoding all the instructions we should also emit 
the the end TSC?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:100-103
+// This is the last TSC range, so we have to extrapolate. In this case, we
+// assume that each instruction took one TSC, which is what an instruction
+// would take if no parallelism is achieved and the frequency multiplier
+// is 1.

Is this correct?
I don't see an issue with choosing a 1 TSC per instruction as a heuristic in 
this case, but we should be cautious not to explain this heuristic as being 
tied to any "expected" performance of the hardware because there are way too 
many variables that influence IPC like differences in workload, 
microarchitecture (speculative execution/branch prediction implementation, 
vector units, super scalar, etc), etc not to mention the difference b/t TSC and 
CPU cycles (like you mentioned).



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:105
+return m_tsc_conversion->ToNanosFloat(lowest_nanos,
+  range->tsc + items_since_last_tsc);
+  }

Won't our interpolation calculation be messed up since `items_since_last_tsc` 
will be counting all trace errors, events, etc and not just the 
instructions/events that we want to assign a timestamp to and thus should be 
part of this calculation?

I'm thinking of a case like below :
i - instruction, e - error

`tsc, cpu_change, i, e, e, e, tsc`
 in this case the items_since last_tsc would be 5 even though there is only 1 
instruction.

i guess this brings up a more general question around how timestamps should be 
assigned. For example, in this case I see a couple different options:
1. What we are doing now where we assign all events timestamps
Pro: easy implementation
Con: visualization may be weird bc timestamps are being influenced by events 
(ie errors) that users of the visualization don't care or know about
2. Ignore errors but treat all instructions and events the same (ie 
instructions and events get timestamps)
3. Ignore errors and treat instructions and events differently (ie instructions 
are the only things that contribute to `items_since_last_tsc`) and then events 
are simply assigned the same timestamp as an instruction

We can discuss this in more depth offline.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130054

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130580: Add string conversion for InstructionControlFlowKind enum

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: persona0220, wallace.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Refactor the string conversion of the `lldb::InstructionControlFlowKind` enum 
out
of `Instruction::Dump` to enable reuse of this logic by the
JSON TraceDumper (to be implemented in separate diff).

Will coordinate the landing of this change with D130320 
 since there will be a minor merge conflict 
between
these changes.

Test Plan:
Run unittests

  > ninja check-lldb-unittests
  [4/5] Running lldb unit test suite
  
  Testing Time: 10.13s
Passed: 1084

Verify '-k' flag's output

  (lldb) thread trace dump instructions -k
  thread #1: tid = 1375377
libstdc++.so.6`std::ostream::flush() + 43
  7048: 0x77b54dabreturn  retq
  7047: 0x77b54daaother   popq   %rbx
  7046: 0x77b54da7other   movq   %rbx, %rax
  7045: 0x77b54da5cond jump   je 0x11adb0  
; <+48>
  7044: 0x77b54da2other   cmpl   $-0x1, %eax
libc.so.6`_IO_fflush + 249
  7043: 0x77161729return  retq
  7042: 0x77161728other   popq   %rbp
  7041: 0x77161727other   popq   %rbx
  7040: 0x77161725other   movl   %edx, %eax
  7039: 0x77161721other   addq   $0x8, %rsp
  7038: 0x77161709cond jump   je 0x87721   
; <+241>
  7037: 0x77161707other   decl   (%rsi)
  7036: 0x771616fecond jump   je 0x87707   
; <+215>
  7035: 0x771616f7other   cmpl   $0x0, 0x33de92(%rip)  
; __libc_multiple_threads
  7034: 0x771616efother   movq   $0x0, 0x8(%rsi)
  7033: 0x771616edcond jump   jne0x87721   
; <+241>
  7032: 0x771616e9other   subl   $0x1, 0x4(%rsi)
  7031: 0x771616e2other   movq   0x88(%rbx), %rsi
  7030: 0x771616e0cond jump   jne0x87721   
; <+241>
  7029: 0x771616daother   testl  $0x8000, (%rbx)   
; imm = 0x8000


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130580

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Target/TraceDumper.cpp

Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -218,6 +218,11 @@
 m_j.attribute("mnemonic",
   ToOptionalString(item.symbol_info->instruction->GetMnemonic(
   &item.symbol_info->exe_ctx)));
+
+m_j.attribute(
+"mnemonic",
+ToOptionalString(item.symbol_info->instruction->GetMnemonic(
+&item.symbol_info->exe_ctx)));
   }
 
   if (IsLineEntryValid(item.symbol_info->sc.line_entry)) {
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -905,6 +905,30 @@
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+return "unknown";
+  case eInstructionControlFlowKindOther:
+return "other";
+  case eInstructionControlFlowKindCall:
+return "call";
+  case eInstructionControlFlowKindReturn:
+return "return";
+  case eInstructionControlFlowKindJump:
+return "jump";
+  case eInstructionControlFlowKindCondJump:
+return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+return "far call";
+  case eInstructionControlFlowKindFarReturn:
+return "far return";
+  case eInstructionControlFlowKindFarJump:
+return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
bool show_address, bool show_bytes,
bool show_control_flow_kind,
@@ -946,35 +970,10 @@
   }
 
   if (show_control_flow_kind) {
-switch (GetControlFlowKind(exe_ctx->GetTargetRef().GetArchitecture())) {
-case eInstructionControlFlowKindUnknown:
-  ss.Printf("%-12s", "unknown");
-  break;
-case eInstructionControlFlowKindOther:
-  ss.Printf("%-12s", "other");
-  break;
-case eInstructionControlFlowKindCall:
-  ss.Printf("%-12s", "call");
-  break;
-case eInstructionControlFlowKindReturn:
-  ss.Printf("%-12s", "return");
-  break;
-case eInstructionControlFlowKindJump:
-  ss.Pr

[Lldb-commits] [PATCH] D130580: Add string conversion for InstructionControlFlowKind enum

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 447727.
jj10306 added a comment.

remove accidentily included change to trace dumper


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130580

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Core/Disassembler.cpp

Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -905,6 +905,30 @@
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+return "unknown";
+  case eInstructionControlFlowKindOther:
+return "other";
+  case eInstructionControlFlowKindCall:
+return "call";
+  case eInstructionControlFlowKindReturn:
+return "return";
+  case eInstructionControlFlowKindJump:
+return "jump";
+  case eInstructionControlFlowKindCondJump:
+return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+return "far call";
+  case eInstructionControlFlowKindFarReturn:
+return "far return";
+  case eInstructionControlFlowKindFarJump:
+return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
bool show_address, bool show_bytes,
bool show_control_flow_kind,
@@ -946,35 +970,10 @@
   }
 
   if (show_control_flow_kind) {
-switch (GetControlFlowKind(exe_ctx->GetTargetRef().GetArchitecture())) {
-case eInstructionControlFlowKindUnknown:
-  ss.Printf("%-12s", "unknown");
-  break;
-case eInstructionControlFlowKindOther:
-  ss.Printf("%-12s", "other");
-  break;
-case eInstructionControlFlowKindCall:
-  ss.Printf("%-12s", "call");
-  break;
-case eInstructionControlFlowKindReturn:
-  ss.Printf("%-12s", "return");
-  break;
-case eInstructionControlFlowKindJump:
-  ss.Printf("%-12s", "jump");
-  break;
-case eInstructionControlFlowKindCondJump:
-  ss.Printf("%-12s", "cond jump");
-  break;
-case eInstructionControlFlowKindFarCall:
-  ss.Printf("%-12s", "far call");
-  break;
-case eInstructionControlFlowKindFarReturn:
-  ss.Printf("%-12s", "far return");
-  break;
-case eInstructionControlFlowKindFarJump:
-  ss.Printf("%-12s", "far jump");
-  break;
-}
+lldb::InstructionControlFlowKind instruction_control_flow_kind =
+GetControlFlowKind(exe_ctx->GetTargetRef().GetArchitecture());
+ss.Printf("%-12s", GetNameForInstructionControlFlowKind(
+   instruction_control_flow_kind));
   }
 
   const size_t opcode_pos = ss.GetSizeOfLastLine();
Index: lldb/include/lldb/lldb-enumerations.h
===
--- lldb/include/lldb/lldb-enumerations.h
+++ lldb/include/lldb/lldb-enumerations.h
@@ -972,6 +972,8 @@
 /// control flow of a trace.
 ///
 /// A single instruction can match one or more of these categories.
+/// The enum -> string conversion is in Disassembler.cpp, don't change
+/// this enum without updating that code as well.
 enum InstructionControlFlowKind {
   /// The instruction could not be classified.
   eInstructionControlFlowKindUnknown = 0,
Index: lldb/include/lldb/Core/Disassembler.h
===
--- lldb/include/lldb/Core/Disassembler.h
+++ lldb/include/lldb/Core/Disassembler.h
@@ -223,6 +223,9 @@
 
   virtual bool IsCall() { return false; }
 
+  static const char *GetNameForInstructionControlFlowKind(
+  lldb::InstructionControlFlowKind instruction_control_flow_kind);
+
 protected:
   Address m_address; // The section offset address of this instruction
  // We include an address class in the Instruction class to
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130580: Add string conversion for InstructionControlFlowKind enum

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Target/TraceDumper.cpp:221
   &item.symbol_info->exe_ctx)));
+
+m_j.attribute(

ignore this accidentally included change, will delete this and update the diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130580

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Core/Disassembler.cpp:930
+  }
+}
+

Thoughts on adding a default case with a string that indicates that no string 
conversion was implemented for the provided variant?
This would potentially make it easier for someone to realize they didn't add a 
case here for a newly added variant of InstructionControlFlowKind. Otherwise 
undefined data would be returned if a new case isn't added for a new variant.
wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130580

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 447802.
jj10306 added a comment.

Rebase, fix merge conflicts with D130320 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130580

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Core/Disassembler.cpp

Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -577,6 +577,30 @@
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+return "unknown";
+  case eInstructionControlFlowKindOther:
+return "other";
+  case eInstructionControlFlowKindCall:
+return "call";
+  case eInstructionControlFlowKindReturn:
+return "return";
+  case eInstructionControlFlowKindJump:
+return "jump";
+  case eInstructionControlFlowKindCondJump:
+return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+return "far call";
+  case eInstructionControlFlowKindFarReturn:
+return "far return";
+  case eInstructionControlFlowKindFarJump:
+return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
bool show_address, bool show_bytes,
bool show_control_flow_kind,
@@ -618,35 +642,10 @@
   }
 
   if (show_control_flow_kind) {
-switch (GetControlFlowKind(exe_ctx)) {
-case eInstructionControlFlowKindUnknown:
-  ss.Printf("%-12s", "unknown");
-  break;
-case eInstructionControlFlowKindOther:
-  ss.Printf("%-12s", "other");
-  break;
-case eInstructionControlFlowKindCall:
-  ss.Printf("%-12s", "call");
-  break;
-case eInstructionControlFlowKindReturn:
-  ss.Printf("%-12s", "return");
-  break;
-case eInstructionControlFlowKindJump:
-  ss.Printf("%-12s", "jump");
-  break;
-case eInstructionControlFlowKindCondJump:
-  ss.Printf("%-12s", "cond jump");
-  break;
-case eInstructionControlFlowKindFarCall:
-  ss.Printf("%-12s", "far call");
-  break;
-case eInstructionControlFlowKindFarReturn:
-  ss.Printf("%-12s", "far return");
-  break;
-case eInstructionControlFlowKindFarJump:
-  ss.Printf("%-12s", "far jump");
-  break;
-}
+lldb::InstructionControlFlowKind instruction_control_flow_kind =
+GetControlFlowKind(exe_ctx);
+ss.Printf("%-12s", GetNameForInstructionControlFlowKind(
+   instruction_control_flow_kind));
   }
 
   const size_t opcode_pos = ss.GetSizeOfLastLine();
Index: lldb/include/lldb/lldb-enumerations.h
===
--- lldb/include/lldb/lldb-enumerations.h
+++ lldb/include/lldb/lldb-enumerations.h
@@ -972,6 +972,8 @@
 /// control flow of a trace.
 ///
 /// A single instruction can match one or more of these categories.
+/// The enum -> string conversion is in Disassembler.cpp, don't change
+/// this enum without updating that code as well.
 enum InstructionControlFlowKind {
   /// The instruction could not be classified.
   eInstructionControlFlowKindUnknown = 0,
Index: lldb/include/lldb/Core/Disassembler.h
===
--- lldb/include/lldb/Core/Disassembler.h
+++ lldb/include/lldb/Core/Disassembler.h
@@ -226,6 +226,9 @@
 
   virtual bool IsCall() { return false; }
 
+  static const char *GetNameForInstructionControlFlowKind(
+  lldb::InstructionControlFlowKind instruction_control_flow_kind);
+
 protected:
   Address m_address; // The section offset address of this instruction
  // We include an address class in the Instruction class to
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 447804.
jj10306 marked 3 inline comments as done.
jj10306 added a comment.

Remove unnecessary comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130580

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/source/Core/Disassembler.cpp


Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -577,6 +577,30 @@
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+return "unknown";
+  case eInstructionControlFlowKindOther:
+return "other";
+  case eInstructionControlFlowKindCall:
+return "call";
+  case eInstructionControlFlowKindReturn:
+return "return";
+  case eInstructionControlFlowKindJump:
+return "jump";
+  case eInstructionControlFlowKindCondJump:
+return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+return "far call";
+  case eInstructionControlFlowKindFarReturn:
+return "far return";
+  case eInstructionControlFlowKindFarJump:
+return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
bool show_address, bool show_bytes,
bool show_control_flow_kind,
@@ -618,35 +642,10 @@
   }
 
   if (show_control_flow_kind) {
-switch (GetControlFlowKind(exe_ctx)) {
-case eInstructionControlFlowKindUnknown:
-  ss.Printf("%-12s", "unknown");
-  break;
-case eInstructionControlFlowKindOther:
-  ss.Printf("%-12s", "other");
-  break;
-case eInstructionControlFlowKindCall:
-  ss.Printf("%-12s", "call");
-  break;
-case eInstructionControlFlowKindReturn:
-  ss.Printf("%-12s", "return");
-  break;
-case eInstructionControlFlowKindJump:
-  ss.Printf("%-12s", "jump");
-  break;
-case eInstructionControlFlowKindCondJump:
-  ss.Printf("%-12s", "cond jump");
-  break;
-case eInstructionControlFlowKindFarCall:
-  ss.Printf("%-12s", "far call");
-  break;
-case eInstructionControlFlowKindFarReturn:
-  ss.Printf("%-12s", "far return");
-  break;
-case eInstructionControlFlowKindFarJump:
-  ss.Printf("%-12s", "far jump");
-  break;
-}
+lldb::InstructionControlFlowKind instruction_control_flow_kind =
+GetControlFlowKind(exe_ctx);
+ss.Printf("%-12s", GetNameForInstructionControlFlowKind(
+   instruction_control_flow_kind));
   }
 
   const size_t opcode_pos = ss.GetSizeOfLastLine();
Index: lldb/include/lldb/Core/Disassembler.h
===
--- lldb/include/lldb/Core/Disassembler.h
+++ lldb/include/lldb/Core/Disassembler.h
@@ -226,6 +226,9 @@
 
   virtual bool IsCall() { return false; }
 
+  static const char *GetNameForInstructionControlFlowKind(
+  lldb::InstructionControlFlowKind instruction_control_flow_kind);
+
 protected:
   Address m_address; // The section offset address of this instruction
  // We include an address class in the Instruction class to


Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -577,6 +577,30 @@
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+return "unknown";
+  case eInstructionControlFlowKindOther:
+return "other";
+  case eInstructionControlFlowKindCall:
+return "call";
+  case eInstructionControlFlowKindReturn:
+return "return";
+  case eInstructionControlFlowKindJump:
+return "jump";
+  case eInstructionControlFlowKindCondJump:
+return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+return "far call";
+  case eInstructionControlFlowKindFarReturn:
+return "far return";
+  case eInstructionControlFlowKindFarJump:
+return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
bool show_address, bool show_bytes,
bool show_control_flow_kind,
@@ -618,35 +642,10 @@
   }
 
   if (show_control_flow_kind) {
-switch (GetControlFlowKind(exe_ctx)) {
-case eInstructionControlFlowKindUnknown:
-  ss.Printf("%-12s", "unknown");
-  break;
-case eInstructionControlFlowKindOther:
-  ss.Printf("%-12s", "other");
-  break;
-case eInstructionControlFlowKindCall:
-  ss.Printf(

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd36ae4952d70: Add string conversion for 
InstructionControlFlowKind enum (authored by jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130580

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/source/Core/Disassembler.cpp


Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -577,6 +577,30 @@
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+return "unknown";
+  case eInstructionControlFlowKindOther:
+return "other";
+  case eInstructionControlFlowKindCall:
+return "call";
+  case eInstructionControlFlowKindReturn:
+return "return";
+  case eInstructionControlFlowKindJump:
+return "jump";
+  case eInstructionControlFlowKindCondJump:
+return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+return "far call";
+  case eInstructionControlFlowKindFarReturn:
+return "far return";
+  case eInstructionControlFlowKindFarJump:
+return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
bool show_address, bool show_bytes,
bool show_control_flow_kind,
@@ -618,35 +642,10 @@
   }
 
   if (show_control_flow_kind) {
-switch (GetControlFlowKind(exe_ctx)) {
-case eInstructionControlFlowKindUnknown:
-  ss.Printf("%-12s", "unknown");
-  break;
-case eInstructionControlFlowKindOther:
-  ss.Printf("%-12s", "other");
-  break;
-case eInstructionControlFlowKindCall:
-  ss.Printf("%-12s", "call");
-  break;
-case eInstructionControlFlowKindReturn:
-  ss.Printf("%-12s", "return");
-  break;
-case eInstructionControlFlowKindJump:
-  ss.Printf("%-12s", "jump");
-  break;
-case eInstructionControlFlowKindCondJump:
-  ss.Printf("%-12s", "cond jump");
-  break;
-case eInstructionControlFlowKindFarCall:
-  ss.Printf("%-12s", "far call");
-  break;
-case eInstructionControlFlowKindFarReturn:
-  ss.Printf("%-12s", "far return");
-  break;
-case eInstructionControlFlowKindFarJump:
-  ss.Printf("%-12s", "far jump");
-  break;
-}
+lldb::InstructionControlFlowKind instruction_control_flow_kind =
+GetControlFlowKind(exe_ctx);
+ss.Printf("%-12s", GetNameForInstructionControlFlowKind(
+   instruction_control_flow_kind));
   }
 
   const size_t opcode_pos = ss.GetSizeOfLastLine();
Index: lldb/include/lldb/Core/Disassembler.h
===
--- lldb/include/lldb/Core/Disassembler.h
+++ lldb/include/lldb/Core/Disassembler.h
@@ -226,6 +226,9 @@
 
   virtual bool IsCall() { return false; }
 
+  static const char *GetNameForInstructionControlFlowKind(
+  lldb::InstructionControlFlowKind instruction_control_flow_kind);
+
 protected:
   Address m_address; // The section offset address of this instruction
  // We include an address class in the Instruction class to


Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -577,6 +577,30 @@
   return m_address_class;
 }
 
+const char *Instruction::GetNameForInstructionControlFlowKind(
+lldb::InstructionControlFlowKind instruction_control_flow_kind) {
+  switch (instruction_control_flow_kind) {
+  case eInstructionControlFlowKindUnknown:
+return "unknown";
+  case eInstructionControlFlowKindOther:
+return "other";
+  case eInstructionControlFlowKindCall:
+return "call";
+  case eInstructionControlFlowKindReturn:
+return "return";
+  case eInstructionControlFlowKindJump:
+return "jump";
+  case eInstructionControlFlowKindCondJump:
+return "cond jump";
+  case eInstructionControlFlowKindFarCall:
+return "far call";
+  case eInstructionControlFlowKindFarReturn:
+return "far return";
+  case eInstructionControlFlowKindFarJump:
+return "far jump";
+  }
+}
+
 void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
bool show_address, bool show_bytes,
bool show_control_flow_kind,
@@ -618,35 +642,10 @@
   }
 
   if (show_control_flow_kind) {
-switch (GetControlFlowKind(exe_ctx)) {
-case eInstructionControlFlowKindUnknown:
-  ss.Printf("%-12s", "unknown");
-  break;
-case eInstructionControlFlowKindOther:
-  ss.Printf("%-12s", "other");
-  break;
-case eIn

[Lldb-commits] [PATCH] D130607: [trace] Add instruction control flow kind to JSON trace dumper's output

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: wallace, persona0220.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

D128477  adds a '-k' flag which displays each 
instruction's control flow in the `thread trace dump instructions` command's 
non-json  output (ie no '-j' or '-J' flag)
This diff adds the instruction control flow kind to the `thread trace dump 
instructions` command's JSON output (ie '-j' or '-J' flag)

Test Plan:
Confirm "controlFlowKind" is present in JSON when '-k' is provided

  (lldb) thread trace dump instructions -J -k
  [
{   


  [141/1952]
  "id": 7755,
  "loadAddress": "0x400868",
  "module": "test.out",
  "symbol": "main",
  "mnemonic": "jmp",
  "controlFlowKind": "jump",
  "source": "/home/jakobjohnson/jakob-dev/test.cpp",
  "line": 41,
  "column": 29
},
{
  "id": 7753,
  "loadAddress": "0x77b54dab",
  "module": "libstdc++.so.6",
  "symbol": "std::ostream::flush()",
  "mnemonic": "retq",
  "controlFlowKind": "return"
},
{
  "id": 7752,
  "loadAddress": "0x77b54daa",
  "module": "libstdc++.so.6",
  "symbol": "std::ostream::flush()",
  "mnemonic": "popq",
  "controlFlowKind": "other"
},
...
  ]

Confirm "controlFlowKind" is not present when '-k' isn't provided

  (lldb) thread trace dump instructions -J
  [
{
  "id": 7755,
  "loadAddress": "0x400868",
  "module": "test.out",
  "symbol": "main",
  "mnemonic": "jmp",
  "source": "/home/jakobjohnson/jakob-dev/test.cpp",
  "line": 41,
  "column": 29
},
{
  "id": 7753,
  "loadAddress": "0x77b54dab",
  "module": "libstdc++.so.6",
  "symbol": "std::ostream::flush()",
  "mnemonic": "retq"
},
{
  "id": 7752,
  "loadAddress": "0x77b54daa",
  "module": "libstdc++.so.6",
  "symbol": "std::ostream::flush()",
  "mnemonic": "popq"
},


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130607

Files:
  lldb/source/Target/TraceDumper.cpp


Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -199,6 +199,7 @@
   "column"?: decimal,
   "source"?: string,
   "mnemonic"?: string,
+  "controlFlowKind"?: string,
 }
   */
 public:
@@ -234,10 +235,18 @@
   "symbol",
   
ToOptionalString(item.symbol_info->sc.GetFunctionName().AsCString()));
 
-  if (item.symbol_info->instruction) {
+  if (lldb::InstructionSP instruction = item.symbol_info->instruction) {
+ExecutionContext exe_ctx = item.symbol_info->exe_ctx;
 m_j.attribute("mnemonic",
-  
ToOptionalString(item.symbol_info->instruction->GetMnemonic(
-  &item.symbol_info->exe_ctx)));
+  ToOptionalString(instruction->GetMnemonic(&exe_ctx)));
+if (m_options.show_control_flow_kind) {
+  lldb::InstructionControlFlowKind instruction_control_flow_kind =
+  instruction->GetControlFlowKind(&exe_ctx);
+  m_j.attribute("controlFlowKind",
+ToOptionalString(
+Instruction::GetNameForInstructionControlFlowKind(
+instruction_control_flow_kind)));
+}
   }
 
   if (IsLineEntryValid(item.symbol_info->sc.line_entry)) {


Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -199,6 +199,7 @@
   "column"?: decimal,
   "source"?: string,
   "mnemonic"?: string,
+  "controlFlowKind"?: string,
 }
   */
 public:
@@ -234,10 +235,18 @@
   "symbol",
   ToOptionalString(item.symbol_info->sc.GetFunctionName().AsCString()));
 
-  if (item.symbol_info->instruction) {
+  if (lldb::InstructionSP instruction = item.symbol_info->instruction) {
+ExecutionContext exe_ctx = item.symbol_info->exe_ctx;
 m_j.attribute("mnemonic",
-  ToOptionalString(item.symbol_info->instruction->GetMnemonic(
-  &item.symbol_info->exe_ctx)));
+  ToOptionalString(instruction->GetMnemonic(&exe_ctx)));
+if (m_options.show_control_flow_kind) {
+  lldb::InstructionControlFlowKind instruction_control_flow_kind =
+  instruction->GetControlFlowKind(&exe_ctx);
+   

[Lldb-commits] [PATCH] D130607: [trace] Add instruction control flow kind to JSON trace dumper's output

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdde3cf2e83d2: [trace] Add instruction control flow kind to 
JSON trace dumper's output (authored by jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130607

Files:
  lldb/source/Target/TraceDumper.cpp


Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -199,6 +199,7 @@
   "column"?: decimal,
   "source"?: string,
   "mnemonic"?: string,
+  "controlFlowKind"?: string,
 }
   */
 public:
@@ -234,10 +235,18 @@
   "symbol",
   
ToOptionalString(item.symbol_info->sc.GetFunctionName().AsCString()));
 
-  if (item.symbol_info->instruction) {
+  if (lldb::InstructionSP instruction = item.symbol_info->instruction) {
+ExecutionContext exe_ctx = item.symbol_info->exe_ctx;
 m_j.attribute("mnemonic",
-  
ToOptionalString(item.symbol_info->instruction->GetMnemonic(
-  &item.symbol_info->exe_ctx)));
+  ToOptionalString(instruction->GetMnemonic(&exe_ctx)));
+if (m_options.show_control_flow_kind) {
+  lldb::InstructionControlFlowKind instruction_control_flow_kind =
+  instruction->GetControlFlowKind(&exe_ctx);
+  m_j.attribute("controlFlowKind",
+ToOptionalString(
+Instruction::GetNameForInstructionControlFlowKind(
+instruction_control_flow_kind)));
+}
   }
 
   if (IsLineEntryValid(item.symbol_info->sc.line_entry)) {


Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -199,6 +199,7 @@
   "column"?: decimal,
   "source"?: string,
   "mnemonic"?: string,
+  "controlFlowKind"?: string,
 }
   */
 public:
@@ -234,10 +235,18 @@
   "symbol",
   ToOptionalString(item.symbol_info->sc.GetFunctionName().AsCString()));
 
-  if (item.symbol_info->instruction) {
+  if (lldb::InstructionSP instruction = item.symbol_info->instruction) {
+ExecutionContext exe_ctx = item.symbol_info->exe_ctx;
 m_j.attribute("mnemonic",
-  ToOptionalString(item.symbol_info->instruction->GetMnemonic(
-  &item.symbol_info->exe_ctx)));
+  ToOptionalString(instruction->GetMnemonic(&exe_ctx)));
+if (m_options.show_control_flow_kind) {
+  lldb::InstructionControlFlowKind instruction_control_flow_kind =
+  instruction->GetControlFlowKind(&exe_ctx);
+  m_j.attribute("controlFlowKind",
+ToOptionalString(
+Instruction::GetNameForInstructionControlFlowKind(
+instruction_control_flow_kind)));
+}
   }
 
   if (IsLineEntryValid(item.symbol_info->sc.line_entry)) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130924: [trace][intelpt] Update TraceIntelPTBundleSaver.cpp to accommodate FileSpec API changes

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: clayborg, wallace, persona0220.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

D130309  introduced changes to the FileSpec 
API which broke usages of
`GetCString()` in TraceIntelPTBundleSaver.cpp. This diff replaces usages
of `GetCString()` with `GetPath().c_str()` as suggested by D130309 
.

Test Plan:
Building with the trace plug-in now succeeds


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130924

Files:
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp


Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
@@ -103,7 +103,7 @@
 
   FileSpec threads_dir = directory;
   threads_dir.AppendPathComponent("threads");
-  sys::fs::create_directories(threads_dir.GetCString());
+  sys::fs::create_directories(threads_dir.GetPath().c_str());
 
   for (ThreadSP thread_sp : process.Threads()) {
 lldb::tid_t tid = thread_sp->GetID();
@@ -200,7 +200,7 @@
   std::vector json_cpus;
   FileSpec cpus_dir = directory;
   cpus_dir.AppendPathComponent("cpus");
-  sys::fs::create_directories(cpus_dir.GetCString());
+  sys::fs::create_directories(cpus_dir.GetPath().c_str());
 
   for (lldb::cpu_id_t cpu_id : trace_ipt.GetTracedCpus()) {
 JSONCpu json_cpu;


Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
@@ -103,7 +103,7 @@
 
   FileSpec threads_dir = directory;
   threads_dir.AppendPathComponent("threads");
-  sys::fs::create_directories(threads_dir.GetCString());
+  sys::fs::create_directories(threads_dir.GetPath().c_str());
 
   for (ThreadSP thread_sp : process.Threads()) {
 lldb::tid_t tid = thread_sp->GetID();
@@ -200,7 +200,7 @@
   std::vector json_cpus;
   FileSpec cpus_dir = directory;
   cpus_dir.AppendPathComponent("cpus");
-  sys::fs::create_directories(cpus_dir.GetCString());
+  sys::fs::create_directories(cpus_dir.GetPath().c_str());
 
   for (lldb::cpu_id_t cpu_id : trace_ipt.GetTracedCpus()) {
 JSONCpu json_cpu;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130925: [trace] Replace TraceCursorUP with TraceCursorSP

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: wallace, persona0220.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The use of `std::unique_ptr` with `TraceCursor` adds unnecessary complexity to 
adding `SBTraceCursor` bindings
Specifically, since `TraceCursor` is an abstract class there's no clean
way to provide "deep clone" semantics for `TraceCursorUP` short of
creating a pure virtual `clone()` method (afaict).

After discussing with @wallace, we decided there is no strong reason to
favor wrapping `TraceCursor` with `std::unique_ptr` over `std::shared_ptr`, 
thus this diff
replaces all usages of `std::unique_ptr` with 
`std::shared_ptr`.

This sets the stage for future diffs to introduce `SBTraceCursor`
bindings in a more clean fashion.

Test Plan:


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130925

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceDumper.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.cpp
  lldb/source/Target/TraceDumper.cpp

Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -295,32 +295,32 @@
 new OutputWriterCLI(s, options, thread));
 }
 
-TraceDumper::TraceDumper(lldb::TraceCursorUP &&cursor_up, Stream &s,
+TraceDumper::TraceDumper(lldb::TraceCursorSP cursor_sp, Stream &s,
  const TraceDumperOptions &options)
-: m_cursor_up(std::move(cursor_up)), m_options(options),
+: m_cursor_sp(std::move(cursor_sp)), m_options(options),
   m_writer_up(CreateWriter(
-  s, m_options, *m_cursor_up->GetExecutionContextRef().GetThreadSP())) {
+  s, m_options, *m_cursor_sp->GetExecutionContextRef().GetThreadSP())) {
 
   if (m_options.id)
-m_cursor_up->GoToId(*m_options.id);
+m_cursor_sp->GoToId(*m_options.id);
   else if (m_options.forwards)
-m_cursor_up->Seek(0, TraceCursor::SeekType::Beginning);
+m_cursor_sp->Seek(0, TraceCursor::SeekType::Beginning);
   else
-m_cursor_up->Seek(0, TraceCursor::SeekType::End);
+m_cursor_sp->Seek(0, TraceCursor::SeekType::End);
 
-  m_cursor_up->SetForwards(m_options.forwards);
+  m_cursor_sp->SetForwards(m_options.forwards);
   if (m_options.skip) {
-m_cursor_up->Seek((m_options.forwards ? 1 : -1) * *m_options.skip,
+m_cursor_sp->Seek((m_options.forwards ? 1 : -1) * *m_options.skip,
   TraceCursor::SeekType::Current);
   }
 }
 
 TraceDumper::TraceItem TraceDumper::CreatRawTraceItem() {
   TraceItem item = {};
-  item.id = m_cursor_up->GetId();
+  item.id = m_cursor_sp->GetId();
 
   if (m_options.show_timestamps)
-item.timestamp = m_cursor_up->GetWallClockTime();
+item.timestamp = m_cursor_sp->GetWallClockTime();
   return item;
 }
 
@@ -378,7 +378,7 @@
 }
 
 Optional TraceDumper::DumpInstructions(size_t count) {
-  ThreadSP thread_sp = m_cursor_up->GetExecutionContextRef().GetThreadSP();
+  ThreadSP thread_sp = m_cursor_sp->GetExecutionContextRef().GetThreadSP();
 
   SymbolInfo prev_symbol_info;
   Optional last_id;
@@ -386,32 +386,32 @@
   ExecutionContext exe_ctx;
   thread_sp->GetProcess()->GetTarget().CalculateExecutionContext(exe_ctx);
 
-  for (size_t insn_seen = 0; insn_seen < count && m_cursor_up->HasValue();
-   m_cursor_up->Next()) {
+  for (size_t insn_seen = 0; insn_seen < count && m_cursor_sp->HasValue();
+   m_cursor_sp->Next()) {
 
-last_id = m_cursor_up->GetId();
+last_id = m_cursor_sp->GetId();
 TraceItem item = CreatRawTraceItem();
 
-if (m_cursor_up->IsEvent()) {
+if (m_cursor_sp->IsEvent()) {
   if (!m_options.show_events)
 continue;
-  item.event = m_cursor_up->GetEventType();
+  item.event = m_cursor_sp->GetEventType();
   switch (*item.event) {
   case eTraceEventCPUChanged:
-item.cpu_id = m_cursor_up->GetCPU();
+item.cpu_id = m_cursor_sp->GetCPU();
 break;
   case eTraceEventHWClockTick:
-item.hw_clock = m_cursor_up->GetHWClock();
+item.hw_clock = m_cursor_sp->GetHWClock();
 break;
   case eTraceEventDisabledHW:
   case eTraceEventDisabledSW:
 break;
   }
-} else if (m_cursor_up->IsError()) {
-  item.error = m_cursor_up->GetError();
+} else if (m_cursor_sp->IsError()) {
+  item.error = m_cursor_sp->GetError();
 } else {
   insn_seen++;
-  item.load_address = m_cursor_up->GetLoadAddress();
+  item.load_address = m_cursor_sp->GetLoadAddress();
 
   if (!m_options.raw) {
 SymbolInfo symbol_info;
@@ -429,7 +429,7 @@
  

[Lldb-commits] [PATCH] D130924: [NFC][trace] Update TraceIntelPTBundleSaver.cpp to accommodate FileSpec API changes

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9bab358e3922: [trace][intelpt] Update 
TraceIntelPTBundleSaver.cpp to accommodate FileSpec API… (authored by jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130924

Files:
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp


Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
@@ -103,7 +103,7 @@
 
   FileSpec threads_dir = directory;
   threads_dir.AppendPathComponent("threads");
-  sys::fs::create_directories(threads_dir.GetCString());
+  sys::fs::create_directories(threads_dir.GetPath().c_str());
 
   for (ThreadSP thread_sp : process.Threads()) {
 lldb::tid_t tid = thread_sp->GetID();
@@ -200,7 +200,7 @@
   std::vector json_cpus;
   FileSpec cpus_dir = directory;
   cpus_dir.AppendPathComponent("cpus");
-  sys::fs::create_directories(cpus_dir.GetCString());
+  sys::fs::create_directories(cpus_dir.GetPath().c_str());
 
   for (lldb::cpu_id_t cpu_id : trace_ipt.GetTracedCpus()) {
 JSONCpu json_cpu;


Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
@@ -103,7 +103,7 @@
 
   FileSpec threads_dir = directory;
   threads_dir.AppendPathComponent("threads");
-  sys::fs::create_directories(threads_dir.GetCString());
+  sys::fs::create_directories(threads_dir.GetPath().c_str());
 
   for (ThreadSP thread_sp : process.Threads()) {
 lldb::tid_t tid = thread_sp->GetID();
@@ -200,7 +200,7 @@
   std::vector json_cpus;
   FileSpec cpus_dir = directory;
   cpus_dir.AppendPathComponent("cpus");
-  sys::fs::create_directories(cpus_dir.GetCString());
+  sys::fs::create_directories(cpus_dir.GetPath().c_str());
 
   for (lldb::cpu_id_t cpu_id : trace_ipt.GetTracedCpus()) {
 JSONCpu json_cpu;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: wallace, persona0220.
Herald added a subscriber: mgorny.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Add bindings for the `TraceCursor` to allow for programatic traversal of
traces.
This diff adds bindings for all public `TraceCursor` methods except
`GetHwClock` and also adds `SBTrace::CreateNewCursor`. A new unittest
has been added to TestTraceLoad.py that uses the new `SBTraceCursor` API
to test that the sequential and random access APIs of the `TraceCursor`
are equivalent.

This diff depends on D130925 .

Test Plan:
`ninja lldb-dotest && ./bin/lldb-dotest -p TestTraceLoad`


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130930

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/bindings/interface/SBTraceCursor.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/API/SBTraceCursor.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceCursor.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -264,3 +264,84 @@
 expected_substrs = ['error: missing value at traceBundle.processes[1].pid']
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 self.assertEqual(self.dbg.GetNumTargets(), 0)
+
+def testLoadTraceCursor(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+traceDescriptionFile = lldb.SBFileSpec(trace_description_file_path, True)
+
+error = lldb.SBError()
+trace = self.dbg.LoadTraceFromFile(error, traceDescriptionFile)
+self.assertSBError(error)
+
+target = self.dbg.GetSelectedTarget()
+process = target.process
+
+# Helper function to check equality of the current item of two trace cursors.
+def assertCurrentTraceCursorItemEqual(lhs, rhs):
+self.assertTrue(lhs.HasValue() and rhs.HasValue())
+
+self.assertEqual(lhs.GetId(), rhs.GetId())
+self.assertEqual(lhs.GetItemKind(), rhs.GetItemKind())
+if lhs.IsError():
+self.assertEqual(lhs.GetError(), rhs.GetError())
+elif lhs.IsEvent():
+self.assertEqual(lhs.GetEventType(), rhs.GetEventType())
+self.assertEqual(lhs.GetEventTypeAsString(), rhs.GetEventTypeAsString())
+elif lhs.IsInstruction():
+self.assertEqual(lhs.GetLoadAddress(), rhs.GetLoadAddress())
+else:
+self.fail("Unknown trace item kind")
+
+for thread in process.threads:
+sequentialTraversalCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Skip threads with no trace items
+if not sequentialTraversalCursor.HasValue():
+continue
+
+
+# Test "End" boundary of the trace by advancing past the trace's last item. 
+sequentialTraversalCursor.Seek(0, lldb.SBTraceCursor.End)
+self.assertTrue(sequentialTraversalCursor.HasValue())
+sequentialTraversalCursor.SetForwards(True)
+sequentialTraversalCursor.Next()
+self.assertFalse(sequentialTraversalCursor.HasValue())
+
+
+
+# Test sequential traversal using sequential access API (ie Next())
+# and random access API (ie GoToId()) simultaneously.
+randomAccessCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Reset the sequential cursor 
+sequentialTraversalCursor.Seek(0, lldb.SBTraceCursor.Beginning)
+sequentialTraversalCursor.SetForwards(True)
+self.assertTrue(sequentialTraversalCursor.IsForwards())
+
+while sequentialTraversalCursor.HasValue():
+itemId = sequentialTraversalCursor.GetId()
+randomAccessCursor.GoToId(itemId)
+assertCurrentTraceCursorItemEqual(sequentialTraversalCursor, randomAccessCursor)
+sequentialTraversalCursor.Next()
+
+
+
+# Test a random access with random access API (ie Seek()) and
+# sequential access API (ie consecutive calls to Next()).
+TEST_SEEK_ID = 3
+randomAccessCursor.GoToId(TEST_SEEK_ID )
+# Reset the sequential cursor 
+sequentialTraversalCursor.Seek(0, lldb.SBTraceCursor.Beginning)
+sequentialTraversalCursor.SetForwards(True)
+f

[Lldb-commits] [PATCH] D130925: [trace] Replace TraceCursorUP with TraceCursorSP

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3bec33b16db1: [trace] Replace TraceCursorUP with 
TraceCursorSP (authored by jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130925

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceDumper.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.cpp
  lldb/source/Target/TraceDumper.cpp

Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -295,32 +295,32 @@
 new OutputWriterCLI(s, options, thread));
 }
 
-TraceDumper::TraceDumper(lldb::TraceCursorUP &&cursor_up, Stream &s,
+TraceDumper::TraceDumper(lldb::TraceCursorSP cursor_sp, Stream &s,
  const TraceDumperOptions &options)
-: m_cursor_up(std::move(cursor_up)), m_options(options),
+: m_cursor_sp(std::move(cursor_sp)), m_options(options),
   m_writer_up(CreateWriter(
-  s, m_options, *m_cursor_up->GetExecutionContextRef().GetThreadSP())) {
+  s, m_options, *m_cursor_sp->GetExecutionContextRef().GetThreadSP())) {
 
   if (m_options.id)
-m_cursor_up->GoToId(*m_options.id);
+m_cursor_sp->GoToId(*m_options.id);
   else if (m_options.forwards)
-m_cursor_up->Seek(0, TraceCursor::SeekType::Beginning);
+m_cursor_sp->Seek(0, TraceCursor::SeekType::Beginning);
   else
-m_cursor_up->Seek(0, TraceCursor::SeekType::End);
+m_cursor_sp->Seek(0, TraceCursor::SeekType::End);
 
-  m_cursor_up->SetForwards(m_options.forwards);
+  m_cursor_sp->SetForwards(m_options.forwards);
   if (m_options.skip) {
-m_cursor_up->Seek((m_options.forwards ? 1 : -1) * *m_options.skip,
+m_cursor_sp->Seek((m_options.forwards ? 1 : -1) * *m_options.skip,
   TraceCursor::SeekType::Current);
   }
 }
 
 TraceDumper::TraceItem TraceDumper::CreatRawTraceItem() {
   TraceItem item = {};
-  item.id = m_cursor_up->GetId();
+  item.id = m_cursor_sp->GetId();
 
   if (m_options.show_timestamps)
-item.timestamp = m_cursor_up->GetWallClockTime();
+item.timestamp = m_cursor_sp->GetWallClockTime();
   return item;
 }
 
@@ -378,7 +378,7 @@
 }
 
 Optional TraceDumper::DumpInstructions(size_t count) {
-  ThreadSP thread_sp = m_cursor_up->GetExecutionContextRef().GetThreadSP();
+  ThreadSP thread_sp = m_cursor_sp->GetExecutionContextRef().GetThreadSP();
 
   SymbolInfo prev_symbol_info;
   Optional last_id;
@@ -386,32 +386,32 @@
   ExecutionContext exe_ctx;
   thread_sp->GetProcess()->GetTarget().CalculateExecutionContext(exe_ctx);
 
-  for (size_t insn_seen = 0; insn_seen < count && m_cursor_up->HasValue();
-   m_cursor_up->Next()) {
+  for (size_t insn_seen = 0; insn_seen < count && m_cursor_sp->HasValue();
+   m_cursor_sp->Next()) {
 
-last_id = m_cursor_up->GetId();
+last_id = m_cursor_sp->GetId();
 TraceItem item = CreatRawTraceItem();
 
-if (m_cursor_up->IsEvent()) {
+if (m_cursor_sp->IsEvent()) {
   if (!m_options.show_events)
 continue;
-  item.event = m_cursor_up->GetEventType();
+  item.event = m_cursor_sp->GetEventType();
   switch (*item.event) {
   case eTraceEventCPUChanged:
-item.cpu_id = m_cursor_up->GetCPU();
+item.cpu_id = m_cursor_sp->GetCPU();
 break;
   case eTraceEventHWClockTick:
-item.hw_clock = m_cursor_up->GetHWClock();
+item.hw_clock = m_cursor_sp->GetHWClock();
 break;
   case eTraceEventDisabledHW:
   case eTraceEventDisabledSW:
 break;
   }
-} else if (m_cursor_up->IsError()) {
-  item.error = m_cursor_up->GetError();
+} else if (m_cursor_sp->IsError()) {
+  item.error = m_cursor_sp->GetError();
 } else {
   insn_seen++;
-  item.load_address = m_cursor_up->GetLoadAddress();
+  item.load_address = m_cursor_sp->GetLoadAddress();
 
   if (!m_options.raw) {
 SymbolInfo symbol_info;
@@ -429,7 +429,7 @@
 }
 m_writer_up->TraceItem(item);
   }
-  if (!m_cursor_up->HasValue())
+  if (!m_cursor_sp->HasValue())
 m_writer_up->NoMoreData();
   return last_id;
 }
Index: lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.cpp
===
--- lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.cpp
+++ lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.cpp
@@ -81,7 +81,7 @@
 return false;
   } else {
 auto do_work = [&]() -> Error {
-  Expected cursor = trace_sp->CreateNewCursor(*thread);

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/API/SBTraceCursor.h:166
+  /// The specific kind of event the cursor is pointing at, or \b
+  /// TraceEvent::eTraceEventNone if the cursor not pointing to an event.
+  lldb::TraceEvent GetEventType() const;

wallace wrote:
> The event eTraceEventNone was deleted, so don't mention it here.
thanks - this is copypasta from TraceCursor.h, so I'll go ahead and fix this 
here and there



Comment at: lldb/source/API/SBTrace.cpp:47
+SBTraceCursor SBTrace::CreateNewCursor(SBError &error, SBThread &thread) {
+  LLDB_INSTRUMENT_VA(this);
+  if (!m_opaque_sp || !thread.get()) {

JDevlieghere wrote:
> There should be a newline after `LLDB_INSTRUMENT_VA` to match the output of 
> `lldb-instr`. 
thanks for catching this!



Comment at: lldb/source/API/SBTraceCursor.cpp:17-18
+
+// relevant to the clone issue:
+// 
https://stackoverflow.com/questions/16030081/copy-constructor-for-a-class-with-unique-ptr
+SBTraceCursor::SBTraceCursor() { LLDB_INSTRUMENT_VA(this); }

wallace wrote:
> no need to mention this. You can just create a invalid SBTraceCursor. Don't 
> forget to add the IsValid method
oops I added this as a reference when I was exploring adding the bindings 
before changing to using SP and forgot to delete it. will remove - thanks!



Comment at: lldb/source/API/SBTraceCursor.cpp:63-64
+  LLDB_INSTRUMENT_VA(this, offset, origin);
+  // Convert from public `SBTraceCursor::SeekType` enum to private
+  // `TraceCursor::SeekType` enum.
+  auto convert_seek_type_enum = [](SeekType seek_type) {

wallace wrote:
> You should instead create a new public enumeration TraceCursorSeekType. That 
> will simplify the code
I was leaning towards this while adding the bindings, so yea will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 449289.
jj10306 marked 4 inline comments as done.
jj10306 added a comment.

1. address comments
2. rebase
3. minor changes to `DecodedThread::GetInstructionLoadAddress` and 
`DecodedThread::GetErrorByIndex`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/bindings/interface/SBTraceCursor.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/API/SBTraceCursor.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-defines.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceCursor.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceDumper.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -264,3 +264,115 @@
 expected_substrs = ['error: missing value at traceBundle.processes[1].pid']
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 self.assertEqual(self.dbg.GetNumTargets(), 0)
+
+def testLoadTraceCursor(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+traceDescriptionFile = lldb.SBFileSpec(trace_description_file_path, True)
+
+error = lldb.SBError()
+trace = self.dbg.LoadTraceFromFile(error, traceDescriptionFile)
+self.assertSBError(error)
+
+target = self.dbg.GetSelectedTarget()
+process = target.process
+
+
+# 1. Test some expected items of thread 1's trace cursor.
+thread1 = process.threads[1]
+cursor = trace.CreateNewCursor(error, thread1) 
+self.assertTrue(cursor)
+self.assertTrue(cursor.HasValue())
+cursor.Seek(0, lldb.eTraceCursorSeekTypeBeginning)
+cursor.SetForwards(True)
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "HW clock tick")
+self.assertEqual(cursor.GetCPU(), lldb.LLDB_INVALID_CPU_ID)
+self.assertEqual(cursor.GetLoadAddress(), lldb.LLDB_INVALID_ADDRESS)
+
+cursor.Next()
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "CPU core changed")
+self.assertEqual(cursor.GetCPU(), 51)
+
+cursor.GoToId(19531)
+
+self.assertTrue(cursor.IsError())
+self.assertEqual(cursor.GetError(), "expected tracing enabled event")
+
+cursor.GoToId(19523)
+
+self.assertTrue(cursor.IsInstruction())
+self.assertEqual(cursor.GetLoadAddress(), 4197287)
+
+
+
+# Helper function to check equality of the current item of two trace cursors.
+def assertCurrentTraceCursorItemEqual(lhs, rhs):
+self.assertTrue(lhs.HasValue() and rhs.HasValue())
+
+self.assertEqual(lhs.GetId(), rhs.GetId())
+self.assertEqual(lhs.GetItemKind(), rhs.GetItemKind())
+if lhs.IsError():
+self.assertEqual(lhs.GetError(), rhs.GetError())
+elif lhs.IsEvent():
+self.assertEqual(lhs.GetEventType(), rhs.GetEventType())
+self.assertEqual(lhs.GetEventTypeAsString(), rhs.GetEventTypeAsString())
+elif lhs.IsInstruction():
+self.assertEqual(lhs.GetLoadAddress(), rhs.GetLoadAddress())
+else:
+self.fail("Unknown trace item kind")
+
+for thread in process.threads:
+sequentialTraversalCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Skip threads with no trace items
+if not sequentialTraversalCursor.HasValue():
+continue
+
+# 2. Test "End" boundary of the trace by advancing past the trace's last item. 
+sequentialTraversalCursor.Seek(0, lldb.eTraceCursorSeekTypeEnd)
+self.assertTrue(sequentialTraversalCursor.HasValue())
+sequentialTraversalCursor.SetForwards(True)
+sequentialTraversalCursor.Next()
+self.assertFalse(sequentialTraversalCursor.HasValue())
+
+
+
+# 3. Test sequential traversal using sequential access API (ie Next())
+# and random access API (ie GoToId()) simu

[Lldb-commits] [PATCH] D130805: [trace][intel pt] Support a new kernel section in LLDB’s trace bundle schema

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.

Nice work!
qq: Do we plan to add this kernel tracing support for live tracing as well?




Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:83
 JSONTraceBundleDescription &bundle_description, ArrayRef 
traced_processes,
-ArrayRef traced_threads) {
+ArrayRef traced_threads, TraceMode trace_mode) {
   TraceIntelPTSP trace_sp(new TraceIntelPT(bundle_description, 
traced_processes));

What's the `trace_mode` being used for? afaict it's unused in this method.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:176
 
+  enum TraceMode {
+UserMode,

wallace wrote:
> wallace wrote:
> > enum class
> @jj10306 , TraceMode or TracingMode?
I don't have a strong preference on the naming, but I was wondering how/if this 
enum is being used currently?
I understand this would be necessary when configuring a live tracing session bc 
this enum would control the configuration of the perf_event, but afaict this 
diff is only adding support for the kernel in the post mortem case so I don't 
quite understand how it's being used.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:191-199
   /// \param[in] traced_processes
   /// The processes traced in the live session.
   ///
   /// \param[in] trace_threads
   /// The threads traced in the live session. They must belong to the
   /// processes mentioned above.
   ///

Should these be for postmortem instead of live?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h:25
 const bool kDefaultDisableCgroupFiltering = false;
+const uint64_t kDefaultKernelLoadAddress = 0x8100;
 

wallace wrote:
> 
a couple questsions here:
1. where is this value coming from? Would be useful to provide a link to 
documentation
2. does this depend on the system?
3. I know you and @wallace were discussing the implications of ASLR offline, 
but curious what will happen if this load address is not correct due to ASLR or 
any other reason. Do we have a way to detect this?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:161-162
 return false;
+  // When kernel section is present, this is kernel mode tracing. Thus, throw 
an
+  // error if there is any user process or cpus section is not present.
+  if (bundle_description.kernel){





Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:61
   llvm::Optional tsc_perf_zero_conversion;
+  llvm::Optional kernel;
 

nit: any reason not to use reuse `JSONModule` here? The only difference I see 
is that the load address is optional. 
Not sure if there's a strong reason against this, but imo it should be the 
bundle creator's responsibility to provide a load address of the kernel and not 
LLDB's responsibility to use a default kernel load address.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130805

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:201
 lldb::TraceEvent DecodedThread::GetEventByIndex(int item_index) const {
+  // This currently returns an undefined value when the item isn't an event.
   return m_item_data[item_index].event;

what's the best return value if this is called and the item isn't an event?
ideally we could return something similar to LLDB_INVALID_ADDRESS, but since 
this is an enum we would need to add a new variant that represents the 
"invalid" case. Perhaps we could bring back the `eTraceEventNone` variant that 
was recently removed?
wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131005: [LLDB] Add SBInstruction::GetControlFlowKind()

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 created this revision.
jj10306 added reviewers: wallace, persona0220.
Herald added a project: All.
jj10306 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

D128477  adds the control flow kind for 
`Instruction` and displays this
in the `thread trace dump instruction -k` command.

This diff exposes the control flow kind via the new
`SBInstruction::GetControlFlowKind` method.

I've expanded `TestDisassembleRawData` to test this method, but please
let me know if there are any other unittests that should also be updated.

Test Plan:
`./bin/lldb-dotest -p TestDisassembleRawData`


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131005

Files:
  lldb/bindings/interface/SBInstruction.i
  lldb/include/lldb/API/SBInstruction.h
  lldb/source/API/SBInstruction.cpp
  lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py


Index: lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
===
--- lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
+++ lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
@@ -52,16 +52,26 @@
 self.assertEqual(inst.GetMnemonic(target), "move")
 self.assertEqual(inst.GetOperands(target),
 '$' + "fp, " + '$' + "sp")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 elif re.match("powerpc64le", arch):
 self.assertEqual(inst.GetMnemonic(target), "li")
 self.assertEqual(inst.GetOperands(target), "4, 0")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 elif arch in ("aarch64", "arm64"):
 self.assertEqual(inst.GetMnemonic(target), "mov")
 self.assertEqual(inst.GetOperands(target), "w0, #0x63")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 elif arch == "arm":
 self.assertEqual(inst.GetMnemonic(target), "mov")
 self.assertEqual(inst.GetOperands(target), "r3, #99")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 else:
 self.assertEqual(inst.GetMnemonic(target), "movq")
 self.assertEqual(inst.GetOperands(target),
 '%' + "rsp, " + '%' + "rbp")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindOther)
Index: lldb/source/API/SBInstruction.cpp
===
--- lldb/source/API/SBInstruction.cpp
+++ lldb/source/API/SBInstruction.cpp
@@ -164,6 +164,25 @@
   return nullptr;
 }
 
+lldb::InstructionControlFlowKind 
SBInstruction::GetControlFlowKind(lldb::SBTarget target) {
+  LLDB_INSTRUMENT_VA(this, target);
+
+  lldb::InstructionSP inst_sp(GetOpaque());
+  if (inst_sp) {
+ExecutionContext exe_ctx;
+TargetSP target_sp(target.GetSP());
+std::unique_lock lock;
+if (target_sp) {
+  lock = std::unique_lock(target_sp->GetAPIMutex());
+
+  target_sp->CalculateExecutionContext(exe_ctx);
+  exe_ctx.SetProcessSP(target_sp->GetProcessSP());
+}
+return inst_sp->GetControlFlowKind(&exe_ctx);
+  }
+  return lldb::eInstructionControlFlowKindUnknown;
+}
+
 size_t SBInstruction::GetByteSize() {
   LLDB_INSTRUMENT_VA(this);
 
Index: lldb/include/lldb/API/SBInstruction.h
===
--- lldb/include/lldb/API/SBInstruction.h
+++ lldb/include/lldb/API/SBInstruction.h
@@ -43,6 +43,8 @@
 
   const char *GetComment(lldb::SBTarget target);
 
+  lldb::InstructionControlFlowKind GetControlFlowKind(lldb::SBTarget target);
+
   lldb::SBData GetData(lldb::SBTarget target);
 
   size_t GetByteSize();
Index: lldb/bindings/interface/SBInstruction.i
===
--- lldb/bindings/interface/SBInstruction.i
+++ lldb/bindings/interface/SBInstruction.i
@@ -44,6 +44,9 @@
 const char *
 GetComment (lldb::SBTarget target);
 
+lldb::InstructionControlFlowKind
+GetControlFlowKind(lldb::SBTarget target);
+
 lldb::SBData
 GetData (lldb::SBTarget target);
 


Index: lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
===
--- lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
+++ lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
@@ -52,16 +52,26 @@
 self.assertEqual(inst.GetMnemonic(target), "move")
 self.assertEqual(inst.GetO

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 449461.
jj10306 marked 4 inline comments as done.
jj10306 added a comment.

Revert changes to DecodedThread trace item getter API's


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/bindings/interface/SBTraceCursor.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/API/SBTraceCursor.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-defines.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceCursor.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceDumper.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -264,3 +264,114 @@
 expected_substrs = ['error: missing value at traceBundle.processes[1].pid']
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 self.assertEqual(self.dbg.GetNumTargets(), 0)
+
+def testLoadTraceCursor(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+traceDescriptionFile = lldb.SBFileSpec(trace_description_file_path, True)
+
+error = lldb.SBError()
+trace = self.dbg.LoadTraceFromFile(error, traceDescriptionFile)
+self.assertSBError(error)
+
+target = self.dbg.GetSelectedTarget()
+process = target.process
+
+
+# 1. Test some expected items of thread 1's trace cursor.
+thread1 = process.threads[1]
+cursor = trace.CreateNewCursor(error, thread1) 
+self.assertTrue(cursor)
+self.assertTrue(cursor.HasValue())
+cursor.Seek(0, lldb.eTraceCursorSeekTypeBeginning)
+cursor.SetForwards(True)
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "HW clock tick")
+self.assertEqual(cursor.GetCPU(), lldb.LLDB_INVALID_CPU_ID)
+
+cursor.Next()
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "CPU core changed")
+self.assertEqual(cursor.GetCPU(), 51)
+
+cursor.GoToId(19531)
+
+self.assertTrue(cursor.IsError())
+self.assertEqual(cursor.GetError(), "expected tracing enabled event")
+
+cursor.GoToId(19523)
+
+self.assertTrue(cursor.IsInstruction())
+self.assertEqual(cursor.GetLoadAddress(), 4197287)
+
+
+
+# Helper function to check equality of the current item of two trace cursors.
+def assertCurrentTraceCursorItemEqual(lhs, rhs):
+self.assertTrue(lhs.HasValue() and rhs.HasValue())
+
+self.assertEqual(lhs.GetId(), rhs.GetId())
+self.assertEqual(lhs.GetItemKind(), rhs.GetItemKind())
+if lhs.IsError():
+self.assertEqual(lhs.GetError(), rhs.GetError())
+elif lhs.IsEvent():
+self.assertEqual(lhs.GetEventType(), rhs.GetEventType())
+self.assertEqual(lhs.GetEventTypeAsString(), rhs.GetEventTypeAsString())
+elif lhs.IsInstruction():
+self.assertEqual(lhs.GetLoadAddress(), rhs.GetLoadAddress())
+else:
+self.fail("Unknown trace item kind")
+
+for thread in process.threads:
+sequentialTraversalCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Skip threads with no trace items
+if not sequentialTraversalCursor.HasValue():
+continue
+
+# 2. Test "End" boundary of the trace by advancing past the trace's last item. 
+sequentialTraversalCursor.Seek(0, lldb.eTraceCursorSeekTypeEnd)
+self.assertTrue(sequentialTraversalCursor.HasValue())
+sequentialTraversalCursor.SetForwards(True)
+sequentialTraversalCursor.Next()
+self.assertFalse(sequentialTraversalCursor.HasValue())
+
+
+
+# 3. Test sequential traversal using sequential access API (ie Next())
+# and random access API (ie GoToId()) simultaneously.
+randomAccessCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Reset the seque

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 449462.
jj10306 added a comment.

nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/bindings/interface/SBTraceCursor.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/API/SBTraceCursor.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-defines.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceCursor.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceDumper.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -264,3 +264,114 @@
 expected_substrs = ['error: missing value at traceBundle.processes[1].pid']
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 self.assertEqual(self.dbg.GetNumTargets(), 0)
+
+def testLoadTraceCursor(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+traceDescriptionFile = lldb.SBFileSpec(trace_description_file_path, True)
+
+error = lldb.SBError()
+trace = self.dbg.LoadTraceFromFile(error, traceDescriptionFile)
+self.assertSBError(error)
+
+target = self.dbg.GetSelectedTarget()
+process = target.process
+
+
+# 1. Test some expected items of thread 1's trace cursor.
+thread1 = process.threads[1]
+cursor = trace.CreateNewCursor(error, thread1) 
+self.assertTrue(cursor)
+self.assertTrue(cursor.HasValue())
+cursor.Seek(0, lldb.eTraceCursorSeekTypeBeginning)
+cursor.SetForwards(True)
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "HW clock tick")
+self.assertEqual(cursor.GetCPU(), lldb.LLDB_INVALID_CPU_ID)
+
+cursor.Next()
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "CPU core changed")
+self.assertEqual(cursor.GetCPU(), 51)
+
+cursor.GoToId(19531)
+
+self.assertTrue(cursor.IsError())
+self.assertEqual(cursor.GetError(), "expected tracing enabled event")
+
+cursor.GoToId(19523)
+
+self.assertTrue(cursor.IsInstruction())
+self.assertEqual(cursor.GetLoadAddress(), 4197287)
+
+
+
+# Helper function to check equality of the current item of two trace cursors.
+def assertCurrentTraceCursorItemEqual(lhs, rhs):
+self.assertTrue(lhs.HasValue() and rhs.HasValue())
+
+self.assertEqual(lhs.GetId(), rhs.GetId())
+self.assertEqual(lhs.GetItemKind(), rhs.GetItemKind())
+if lhs.IsError():
+self.assertEqual(lhs.GetError(), rhs.GetError())
+elif lhs.IsEvent():
+self.assertEqual(lhs.GetEventType(), rhs.GetEventType())
+self.assertEqual(lhs.GetEventTypeAsString(), rhs.GetEventTypeAsString())
+elif lhs.IsInstruction():
+self.assertEqual(lhs.GetLoadAddress(), rhs.GetLoadAddress())
+else:
+self.fail("Unknown trace item kind")
+
+for thread in process.threads:
+sequentialTraversalCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Skip threads with no trace items
+if not sequentialTraversalCursor.HasValue():
+continue
+
+# 2. Test "End" boundary of the trace by advancing past the trace's last item. 
+sequentialTraversalCursor.Seek(0, lldb.eTraceCursorSeekTypeEnd)
+self.assertTrue(sequentialTraversalCursor.HasValue())
+sequentialTraversalCursor.SetForwards(True)
+sequentialTraversalCursor.Next()
+self.assertFalse(sequentialTraversalCursor.HasValue())
+
+
+
+# 3. Test sequential traversal using sequential access API (ie Next())
+# and random access API (ie GoToId()) simultaneously.
+randomAccessCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Reset the sequential cursor 
+sequentialTraversalCursor.Seek(0, lldb.eTraceCursorSeekTypeBeginnin

[Lldb-commits] [PATCH] D131005: [LLDB] Add SBInstruction::GetControlFlowKind()

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
jj10306 marked an inline comment as done.
Closed by commit rG6cbc6e9a6d5f: [LLDB] Add SBInstruction::GetControlFlowKind() 
(authored by jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131005

Files:
  lldb/bindings/interface/SBInstruction.i
  lldb/include/lldb/API/SBInstruction.h
  lldb/source/API/SBInstruction.cpp
  lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py


Index: lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
===
--- lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
+++ lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
@@ -52,16 +52,26 @@
 self.assertEqual(inst.GetMnemonic(target), "move")
 self.assertEqual(inst.GetOperands(target),
 '$' + "fp, " + '$' + "sp")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 elif re.match("powerpc64le", arch):
 self.assertEqual(inst.GetMnemonic(target), "li")
 self.assertEqual(inst.GetOperands(target), "4, 0")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 elif arch in ("aarch64", "arm64"):
 self.assertEqual(inst.GetMnemonic(target), "mov")
 self.assertEqual(inst.GetOperands(target), "w0, #0x63")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 elif arch == "arm":
 self.assertEqual(inst.GetMnemonic(target), "mov")
 self.assertEqual(inst.GetOperands(target), "r3, #99")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 else:
 self.assertEqual(inst.GetMnemonic(target), "movq")
 self.assertEqual(inst.GetOperands(target),
 '%' + "rsp, " + '%' + "rbp")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindOther)
Index: lldb/source/API/SBInstruction.cpp
===
--- lldb/source/API/SBInstruction.cpp
+++ lldb/source/API/SBInstruction.cpp
@@ -164,6 +164,25 @@
   return nullptr;
 }
 
+lldb::InstructionControlFlowKind 
SBInstruction::GetControlFlowKind(lldb::SBTarget target) {
+  LLDB_INSTRUMENT_VA(this, target);
+
+  lldb::InstructionSP inst_sp(GetOpaque());
+  if (inst_sp) {
+ExecutionContext exe_ctx;
+TargetSP target_sp(target.GetSP());
+std::unique_lock lock;
+if (target_sp) {
+  lock = std::unique_lock(target_sp->GetAPIMutex());
+
+  target_sp->CalculateExecutionContext(exe_ctx);
+  exe_ctx.SetProcessSP(target_sp->GetProcessSP());
+}
+return inst_sp->GetControlFlowKind(&exe_ctx);
+  }
+  return lldb::eInstructionControlFlowKindUnknown;
+}
+
 size_t SBInstruction::GetByteSize() {
   LLDB_INSTRUMENT_VA(this);
 
Index: lldb/include/lldb/API/SBInstruction.h
===
--- lldb/include/lldb/API/SBInstruction.h
+++ lldb/include/lldb/API/SBInstruction.h
@@ -43,6 +43,8 @@
 
   const char *GetComment(lldb::SBTarget target);
 
+  lldb::InstructionControlFlowKind GetControlFlowKind(lldb::SBTarget target);
+
   lldb::SBData GetData(lldb::SBTarget target);
 
   size_t GetByteSize();
Index: lldb/bindings/interface/SBInstruction.i
===
--- lldb/bindings/interface/SBInstruction.i
+++ lldb/bindings/interface/SBInstruction.i
@@ -44,6 +44,9 @@
 const char *
 GetComment (lldb::SBTarget target);
 
+lldb::InstructionControlFlowKind
+GetControlFlowKind(lldb::SBTarget target);
+
 lldb::SBData
 GetData (lldb::SBTarget target);
 


Index: lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
===
--- lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
+++ lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
@@ -52,16 +52,26 @@
 self.assertEqual(inst.GetMnemonic(target), "move")
 self.assertEqual(inst.GetOperands(target),
 '$' + "fp, " + '$' + "sp")
+self.assertEqual(inst.GetControlFlowKind(target),
+lldb.eInstructionControlFlowKindUnknown)
 elif re.match("powerpc64le", arch):
 self.assertEqual(inst.GetMnemonic(target), "li")
 self.assertEqual(inst.GetOperands(target), "4, 0")
+self.

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf9b4ea0ce9ef: [trace] Add SBTraceCursor bindings (authored 
by jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130930

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/bindings/interface/SBTraceCursor.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/API/SBTraceCursor.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-defines.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceCursor.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceDumper.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -264,3 +264,114 @@
 expected_substrs = ['error: missing value at traceBundle.processes[1].pid']
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 self.assertEqual(self.dbg.GetNumTargets(), 0)
+
+def testLoadTraceCursor(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+traceDescriptionFile = lldb.SBFileSpec(trace_description_file_path, True)
+
+error = lldb.SBError()
+trace = self.dbg.LoadTraceFromFile(error, traceDescriptionFile)
+self.assertSBError(error)
+
+target = self.dbg.GetSelectedTarget()
+process = target.process
+
+
+# 1. Test some expected items of thread 1's trace cursor.
+thread1 = process.threads[1]
+cursor = trace.CreateNewCursor(error, thread1) 
+self.assertTrue(cursor)
+self.assertTrue(cursor.HasValue())
+cursor.Seek(0, lldb.eTraceCursorSeekTypeBeginning)
+cursor.SetForwards(True)
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "HW clock tick")
+self.assertEqual(cursor.GetCPU(), lldb.LLDB_INVALID_CPU_ID)
+
+cursor.Next()
+
+self.assertTrue(cursor.IsEvent())
+self.assertEqual(cursor.GetEventTypeAsString(), "CPU core changed")
+self.assertEqual(cursor.GetCPU(), 51)
+
+cursor.GoToId(19531)
+
+self.assertTrue(cursor.IsError())
+self.assertEqual(cursor.GetError(), "expected tracing enabled event")
+
+cursor.GoToId(19523)
+
+self.assertTrue(cursor.IsInstruction())
+self.assertEqual(cursor.GetLoadAddress(), 4197287)
+
+
+
+# Helper function to check equality of the current item of two trace cursors.
+def assertCurrentTraceCursorItemEqual(lhs, rhs):
+self.assertTrue(lhs.HasValue() and rhs.HasValue())
+
+self.assertEqual(lhs.GetId(), rhs.GetId())
+self.assertEqual(lhs.GetItemKind(), rhs.GetItemKind())
+if lhs.IsError():
+self.assertEqual(lhs.GetError(), rhs.GetError())
+elif lhs.IsEvent():
+self.assertEqual(lhs.GetEventType(), rhs.GetEventType())
+self.assertEqual(lhs.GetEventTypeAsString(), rhs.GetEventTypeAsString())
+elif lhs.IsInstruction():
+self.assertEqual(lhs.GetLoadAddress(), rhs.GetLoadAddress())
+else:
+self.fail("Unknown trace item kind")
+
+for thread in process.threads:
+sequentialTraversalCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Skip threads with no trace items
+if not sequentialTraversalCursor.HasValue():
+continue
+
+# 2. Test "End" boundary of the trace by advancing past the trace's last item. 
+sequentialTraversalCursor.Seek(0, lldb.eTraceCursorSeekTypeEnd)
+self.assertTrue(sequentialTraversalCursor.HasValue())
+sequentialTraversalCursor.SetForwards(True)
+sequentialTraversalCursor.Next()
+self.assertFalse(sequentialTraversalCursor.HasValue())
+
+
+
+# 3. Test sequential traversal using sequential access API (ie Next())
+# and random access API (ie GoToId()) simultaneously.
+randomAccessCursor = trace.CreateNewCursor(error, thread) 
+self.assertSBError(error)
+# Reset the sequential

[Lldb-commits] [PATCH] D131630: [trace][intel pt] Fix per-psb packet decoding

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:164-166
+  /// \param[in] decoder
+  /// A decoder configured to start and end within the boundaries of the
+  /// given \p psb_block.

should this be trace_intel_pt?




Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:476-478
+  if (event.has_tsc) {
+tsc = event.tsc;
+break;

so is this inner loop what's actually doing the work of getting to the next 
PSB? is the assumption that if an event has a tsc then it's a PSB?
Can you explain what the two different while loops are doing?



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h:109
+llvm::Expected>
 SplitTraceInContinuousExecutions(TraceIntelPT &trace_intel_pt,
+ llvm::ArrayRef buffer,

should we rename now that we are using PSBBlock everywhere?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp:95
-static Expected>
+static Expected>
 GetIntelPTSubtracesForCpu(TraceIntelPT &trace, cpu_id_t cpu_id) {
-  std::vector intel_pt_subtraces;

rename?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131630: [trace][intel pt] Fix per-psb packet decoding

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:440
+Expected>
 lldb_private::trace_intel_pt::SplitTraceInContinuousExecutions(
+TraceIntelPT &trace_intel_pt, llvm::ArrayRef buffer,

consider renaming?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 415918.
jj10306 added a comment.

Fix minor comment issues


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

https://reviews.llvm.org/D121711

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Plugins/Process/Linux/CMakeLists.txt
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTCollector.h
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/unittests/Process/Linux/CMakeLists.txt
  lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
  lldb/unittests/Process/Linux/IntelPTManagerTests.cpp

Index: lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
===
--- lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
+++ lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
@@ -1,4 +1,4 @@
-//===-- IntelPTManagerTests.cpp ---===//
+//===-- IntelPTCollectorTests.cpp ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -8,7 +8,7 @@
 
 #include "gtest/gtest.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "llvm/ADT/ArrayRef.h"
 
 
Index: lldb/unittests/Process/Linux/CMakeLists.txt
===
--- lldb/unittests/Process/Linux/CMakeLists.txt
+++ lldb/unittests/Process/Linux/CMakeLists.txt
@@ -1,5 +1,5 @@
 add_lldb_unittest(TraceIntelPTTests
-  IntelPTManagerTests.cpp
+  IntelPTCollectorTests.cpp
 
   LINK_LIBS
 lldbPluginProcessLinux
Index: lldb/source/Target/TraceInstructionDumper.cpp
===
--- lldb/source/Target/TraceInstructionDumper.cpp
+++ lldb/source/Target/TraceInstructionDumper.cpp
@@ -183,7 +183,7 @@
 if (m_show_tsc) {
   s.Printf("[tsc=");
 
-  if (Optional timestamp = m_cursor_up->GetTimestampCounter())
+  if (Optional timestamp = m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
 s.Printf("0x%016" PRIx64, *timestamp);
   else
 s.Printf("unavailable");
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -28,7 +28,7 @@
 
   lldb::addr_t GetLoadAddress() override;
 
-  llvm::Optional GetTimestampCounter() override;
+  llvm::Optional GetCounter(lldb::TraceCounter counter_type) override;
 
   lldb::TraceInstructionControlFlowType
   GetInstructionControlFlowType() override;
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -85,8 +85,11 @@
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
-Optional TraceCursorIntelPT::GetTimestampCounter() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  switch (counter_type) {
+case lldb::eTraceCounterTSC:
+  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  }
 }
 
 TraceInstructionControlFlowType
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -20,7 +20,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-types.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "NativeThreadLinux.h"
 #include "Plugins/Process/POSIX/NativeProcessELF.h"
 #include "Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h"
@@ -241,7 +241,7 @@
   Status PopulateMemoryRegionCache();
 
   /// Manages Intel PT process and thread traces.
-  IntelPTManager m_intel_pt_manager;
+  IntelPTCollector m_intel_pt_collector;
 
   // Handle a clone()-like event.
   bool MonitorClone(NativeThreadLinux &parent, lldb::pid_t child_pid,
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.c

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 415960.
jj10306 added a comment.

rebase


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

https://reviews.llvm.org/D121711

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Plugins/Process/Linux/CMakeLists.txt
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTCollector.h
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/unittests/Process/Linux/CMakeLists.txt
  lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
  lldb/unittests/Process/Linux/IntelPTManagerTests.cpp

Index: lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
===
--- lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
+++ lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
@@ -1,4 +1,4 @@
-//===-- IntelPTManagerTests.cpp ---===//
+//===-- IntelPTCollectorTests.cpp ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -8,7 +8,7 @@
 
 #include "gtest/gtest.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "llvm/ADT/ArrayRef.h"
 
 
Index: lldb/unittests/Process/Linux/CMakeLists.txt
===
--- lldb/unittests/Process/Linux/CMakeLists.txt
+++ lldb/unittests/Process/Linux/CMakeLists.txt
@@ -1,5 +1,5 @@
 add_lldb_unittest(TraceIntelPTTests
-  IntelPTManagerTests.cpp
+  IntelPTCollectorTests.cpp
 
   LINK_LIBS
 lldbPluginProcessLinux
Index: lldb/source/Target/TraceInstructionDumper.cpp
===
--- lldb/source/Target/TraceInstructionDumper.cpp
+++ lldb/source/Target/TraceInstructionDumper.cpp
@@ -183,7 +183,7 @@
 if (m_show_tsc) {
   s.Printf("[tsc=");
 
-  if (Optional timestamp = m_cursor_up->GetTimestampCounter())
+  if (Optional timestamp = m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
 s.Printf("0x%016" PRIx64, *timestamp);
   else
 s.Printf("unavailable");
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -28,7 +28,7 @@
 
   lldb::addr_t GetLoadAddress() override;
 
-  llvm::Optional GetTimestampCounter() override;
+  llvm::Optional GetCounter(lldb::TraceCounter counter_type) override;
 
   lldb::TraceInstructionControlFlowType
   GetInstructionControlFlowType() override;
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -85,8 +85,11 @@
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
-Optional TraceCursorIntelPT::GetTimestampCounter() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  switch (counter_type) {
+case lldb::eTraceCounterTSC:
+  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  }
 }
 
 TraceInstructionControlFlowType
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -20,7 +20,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-types.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "NativeThreadLinux.h"
 #include "Plugins/Process/POSIX/NativeProcessELF.h"
 #include "Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h"
@@ -241,7 +241,7 @@
   Status PopulateMemoryRegionCache();
 
   /// Manages Intel PT process and thread traces.
-  IntelPTManager m_intel_pt_manager;
+  IntelPTCollector m_intel_pt_collector;
 
   // Handle a clone()-like event.
   bool MonitorClone(NativeThreadLinux &parent, lldb::pid_t child_pid,
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -312,7 +312,

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG22077627ae20: Minor refactor and renaming: (authored by 
jj10306).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121711

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Plugins/Process/Linux/CMakeLists.txt
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTCollector.h
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/unittests/Process/Linux/CMakeLists.txt
  lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
  lldb/unittests/Process/Linux/IntelPTManagerTests.cpp

Index: lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
===
--- lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
+++ lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
@@ -1,4 +1,4 @@
-//===-- IntelPTManagerTests.cpp ---===//
+//===-- IntelPTCollectorTests.cpp ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -8,7 +8,7 @@
 
 #include "gtest/gtest.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "llvm/ADT/ArrayRef.h"
 
 
Index: lldb/unittests/Process/Linux/CMakeLists.txt
===
--- lldb/unittests/Process/Linux/CMakeLists.txt
+++ lldb/unittests/Process/Linux/CMakeLists.txt
@@ -1,5 +1,5 @@
 add_lldb_unittest(TraceIntelPTTests
-  IntelPTManagerTests.cpp
+  IntelPTCollectorTests.cpp
 
   LINK_LIBS
 lldbPluginProcessLinux
Index: lldb/source/Target/TraceInstructionDumper.cpp
===
--- lldb/source/Target/TraceInstructionDumper.cpp
+++ lldb/source/Target/TraceInstructionDumper.cpp
@@ -183,7 +183,7 @@
 if (m_show_tsc) {
   s.Printf("[tsc=");
 
-  if (Optional timestamp = m_cursor_up->GetTimestampCounter())
+  if (Optional timestamp = m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
 s.Printf("0x%016" PRIx64, *timestamp);
   else
 s.Printf("unavailable");
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -28,7 +28,7 @@
 
   lldb::addr_t GetLoadAddress() override;
 
-  llvm::Optional GetTimestampCounter() override;
+  llvm::Optional GetCounter(lldb::TraceCounter counter_type) override;
 
   lldb::TraceInstructionControlFlowType
   GetInstructionControlFlowType() override;
Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -85,8 +85,11 @@
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
-Optional TraceCursorIntelPT::GetTimestampCounter() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+Optional TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  switch (counter_type) {
+case lldb::eTraceCounterTSC:
+  return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  }
 }
 
 TraceInstructionControlFlowType
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -20,7 +20,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-types.h"
 
-#include "IntelPTManager.h"
+#include "IntelPTCollector.h"
 #include "NativeThreadLinux.h"
 #include "Plugins/Process/POSIX/NativeProcessELF.h"
 #include "Plugins/Process/Utility/NativeProcessSoftwareSingleStep.h"
@@ -241,7 +241,7 @@
   Status PopulateMemoryRegionCache();
 
   /// Manages Intel PT process and thread traces.
-  IntelPTManager m_intel_pt_manager;
+  IntelPTCollector m_intel_pt_collector;
 
   // Handle a clone()-like event.
   bool MonitorClone(NativeThreadLinux &parent, lldb::pid_t child_pid,
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Pl

[Lldb-commits] [PATCH] D122023: [trace][intelpt] fix some test failures

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122023

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D121734: [trace][intelpt] Add thin wrapper for perf_event_open API

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe6c84f82b875: Add thin wrapper for perf_event_open API 
(authored by jj10306).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121734

Files:
  lldb/source/Plugins/Process/Linux/CMakeLists.txt
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTCollector.h
  lldb/source/Plugins/Process/Linux/Perf.cpp
  lldb/source/Plugins/Process/Linux/Perf.h
  lldb/unittests/Process/Linux/CMakeLists.txt
  lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
  lldb/unittests/Process/Linux/PerfTests.cpp

Index: lldb/unittests/Process/Linux/PerfTests.cpp
===
--- /dev/null
+++ lldb/unittests/Process/Linux/PerfTests.cpp
@@ -0,0 +1,85 @@
+//===-- PerfTests.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Perf.h"
+
+#include "llvm/Support/Error.h"
+
+#include "gtest/gtest.h"
+#include 
+#include 
+
+using namespace lldb_private;
+using namespace process_linux;
+using namespace llvm;
+
+/// Helper function to read current TSC value.
+///
+/// This code is based on llvm/xray.
+static Expected readTsc() {
+
+  unsigned int eax, ebx, ecx, edx;
+
+  // We check whether rdtscp support is enabled. According to the x86_64 manual,
+  // level should be set at 0x8001, and we should have a look at bit 27 in
+  // EDX. That's 0x800 (or 1u << 27).
+  __asm__ __volatile__("cpuid"
+   : "=a"(eax), "=b"(ebx), "=c"(ecx), "=d"(edx)
+   : "0"(0x8001));
+  if (!(edx & (1u << 27))) {
+return createStringError(inconvertibleErrorCode(),
+ "Missing rdtscp support.");
+  }
+
+  unsigned cpu;
+  unsigned long rax, rdx;
+
+  __asm__ __volatile__("rdtscp\n" : "=a"(rax), "=d"(rdx), "=c"(cpu)::);
+
+  return (rdx << 32) + rax;
+}
+
+// Test TSC to walltime conversion based on perf conversion values.
+TEST(Perf, TscConversion) {
+  // This test works by first reading the TSC value directly before
+  // and after sleeping, then converting these values to nanoseconds, and
+  // finally ensuring the difference is approximately equal to the sleep time.
+  //
+  // There will be slight overhead associated with the sleep call, so it isn't
+  // reasonable to expect the difference to be exactly equal to the sleep time.
+
+  const int SLEEP_SECS = 1;
+  std::chrono::nanoseconds SLEEP_NANOS{std::chrono::seconds(SLEEP_SECS)};
+
+  Expected params =
+  FetchPerfTscConversionParameters();
+
+  // Skip the test if the conversion parameters aren't available.
+  if (!params)
+GTEST_SKIP() << params.takeError();
+
+  Expected tsc_before_sleep = readTsc();
+  sleep(SLEEP_SECS);
+  Expected tsc_after_sleep = readTsc();
+
+  // Skip the test if we are unable to read the TSC value.
+  if (!tsc_before_sleep)
+GTEST_SKIP() << tsc_before_sleep.takeError();
+  if (!tsc_after_sleep)
+GTEST_SKIP() << tsc_after_sleep.takeError();
+
+  std::chrono::nanoseconds converted_tsc_diff =
+  params->ToWallTime(*tsc_after_sleep) -
+  params->ToWallTime(*tsc_before_sleep);
+
+  std::chrono::microseconds acceptable_overhead(500);
+
+  ASSERT_GE(converted_tsc_diff.count(), SLEEP_NANOS.count());
+  ASSERT_LT(converted_tsc_diff.count(),
+(SLEEP_NANOS + acceptable_overhead).count());
+}
Index: lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
===
--- lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
+++ lldb/unittests/Process/Linux/IntelPTCollectorTests.cpp
@@ -20,8 +20,8 @@
   size_t offset) {
   llvm::MutableArrayRef dst(reinterpret_cast(buf),
  buf_size);
-  llvm::MutableArrayRef src(reinterpret_cast(cyc_buf),
- cyc_buf_size);
+  llvm::ArrayRef src(reinterpret_cast(cyc_buf),
+  cyc_buf_size);
   IntelPTThreadTrace::ReadCyclicBuffer(dst, src, cyc_start, offset);
   return dst.size();
 }
Index: lldb/unittests/Process/Linux/CMakeLists.txt
===
--- lldb/unittests/Process/Linux/CMakeLists.txt
+++ lldb/unittests/Process/Linux/CMakeLists.txt
@@ -1,9 +1,10 @@
-add_lldb_unittest(TraceIntelPTTests
+add_lldb_unittest(ProcessLinuxTests
   IntelPTCollectorTests.cpp
+  PerfTests.cpp
 
   LINK_LIBS
 lldbPluginProcessLinux
   )
 
-target_include_directories(Trac

[Lldb-commits] [PATCH] D122176: [trace] clear any existing tracing sessions before relaunching the binary

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

nice catch, lgtm!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122176

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122254: [trace][intelpt] Introduce instruction Ids

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

Couple minor things




Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:66
 private:
+  /// Indicate the dumper that no more data is available in the trace.
+  /// This will prevent further iterations.





Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:54
 
-size_t TraceCursorIntelPT::Seek(int64_t offset, SeekType origin) {
+int64_t TraceCursorIntelPT::Seek(int64_t offset, SeekType origin) {
   int64_t last_index = GetInternalInstructionSize() - 1;

Should this return an unsigned int? Currently, it appears the return value will 
always be >= 0



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:33
+  } else if (forwards) {
+m_cursor_up->Seek(0, TraceCursor::SeekType::Set);
+  } else {

nit: Given that we have `SeekType::End`, why not rename `SeekType::Set`, to 
`SeekType::Start`? `SeekType::Set` was a little confusing to me the first time 
I read it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122254: [trace][intelpt] Introduce instruction Ids

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

nice work!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122293: [wip][intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:172
   std::vector m_instructions;
+  std::vector m_errors;
+

wallace wrote:
> you need to have something like
>   std::unordered_map m_errors;
> 
> that way, you'll be able to quickly look for the error associated with an 
> instruction index. The IntelPTInstruction int his case, instead of storing 
> the Error, can just store one bit of information has_error = true/false;
nit: from https://llvm.org/docs/CodingStandards.html#c-standard-library 
prefer `llvm::DenseMap` over `std::map`'s unless there's a specific reason not 
to.

Don't forget to update `CalculateApproximateMemoryUsage()` as well! Also, 
besides for being inline with the coding standards I linked above, using 
`llvm::DenseMap` here has the actual advantage that it exposes its approximate 
size via  `getMemorySize()`, whereas there is no easy way to get the size of 
`std::map`.



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:120
+  thread.AppendError(make_error(errcode));
+  // instructions.emplace_back(make_error(errcode));
   break;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122293: [wip][intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:152
+  ///   The errors of the trace.
+  std::unordered_map GetErrors() const;
+

Return a reference here to avoid potential expensive copy when returning.

Something else to consider is if we need/want an API exposing all the entire 
error map or if something like:
`llvm::Error GetErrorByInstructionIndex(uint64_t insn_index) const` that allows 
the caller to specify the key into the map would make more sense? 
This also has the advantage that it hides the implementation detail of what 
data type is being used under the hood to represent the error map!
@wallace @zrthxn wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122246: [trace][intelpt] Server side changes for TSC to wall time conversion

[Jakob Johnson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9b79187c96a3: [trace][intelpt] Server side changes for TSC 
to wall time conversion (authored by jj10306).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122246

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Utility/TraceGDBRemotePackets.h
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTCollector.h
  lldb/source/Plugins/Process/Linux/Perf.cpp
  lldb/source/Plugins/Process/Linux/Perf.h
  lldb/source/Plugins/Process/POSIX/ProcessPOSIXLog.h
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
  lldb/unittests/Process/Linux/PerfTests.cpp
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/TraceGDBRemotePacketsTest.cpp

Index: lldb/unittests/Utility/TraceGDBRemotePacketsTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/TraceGDBRemotePacketsTest.cpp
@@ -0,0 +1,101 @@
+//===-- TraceGDBRemotePacketsTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Utility/TraceIntelPTGDBRemotePackets.h"
+
+#include "gtest/gtest.h"
+
+#include 
+
+using namespace lldb_private;
+using namespace llvm;
+
+// Test serialization and deserialization of a non-empty
+// TraceIntelPTGetStateResponse.
+TEST(TraceGDBRemotePacketsTest, IntelPTGetStateResponse) {
+  // This test works as follows:
+  //  1. Create a non-empty TraceIntelPTGetStateResponse
+  //  2. Serialize to JSON
+  //  3. Deserialize the serialized JSON value
+  //  4. Ensure the original value and the deserialized value are equivalent
+  //
+  //  Notes:
+  //- We intentionally set an integer value out of its signed range
+  //  to ensure the serialization/deserialization isn't lossy since JSON
+  //  operates on signed values
+
+  // Choose arbitrary values for time_mult and time_shift
+  uint32_t test_time_mult = 1076264588;
+  uint16_t test_time_shift = 31;
+  // Intentionally set time_zero value out of the signed type's range.
+  uint64_t test_time_zero =
+  static_cast(std::numeric_limits::max()) + 1;
+
+  // Create TraceIntelPTGetStateResponse.
+  TraceIntelPTGetStateResponse response;
+  response.tsc_conversion = std::make_unique(
+  test_time_mult, test_time_shift, test_time_zero);
+
+  // Serialize then deserialize.
+  Expected deserialized_response =
+  json::parse(
+  llvm::formatv("{0}", toJSON(response)).str(),
+  "TraceIntelPTGetStateResponse");
+  if (!deserialized_response)
+FAIL() << toString(deserialized_response.takeError());
+
+  // Choose arbitrary TSC value to test the Convert function.
+  const uint64_t TSC = std::numeric_limits::max();
+  // Expected nanosecond value pre calculated using the TSC to wall time
+  // conversion formula located in the time_zero section of
+  // https://man7.org/linux/man-pages/man2/perf_event_open.2.html
+  const uint64_t EXPECTED_NANOS = 9223372039007304983u;
+
+  uint64_t pre_serialization_conversion =
+  response.tsc_conversion->Convert(TSC).count();
+  uint64_t post_serialization_conversion =
+  deserialized_response->tsc_conversion->Convert(TSC).count();
+
+  // Check equality:
+  // Ensure that both the TraceGetStateResponse and TraceIntelPTGetStateResponse
+  // portions of the JSON representation are unchanged.
+  ASSERT_EQ(toJSON(response), toJSON(*deserialized_response));
+  // Ensure the result of the Convert function is unchanged.
+  ASSERT_EQ(EXPECTED_NANOS, pre_serialization_conversion);
+  ASSERT_EQ(EXPECTED_NANOS, post_serialization_conversion);
+}
+
+// Test serialization and deserialization of an empty
+// TraceIntelPTGetStateResponse.
+TEST(TraceGDBRemotePacketsTest, IntelPTGetStateResponseEmpty) {
+  // This test works as follows:
+  //  1. Create an empty TraceIntelPTGetStateResponse
+  //  2. Serialize to JSON
+  //  3. Deserialize the serialized JSON value
+  //  4. Ensure the original value and the deserialized value are equivalent
+
+  // Create TraceIntelPTGetStateResponse.
+  TraceIntelPTGetStateResponse response;
+
+  // Serialize then deserialize.
+  Expected deserialized_response =
+  json::parse(
+  llvm::formatv("{0}", toJSON(response)).str(),
+  "TraceIntelPTGetStateResponse");
+  if (!deserialized_response)
+FAIL() << toString(deserialized_response.takeError());
+
+  // Check equality:
+  // Ensure that both the TraceGetStateResponse and TraceIntelPTGetState

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41
   m_pt_insn.iclass = ptic_error;
+  m_is_error = true;
 }

Is this boolean necessary? In the case of an error, the other two fields also 
indicate an error so this doesn't seem to add much information.
If we remove it, you can just update IsError to check the other two fields 
accordingly.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:40
 
-IntelPTInstruction::IntelPTInstruction(llvm::Error err) {
-  llvm::handleAllErrors(std::move(err),
-[&](std::unique_ptr info) {
-  m_error = std::move(info);
-});
+IntelPTInstruction::IntelPTInstruction(Error &err) {
   m_pt_insn.ip = LLDB_INVALID_ADDRESS;

Feels kinda weird that the `err` param isn't being used? Not sure if it would 
be preferred but another option would be to make the default constructor 
construct an error instruction.
Passing the error is more clear that this constructor will create an error 
instruction, but then it feels a bit unnecessary to pass it since it's now 
unused. I'm a little torn so would be curious to hear others opinions (:



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:156
+  template  void AppendInstruction(Ts... __args) {
+m_instructions.emplace_back(std::move(IntelPTInstruction(__args...)));
+  }

Should we be using `std::forward()` here? Same question for the `AppendError` 
function



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }

nit: should we update this to use the error map? I don't think there's a 
significant difference performance wise, but the code would be a little cleaner 
imo and consistent with how `GetError()` works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }

zrthxn wrote:
> jj10306 wrote:
> > nit: should we update this to use the error map? I don't think there's a 
> > significant difference performance wise, but the code would be a little 
> > cleaner imo and consistent with how `GetError()` works.
> That would sort of look like this I think
> 
> ```
>   if 
> (m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos).isA())
> return false;
>   else true;
> ```
What about 
`return (bool)m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);`
Another idea is to just remove the `IsError()` function entirely since calling 
`GetError()` tells you if it's an error. iirc all error checks actually use 
`GetError` except for the checks inside of `TraceHtr` which is soon going to be 
deleted by @wallace in new patches, so you could just change those couple 
instances of `IsError` and remove it all together. Definitely not necessary, 
just spitballing ideas (:
@wallace what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:145
+  private:
+friend class DecodedThread;
+

nit: No need to friend the enclosing class since C++11 - 
https://en.cppreference.com/w/cpp/language/nested_types


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

The changes to the decoder look sound, just left two questions related to it 
and a couple other nits.




Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:120
+
+void DecodedThread::ReportTscError(int libipt_error_code) { m_tsc_errors++; }
+

The parameter is unused, is there a reason to keep this or should it be removed 
since the method is simply incrementing the tsc error count?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:221
 
+  /// Return he number of TSC decoding errors that happened. A TSC error
+  /// is not a fatal error and doesn't create gaps in the trace. Instead





Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:106
+void RefreshTscInfo(TscInfo &tsc_info, pt_insn_decoder &decoder,
+DecodedThreadSP decoded_thread_sp) {
+  if (tsc_info.has_tsc == eLazyBoolNo)

see comment on `DecodeInstructions`



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:130
+
+static void AppendError(DecodedThreadSP &decoded_thread_sp, Error &&error,
+TscInfo &tsc_info) {

see comment on `DecodeInstructions`



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:138
+
+static void AppendInstruction(DecodedThreadSP &decoded_thread_sp,
+  const pt_insn &insn, TscInfo &tsc_info) {

see comment on `DecodeInstructions`



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:159
 static void DecodeInstructions(pt_insn_decoder &decoder,
-   DecodedThreadSP &decoded_thread_sp) {
-  while (true) {
-int errcode = FindNextSynchronizationPoint(decoder);
-if (errcode == -pte_eos)
-  break;
+   DecodedThreadSP decoded_thread_sp) {
 

This function isn't taking ownership/storing this SP so consider just passing a 
reference here and in the AppendError, AppendInstruction and RefreshTsc funcs



Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:192
+// We signal a gap only if it's not "end of stream"
+if (errcode != -pte_eos)
+  AppendError(decoded_thread_sp,

This makes sense to not include the errors if you are at the end of the stream, 
I have two questions related to this:
1. Prior to this change, was there always at least one error in the 
instructions from the eos that occurs when tracing? My understanding is that 
eos as a result of pt_insn_next is an indication that you are at the end of the 
buffer and thus the decoding is done, is that correct?
2. When a pt_insn_next call returns pte_eos, does that gurantee that the next 
call to FindNextSynchronizationPoint will return pte_eos as well? If so, could 
this code be changed to immediately return if pte_eos is returned here since 
currently it will break from the inner loop, go back to the top of the outer 
loop, call FindNextSynchronizationPoint which will ultimately return pte_eos 
which causes a break from the outer loop and finally the implicit return from 
the function? Seems like we could fail fast by immediately returning from the 
function here if pt_insn_next returns pte_eos.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122867

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:236-238
+  /// Notify this class that the last added instruction or error has
+  /// an associated TSC.
+  void ReportTscForLastInstruction(uint64_t tsc);

nit: When I initially read "Report" it made me think that the last TSC was 
being logged or reported to something else. Since all this method does is 
record the tsc of the last instruction, potentially "Record" is a better name. 
This is purely my opinion so feel free to keep it as is or change it (:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122867

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.

Looks good




Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:248
 
-  DecodeInstructions(*decoder, decoded_thread_sp);
+  DecodeInstructions(*decoder, *decoded_thread_sp.get());
   pt_insn_free_decoder(decoder);

Do you need .get() or does just `*decoded_thread` work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122867

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123106: [trace][intel pt] Create a class for the libipt decoder wrapper

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

Overall looks good, just a couple cosmetic things and comments about the plan 
for multi-buffer, single thread decoding




Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:41
+
+/// Class that decodes a raw buffer for a single thread using the low level
+/// libipt library.

"for a single thread"
thinking ahead for the multi-CPU buffer case - this class will be responsible 
for decoding a single trace buffer, not necessarily a single thread's buffer, 
right? Also, come multi-buffer time, the notion of `DecodeThread` in this class 
will need to be changed to `DecodedBuffer` or something similar that decouples 
the decoder from a particular thread.

No need to change this now but wanted to make sure I'm understanding the plan 
correctly.




Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:103
+  ///   The libipt decoder status after moving to the next PSB. Negative if
+  ///   not PSB was found.
+  int FindNextSynchronizationPoint() {





Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:112
+
+if (status != -pte_eos && IsLibiptError(status)) {
+  uint64_t decoder_offset = 0;





Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:152
+  status = pt_insn_event(&m_decoder, &event, sizeof(event));
+  if (status < 0)
+break;





Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:253
+using PtInsnDecoderUP =
+std::unique_ptr;
+

nice custom deleter (:



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h:23
+/// library underneath.
+void DecodeInMemoryTrace(DecodedThread &decoded_thread,
+ TraceIntelPT &trace_intel_pt,

If this file is just a wrapper around Libipt and doesn’t have any context about 
how LLDB gets a trace (from the aux buffer or a file), should we rename it to 
‘DecodeTrace’ and the caller is responsible for providing the trace data in a 
byte buffer?
In the case of in memory trace, the caller already has the byte buffer. In the 
case of a file, they could mmap the file and cast it to the appropriate type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123106

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123281: [trace][intel pt] Create a common accessor for live and postmortem data

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/include/lldb/Target/Trace.h:265
+  lldb::tid_t tid, llvm::StringRef kind,
+  std::function data)> callback);
+

typedef the callback to be cleaner and make the intention more clear?



Comment at: lldb/include/lldb/Target/Trace.h:385
+  /// tid -> data kind -> file
+  std::map>
+  m_postmortem_thread_data;

Can you explain what "kind" represents and why we need the nested map? Also, I 
see that for live tracing we have a map for both processes and threads, why is 
this not the case for post mortem?



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:271-274
+  // The libipt library does not modify the trace buffer, hence the
+  // following casts are safe.
+  config.begin = const_cast(buffer.data());
+  config.end = const_cast(buffer.data() + buffer.size());

The couple minor changes to Libiptdecoder aren't related to this diff, maybe 
move these to D123106.



Comment at: lldb/source/Target/Trace.cpp:265
+
+  MemoryBuffer &data = **trace_or_error;
+  ArrayRef array_ref(

is guaranteed that the UP will be non-null? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123281

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123281: [trace][intel pt] Create a common accessor for live and postmortem data

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/include/lldb/Target/Trace.h:386-396
+  llvm::DenseMap>
   m_live_thread_data;
+
   /// data kind -> size
   std::unordered_map m_live_process_data;
+  /// \}
+

Why not change all the maps to DenseMap while we're at it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123281

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123358: [trace][intelpt] Remove code smell when printing the raw trace size

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

Looks good. Any reason not to have `GetRawTraceSize()` at the `Trace` interface 
level instead of being specific to `TraceIntelPT`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123358

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123357: [trace][intelpt] Add task timer classes

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:243
+  /// Total amount of time spent decoding.
+  std::chrono::milliseconds m_total_decoding_time{0};
 };

nit: Do you need an explicit initialization?



Comment at: lldb/source/Plugins/Trace/intel-pt/TaskTimer.cpp:17
+
+ThreadTaskTimer &TaskTimer::ForThread(lldb::tid_t tid) {
+  auto it = m_thread_timers.find(tid);

Should this be named to better reflect what this function is doing? It seems 
that this function is "getting" the ThreadTaskTimer associated with the tid and 
creates it if one doesn't already exist.
Maybe something like `GetThreadTaskTimer`, `TimerForThread`, `TimerByThread` ?



Comment at: lldb/source/Plugins/Trace/intel-pt/TaskTimer.h:58
+private:
+  std::unordered_map m_timed_tasks;
+};




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123357

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/include/lldb/lldb-enumerations.h:1151
+// Events that might happen during a trace session.
+FLAGS_ENUM(TraceEvents){
+// Tracing was paused. If instructions were executed after pausing

What are some other events you could forsee adding to this enum?



Comment at: lldb/include/lldb/lldb-enumerations.h:1155
+// should provide an error signalinig this data loss.
+eTraceEventPaused = (1u << 0),
+};

Will this paused event always be due to a context switch? If so, we should 
rename it as such to reduce ambiguity



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:137
 
-void DecodedThread::AppendError(llvm::Error &&error, uint64_t tsc) {
+void DecodedThread::SetAsFailed(llvm::Error &&error) {
   AppendError(std::move(error));

What is being "Set" and why have the extra layer of indirection with the 
private `AppendError(Error &&e)`? The name is a little confusing imo because 
all this does is call `Append` internally.
Any reason to not have `AppendError` public and call that directly?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:75
+
+  /// \return \b true iff this struct holds a libipt error.
+  explicit operator bool() const;





Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:225
+  ///   The number of instructions with associated events.
+  size_t GetInstructionsWithEventsCount() const;
+

What value does this providing the total number of instructions with events 
add? I think it could be more useful to provide a specific event as a parameter 
and return the total number of instructions with that particular event. wdyt?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.

I know this isn't related to these changes, but this should be updated to be 
consistent with the other `instruction_index -> XXX` maps in this class.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.

jj10306 wrote:
> I know this isn't related to these changes, but this should be updated to be 
> consistent with the other `instruction_index -> XXX` maps in this class.




Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:68
+  /// instruction in the given \a DecodedInstruction.
+  ///
+  /// \return

consider adding docs on `insn` being an "out" parameter so it's a little more 
clear



Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:186
+// were lost.
+insn.libipt_error = -pte_overflow;
+break;

Why have the internal buffer overflow event as an error instead of a variant of 
the newly introduced Events enum?



Comment at: lldb/test/API/commands/trace/TestTraceDumpInfo.py:52
+  Events:
+Total number of instructions with events: 1
+

Will this always be 1 or is there a possibility that it could be different 
depending on the number of context switches?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123984: [trace][intel pt] Add a memory usage test

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

What is the intention of this test? To catch changes that may increase the 
memory by a lot? I'm concerned this could be very flaky given that it's 
dependent on external factors (compiler output, context switches, etc).
Now that we have this test, is there any value in having the check for the 
memory information output in the trace dump test or can that be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123984

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124064: [NFC][trace] simplify the instruction dumper

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

two very minor comments, but looks good - I'm sure you already have, but be 
sure to run the dumper unittest test to ensure the output didn't change 
unexpectedly as a result some minor formatting mistake in this diff 🙂




Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:26
+  lldb::InstructionSP instruction;
+  lldb_private::ExecutionContext exe_ctx;
+};

It shouldn't be an issue now because this struct is never stored any where, but 
in the future we should be aware that if this struct is ever stored "long term" 
(ie in another class), we should instead use `ExececutionContextRef`
From the documentation in `ExecutionContext.h`
```
/// exist during a function that requires the objects. ExecutionContext
/// objects should NOT be used for long term storage since they will keep
/// objects alive with extra shared pointer references to these  objects
```



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:199
+/// instruction's disassembler when possible.
+std::tuple
+CalculateDisass(const InstructionSymbolInfo &insn_info,

should this be a static function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124064

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/include/lldb/Target/TraceCursor.h:265
 
+class TraceEventsUtils {
+public:

nit: maybe this will change in the future to where this will have data and 
instance members, but if not, consider just using a namespace here to house the 
utility functions since the class isn't really providing any state?



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:75
+
+  /// \return \b true iff this struct holds a libipt error.
+  explicit operator bool() const;

wallace wrote:
> jj10306 wrote:
> > 
> oh, i wanted to mean if and only if. Or is that too mathematical and less 
> colloquial? 
Lol my first thought actually was "I wonder if he meant if and only if, so this 
isn't a typo".
Up to you if you want to keep if or leave it as iff 



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.

wallace wrote:
> jj10306 wrote:
> > jj10306 wrote:
> > > I know this isn't related to these changes, but this should be updated to 
> > > be consistent with the other `instruction_index -> XXX` maps in this 
> > > class.
> > 
> in this case we can't do that, because when doing random accesses in the 
> trace, we need to quickly find the most recent TSC for the new instruction, 
> which is done with binary search using a map. This is impossible in a hash 
> map like DenseMap
I know this is a pedantic discussion, but isn't lookup in hash tables amortized 
O(1) (assuming a good hash that doesn't take too long to execute or produce the 
same hash for different values) whereas in an ordered map it's O(logn)?
IIUC, you should only use an ordered map when you rely on the order property of 
it.

In any case, we should use `uint64_t` here instead of `size_t` to be consistent 
with the type for instruction index used in the other maps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

Thanks for making those changes - lgtm!




Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.

wallace wrote:
> jj10306 wrote:
> > wallace wrote:
> > > jj10306 wrote:
> > > > jj10306 wrote:
> > > > > I know this isn't related to these changes, but this should be 
> > > > > updated to be consistent with the other `instruction_index -> XXX` 
> > > > > maps in this class.
> > > > 
> > > in this case we can't do that, because when doing random accesses in the 
> > > trace, we need to quickly find the most recent TSC for the new 
> > > instruction, which is done with binary search using a map. This is 
> > > impossible in a hash map like DenseMap
> > I know this is a pedantic discussion, but isn't lookup in hash tables 
> > amortized O(1) (assuming a good hash that doesn't take too long to execute 
> > or produce the same hash for different values) whereas in an ordered map 
> > it's O(logn)?
> > IIUC, you should only use an ordered map when you rely on the order 
> > property of it.
> > 
> > In any case, we should use `uint64_t` here instead of `size_t` to be 
> > consistent with the type for instruction index used in the other maps.
> > In any case, we should use uint64_t here instead of size_t to be consistent 
> > with the type for instruction index used in the other maps.
> 
> you are right with this one.
> 
> > IIUC, you should only use an ordered map when you rely on the order 
> > property of it.
> 
> yes, and that's what we use for the timestamps. As there are a small number 
> of unique timestamps, we are keeping this map 'insn_index -> timestamp'. We 
> need its order property because when we do GoToId(insn_id) we need to look 
> for most recent entry in the map that has a timestamp, and we use that 
> timestamp for our new instruction. We use map.lower_bound for that. With a 
> hashmap we couldn't do that.
>We use map.lower_bound for that. With a hashmap we couldn't do that.
Ahhh yes, that makes sense - I forgot we were using that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:22
+// List of data kinds used by jLLDBGetState and jLLDBGetBinaryData.
+struct IntelPTDataKinds {
+  static const char *kProcFsCpuInfo;

why not use an enum here?
having "kinds" in the name already makes it sound like an enum and seeing 
IntelPTDataKinds:: everywhere immediately made me thing it was an enum.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124573

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added inline comments.



Comment at: lldb/unittests/Process/Linux/PerfTests.cpp:53
 
+TEST(Perf, HardcodedLogicalCoreIDs) {
+  Expected> core_ids =

Not directly related to this diff, but I just realized the proc fs parsing 
logic doesn't belong in the Perf files since it has nothing to do with perf. 
Feel free to move those functions and these associated tests to a more 
appropriate location or we can do it in a future diff since this isn't the diff 
that made that mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124573

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124640: [trace][intelpt] Support system-wide tracing [2] - Add a dummy --per-core-tracing option

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

Before doing a complete review can you provide clarity on the decision to only 
support perCore for process wide (see my inline comment with my 
thoughts/questions)?  Understanding this will potentially provide the answers 
to many other questions I was going to leave, so figured I'd ask it ahead of 
time (:




Comment at: lldb/docs/lldb-gdb-remote.txt:369
+//
+// /* process tracing only */
 // "processBufferSizeLimit": ,

Why do we only want this option for process tracing?
Per cpu tracing collects all trace data agnostic to a user specified 
process/thread, so why should this only be exposed for process wide? I think it 
makes more sense to decouple the `perCoreTracing` option from process/threads 
specific options entirely so it is its own option all together and cannot be 
used in conjunction with process/thread options.
If there is reason to not go down that route, we then should also add support 
for `perCoreTracing` with the  thread tracing option, not just the process 
tracing option as I feel it doesn't make since to only expose this for process 
tracing since it's doing the same thing behind the scenes.



Comment at: 
lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp:66
 OptionParsingStarting(ExecutionContext *execution_context) {
-  m_thread_buffer_size = kDefaultThreadBufferSize;
+  m_trace_buffer_size = kDefaultTraceBufferSize;
   m_enable_tsc = kDefaultEnableTscValue;

So now m_trace_buffer_size is the size of each trace buffer, regardless of the 
buffer is for a single thread or a single core?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124640

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124648: [trace][intelpt] Support system-wide tracing [3] - Refactor IntelPTThreadTrace

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/docs/lldb-gdb-remote.txt:498
 //  Binary data kinds:
-//- threadTraceBuffer: trace buffer for a thread.
+//- traceBuffer: trace buffer for a thread.
 //- procFsCpuInfo: contents of the /proc/cpuinfo file.

If perCore tracing is enabled, how will this packet work since currently it 
requires a tid, but in perCore mode the trace data will contain all activity on 
that core, not just the specified thread?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:136
-
 /// Manages a list of thread traces.
 class IntelPTThreadTraceCollection {

update doc since this is no longer tied to thread's traces



Comment at: 
lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:292-298
+  if (Expected perf_event = PerfEvent::Init(*attr, tid)) {
+if (Error mmap_err = perf_event->MmapMetadataAndBuffers(buffer_numpages,
+buffer_numpages)) {
+  return std::move(mmap_err);
+}
+return IntelPTSingleBufferTraceUP(
+new IntelPTSingleBufferTrace(std::move(*perf_event), tid));

The PerfEvent logic will need to be updated to support per CPU or per thread as 
it currently only supports per thread



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:1
+//===-- IntelPTSingleBufferTrace.h  -*- C++ 
-*-===//
+//

nit: thoughts on the name  `IntelPTTraceBuffer` instead of 
`IntelPTSingleBufferTrace`? The current name is a bit wordy imo



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:41-48
+  /// \param[in] tid
+  /// The tid of the thread to be traced.
+  ///
+  /// \return
+  ///   A \a IntelPTSingleBufferTrace instance if tracing was successful, or
+  ///   an \a llvm::Error otherwise.
+  static llvm::Expected

Shouldn't this structure be general and have no notion of a tid since it could 
represent the buffer of a single thread or the buffer of a single CPU?
The way I see it, this structure simply wraps the buffer of a specific 
perf_event but has no notion of if it's for a specific tid or cpu.
Then you could have two subclasses, one for thread one for cpu, that inherit 
from this and have the additional context about the buffer. The inheritance may 
be overkill, but point I'm trying to make is that this structure should be 
agnostic to what "unit's" (thread or cpu) trace data it contains


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124648

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

Much cleaner now, thanks for separating out the procfs logic 🙂


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124573

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124648: [trace][intelpt] Support system-wide tracing [3] - Refactor IntelPTThreadTrace

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

I'm assuming the changes to make the single buffer trace class generic for 
threads and cpu buffers is done in the next diff so stamping this, feel free to 
update with those changes if that is not the case




Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:41-48
+  /// \param[in] tid
+  /// The tid of the thread to be traced.
+  ///
+  /// \return
+  ///   A \a IntelPTSingleBufferTrace instance if tracing was successful, or
+  ///   an \a llvm::Error otherwise.
+  static llvm::Expected

wallace wrote:
> jj10306 wrote:
> > Shouldn't this structure be general and have no notion of a tid since it 
> > could represent the buffer of a single thread or the buffer of a single CPU?
> > The way I see it, this structure simply wraps the buffer of a specific 
> > perf_event but has no notion of if it's for a specific tid or cpu.
> > Then you could have two subclasses, one for thread one for cpu, that 
> > inherit from this and have the additional context about the buffer. The 
> > inheritance may be overkill, but point I'm trying to make is that this 
> > structure should be agnostic to what "unit's" (thread or cpu) trace data it 
> > contains
> what I was thinking is to change this in a later diff to be like 
>   Start(const TraceIntelPTStartRequest &request, Optional tid, 
> Optional core_id);
> 
> which seems to me generic enough for all possible use cases. LLDB couldn't 
> support cgroups, so we are limited to tids and core_ids. With this, it seems 
> to me that having two child classes it's a little bit too much because the 
> only difference are just two lines of code where we specify the perf_event 
> configuration for tid and core_id.
> 
> But if you insist in the clarify of the subclasses, I for sure can do it that 
> way.
What you suggested makes sense, do you plan make this change in this diff or is 
this in one of the future ones?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124648

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/docs/lldb-gdb-remote.txt:372-374
+// This limit applies to the sum of the sizes of all trace and core
+// buffers for the current process, excluding the ones started with
+// "thread tracing".

Not sure what is trying to be communicated hear, so maybe my suggestion doesn't 
make sense, but I was confused by the wording here



Comment at: lldb/docs/lldb-gdb-remote.txt:465-474
 //  "tid": ,
 //  "binaryData": [
 //{
 //  "kind": ,
 //  Identifier for some binary data related to this thread to
 //  fetch with the jLLDBTraceGetBinaryData packet.
 //  "size": ,

Should the thread info be optional now that we have an optional `cores`? If 
not, can you explain how this output works in the case that you are doing per 
core tracing? Will both the tid binary data and the cores binary data section 
be populated?



Comment at: lldb/include/lldb/Utility/TraceGDBRemotePackets.h:157-160
+  std::vector traced_threads;
+  std::vector process_binary_data;
+  llvm::Optional> cores;
 };

Similar question here as above on the GetState documentation - how do the cores 
and traced_threads options play together?



Comment at: lldb/include/lldb/lldb-types.h:92
 typedef uint64_t queue_id_t;
+typedef int core_id_t; // CPU core id
 } // namespace lldb

nit: this should be an unsigned int of some sort?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:228-234
+  return m_per_thread_process_trace_up->TraceStart(tid);
 }
 
 Error IntelPTCollector::OnThreadDestroyed(lldb::tid_t tid) {
-  if (IsProcessTracingEnabled() && m_process_trace->TracesThread(tid))
-return m_process_trace->TraceStop(tid);
+  if (IsProcessTracingEnabled() &&
+  m_per_thread_process_trace_up->TracesThread(tid))
+return m_per_thread_process_trace_up->TraceStop(tid);

Do we not need to check if `m_per_thread_process_trace_up` is null here (aka 
per core tracing is enabled)?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:260
+  }
+  if (m_per_core_process_trace_up) {
+state.cores.emplace();

Should this be an "else if" if these two are mutually exclusive?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:63-64
+
+/// Manages a "process trace" instance by tracing each thread individually.
+class IntelPTPerThreadProcessTrace {
 public:

Why is this named "PerThread" if it handles both the per thread and per core 
cases for "process trace"? Perhaps I'm missing something



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:152-156
+  IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up;
+  /// Cores traced due to per-core "process tracing". Only one active
+  /// "process tracing" instance is allowed for a single process.
+  /// This might be \b nullptr.
+  IntelPTMultiCoreTraceUP m_per_core_process_trace_up;

So these are mutually exclusive tight?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:20-22
+  int64_t required = cores.size() * request.trace_buffer_size;
+  int64_t limit = request.process_buffer_size_limit.getValueOr(
+  std::numeric_limits::max());

nit: why are we using signed integers here?



Comment at: lldb/source/Plugins/Process/Linux/Perf.h:111-115
   /// \param[in] pid
-  /// The process to be monitored by the event.
+  /// The process or thread to be monitored by the event.
   ///
   /// \param[in] cpu
   /// The cpu to be monitored by the event.

Maybe add explanation of what is passed to the syscall for pid and cpu when 
they are `None` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124858

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:228-234
+  return m_per_thread_process_trace_up->TraceStart(tid);
 }
 
 Error IntelPTCollector::OnThreadDestroyed(lldb::tid_t tid) {
-  if (IsProcessTracingEnabled() && m_process_trace->TracesThread(tid))
-return m_process_trace->TraceStop(tid);
+  if (IsProcessTracingEnabled() &&
+  m_per_thread_process_trace_up->TracesThread(tid))
+return m_per_thread_process_trace_up->TraceStop(tid);

wallace wrote:
> jj10306 wrote:
> > Do we not need to check if `m_per_thread_process_trace_up` is null here 
> > (aka per core tracing is enabled)?
> I need to check that m_per_thread_process_trace_up exists
Yes, I meant "is *not* null here", my bad (-:



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:63-64
+
+/// Manages a "process trace" instance by tracing each thread individually.
+class IntelPTPerThreadProcessTrace {
 public:

wallace wrote:
> jj10306 wrote:
> > Why is this named "PerThread" if it handles both the per thread and per 
> > core cases for "process trace"? Perhaps I'm missing something
> this doesn't handle the per core case. This is handling exclusively the case 
> of "process trace" without the "per-cpu" option. This class effectively 
> creates a perf event per thread. Even its Start method asks for tids
ahhh yes I got messed up by the diff view and thought that the fields of 
IntelPTCollector were being stored on this per thread structure, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124858

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124962: [trace][intelpt] Support system-wide tracing [5] - Disable/enable per-core tracing based on the process state

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:234
+IntelPTSingleBufferTrace::GetTraceBuffer(size_t offset, size_t size,
+ bool flush) {
   std::vector data(size, 0);

What's the purpose of this new `flush` flag? When would you want to call this 
method with it set to false? I can only think of cases when you want to flush 
the buffer if you're trying to read its data



Comment at: lldb/source/Plugins/Process/Linux/Perf.h:216-222
+  llvm::Error DisableWithIoctl() const;
+
+  /// Use the ioctl API to enable the perf event.
+  ///
+  /// \return
+  ///   An Error if the perf event couldn't be enabled.
+  llvm::Error EnableWithIoctl() const;

Nice, happy to see we are extending the mini perf API (:
In the future we also can update this to control whether or not the event is 
enabled at perf_event_open time because that's the default but there may be 
cases where you don't want the event to be enabled until you explicitly enable 
it w ioctl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124962

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

couple minor things, but looks good overall




Comment at: lldb/docs/lldb-gdb-remote.txt:465-474
 //  "tid": ,
 //  "binaryData": [
 //{
 //  "kind": ,
 //  Identifier for some binary data related to this thread to
 //  fetch with the jLLDBTraceGetBinaryData packet.
 //  "size": ,

wallace wrote:
> wallace wrote:
> > jj10306 wrote:
> > > Should the thread info be optional now that we have an optional `cores`? 
> > > If not, can you explain how this output works in the case that you are 
> > > doing per core tracing? Will both the tid binary data and the cores 
> > > binary data section be populated?
> > I'll make this field optional and include more documentation below
> in the end i didn't make this optional but instead forced the per-core case 
> to return all threads 
so the tid field of a tracedThread object will be populated but the binaryData 
will not be populated in the tracedThread object but instead in the cores 
section?



Comment at: lldb/source/Plugins/Process/Linux/Perf.h:119
   ///
   /// \param[in] group_fd
+  /// File descriptor of the group leader. \b -1 indicated that this

nit: we could make this optional as well and just default to passing in -1 to 
be consistent with cpu and pid



Comment at: lldb/source/Utility/TraceGDBRemotePackets.cpp:124
+json::Value toJSON(const TraceCoreState &packet) {
+  return json::Value(Object{{"coreId", static_cast(packet.core_id)},
+{"binaryData", packet.binary_data}});

isn't uint64_t serialization and deserialization now supported?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124858

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:152-156
+  IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up;
+  /// Cores traced due to per-core "process tracing". Only one active
+  /// "process tracing" instance is allowed for a single process.
+  /// This might be \b nullptr.
+  IntelPTMultiCoreTraceUP m_per_core_process_trace_up;

wallace wrote:
> jj10306 wrote:
> > So these are mutually exclusive tight?
> yes, I'll leave a comment about that here
this could be overkill, but potentially we could use a union here to enforce 
this invariant and a boolean flag/enum to determine which process tracing 
"handler" is being used
```
bool is_per_core_process_tracing_enabled; // used to determine how to interpret 
the union
union {
  IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up;
  IntelPTMultiCoreTraceUP m_per_core_process_trace_up;

};
```
imo this is much more clearly expresses the invariant of only one of the 
process tracing "handles" being non null at a time.
wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124858

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D125047: [trace][intelpt] Support system-wide tracing [6] - Break IntelPTCollector into smaller files and minor refactor

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Utility/TraceGDBRemotePackets.cpp:136
 {"tid", packet.tid},
-{"size", packet.size}});
+{"size", static_cast(packet.size)}});
 }

why is this needed now? I thought that unsigned ints were now supported by the 
json lib. 
is this temporary because of the bug you address in D125322?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125047

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124962: [trace][intelpt] Support system-wide tracing [5] - Disable/enable per-core tracing based on the process state

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:211
 
+Error IntelPTSingleBufferTrace::SetCollectionState(
+TraceCollectionState new_state) {

nit: from the "Set" name, my first impression was this method was simply 
setting `m_collection_state`, but in reality it's doing some nontrivial 
operations, namely changing the state of the perf event via ioctl. Consider 
changing the name from "Set" to avoid giving the impression of a trivial 
operation



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:309
 return attr.takeError();
+  attr->disabled = initial_state == TraceCollectionState::Paused;
 

nice


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124962

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D125503: [trace][intelpt] Support system-wide tracing [7] - Create a base IntelPTProcessTrace class

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:73
+  /// The target process.
   NativeProcessProtocol &m_process;
   /// Threads traced due to "thread tracing"

Could this be moved to be a part of the new `IntelPTProcessTrace` abstract 
class or is this also needed for handling `thread trace`?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:36
 
-Expected IntelPTMultiCoreTrace::StartOnAllCores(
-const TraceIntelPTStartRequest &request) {
+Expected
+IntelPTMultiCoreTrace::StartOnAllCores(const TraceIntelPTStartRequest &request,

If this now returns `IntelPTProcessTraceUP` instead of an instance of itself, 
then we are basically forcing any uses of this class to be done using dynamic 
polymorphism (behind the indirection of a pointer to the super class). Should 
this instead return an instance of itself and then leave it up to the caller to 
decide if they want to use the object directly or behind a pointer?
I know that currently the only use of this is behind the 
`IntelPTProcessTraceUP` (stored on the collector), but maybe in the future we 
would want to use it directly.
wdyt?




Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:119-125
+  // All the process' threads are being traced automatically.
+  return (bool)m_process.GetThreadByID(tid);
+}
+
+llvm::Error IntelPTMultiCoreTrace::TraceStart(lldb::tid_t tid) {
+  // This instance is already tracing all threads automatically.
+  return llvm::Error::success();

In `TracesThread` you check that the tid is a thread of `m_process` but in 
`TraceStart` you return success for all threads without checking if it's a part 
of the process.
I don't think it particularly matters if we do the check or not, but I do think 
that the behavior should be consistent between these functions.



Comment at: 
lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp:58
+
+Expected
+IntelPTPerThreadProcessTrace::Start(const TraceIntelPTStartRequest &request,

same comment here as above on the other subclasss



Comment at: lldb/source/Plugins/Process/Linux/IntelPTProcessTrace.h:19
+
+// Abstract class to be inherited by all the process tracing strategies.
+class IntelPTProcessTrace {




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125503

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D125850: [trace][intelpt] Support system-wide tracing [8] - Improve the single buffer perf_event configuration

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:186
   attr.exclude_kernel = 1;
-  attr.sample_type = PERF_SAMPLE_TIME;
-  attr.sample_id_all = 1;

won't we need this in order to get timestamps in the context switching events? 
I agree we don't need it for the time being so maybe in the diff where you add 
context switch collection you will reintroduce it 🙂



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:316
-if (Error mmap_err = perf_event->MmapMetadataAndBuffers(buffer_numpages,
-buffer_numpages)) {
   return std::move(mmap_err);

In the future if we need to collect itrace we will need a small data buffer.
Do you plan to collect context switch info as part of the same perf event used 
for intel pt or will you open a separate event?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125850

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D125897: [trace][intelpt] Support system-wide tracing [9] - Collect and return context switch traces

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

Submitting my comments so far, will continue my review in a bit




Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:26-28
   static const char *kProcFsCpuInfo;
   static const char *kTraceBuffer;
+  static const char *kPerfContextSwitchTrace;

nit: these are just pointers to a buffer of bytes right? if so, maybe we could 
change the types to be `const uint8_t *`.
Obviously it doesn't make a difference since they are the same size, but when I 
see `const char *` my brain kinda immediately thinks STRING!
maybe it's just me though 🤩,  wdyt?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:41-45
+  void ProcessDidStop();
+
+  /// To be invoked before the process will resume, so that we can capture the
+  /// first instructions after the resume.
+  void ProcessWillResume();

Why do we need to separate these out into two separate functions?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:36
 
+static Expected CreateContextSwitchTracePerfEvent(
+bool disabled, lldb::core_id_t core_id,

thoughts on moving this to be a "utility" method of the Perf.cpp and then call 
that utility method from here? Moving it there would make it more accessible to 
other callers.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:47
+  attr.size = sizeof(attr);
+  attr.sample_period = 0;
+  attr.sample_type = PERF_SAMPLE_TID | PERF_SAMPLE_TIME;

what does this do?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:51-54
+  attr.exclude_kernel = 1;
+  attr.sample_id_all = 1;
+  attr.exclude_hv = 1;
+  attr.disabled = disabled;

In addition to this context switching event needing to be part of the same 
group, you also must ensure that some other config bits (exclude_kernel, 
exclude_hv) are the same.
Also, what would happen if the disabled bit is set here but then the enabled 
bit of the intel pt event was set?

All of these considerations related to keeping the two events "in sync", are 
beginning to make me lean towards what I mentioned above about using the same 
perf event, because that would naturally remove any opportunities for the two 
events to be "out of sync"



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:56
+
+  // The given perf configuration will product context switch records of 32
+  // bytes each. Assuming that every context switch will be emitted twice (one





Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:101-109
+Expected core_trace =
+IntelPTSingleBufferTrace::Start(request, /*tid=*/None, core_id,
+/*disabled=*/true);
+if (!core_trace)
   return IncludePerfEventParanoidMessageInError(core_trace.takeError());
+
+if (Expected context_switch_trace =

to reduce opportunity for things to get out of sync if this is ever touched in 
the future?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.h:97
+  llvm::DenseMap>
+  m_traces_per_core;

perhaps you could make this it's own structure? that would make things a bit 
less verbose here and in the ForEachCore callback?
also, this may make it clearer that they must be part of the same perf event 
and potentially would allow enforcing this easier?
Instead of creating an entirely new structure you also could make the 
ContextSwitchTrace an optional member of IntelPTSingleBuffer trace, but not 
sure if that's the best place to do the coupling.
Another option would be to just use the same underlying perf_event of 
IntelPTSingleBufferTrace. That would naturally enforce the constraint of the 
events being part of the same group and wouldn't require adding this new 
ContextSwitchTrace notion.

lots of options lol, I think things are fine as is but wanted to share my 
thoughts.
wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125897

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D125897: [trace][intelpt] Support system-wide tracing [9] - Collect and return context switch traces

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:122-129
 void IntelPTMultiCoreTrace::ForEachCore(
 std::function
 callback) {
   for (auto &it : m_traces_per_core)
-callback(it.first, *it.second);
+callback(it.first, it.second.first);
 }
 

Is there a way to consolidate these two methods?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:43
+  /// \param[in] disabled
+  /// Whether to start the tracing paused.
   ///

This wording is a bit confusing - maybe provide some additional context here 
about this flag controlling the whether the perf event is enabled/disabled at 
perf_event_open time



Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:161
   if (Expected mmap_metadata_data =
-  DoMmap(nullptr, mmap_size, PROT_WRITE, MAP_SHARED, 0,
+  DoMmap(nullptr, mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED, 0,
  "metadata and data buffer")) {

curious what adding the PROT_READ does here, given that this code was working 
with just PROT_WRITE it seems like PROT_WRITE implies PROT_READ



Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:226
+Expected>
+PerfEvent::ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size) {
+  CollectionState previous_state = m_collection_state;

It's worth noting that this function should only be called when the aux buffer 
is configured as a ring buffer (ie the aux buffer was mmaped with PROT_READ). 
if the aux buffer mmaped with PROT_WRITE then the behavior of the head is 
different and the consumer is required to update the aux_tail when reading.
Perhaps we should check this before doing the reading or make it very clear in 
the docs that this should only be called on perf events with that specific 
configuration



Comment at: lldb/source/Plugins/Process/Linux/Perf.h:101
 class PerfEvent {
+  enum class CollectionState {
+Enabled,

is an enum needed for this? Since the perf event will only ever be in two 
states, feels like a bool would suffice



Comment at: lldb/source/Plugins/Process/Linux/Perf.h:238
+  llvm::Expected>
+  ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size);
+

nit: This is wordy. imo the name needs not mention that it's flushed out, just 
put that in the docs



Comment at: 
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py:226-229
+self.assertTrue(context_switch_size is not None)
+self.assertTrue(trace_buffer_size is not None)
+if context_switch_size > 0:
+found_non_empty_context_switch = True

nice test!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125897

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126267: [trace][intelpt] Support system-wide tracing [13] - Add context switch decoding

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:22-42
+struct PerfContextSwitchRecord {
+  struct perf_event_header header;
+  uint32_t next_prev_pid;
+  uint32_t next_prev_tid;
+  uint32_t pid, tid;
+  uint64_t time_in_nanos;
+

should these structures live in `Perf.h`?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:59-102
+ThreadContinuousExecution ThreadContinuousExecution::CreateCompleteExecution(
+lldb::core_id_t core_id, lldb::tid_t tid, uint64_t start, uint64_t end) {
+  ThreadContinuousExecution o(core_id, tid);
+  o.variant = Variant::Complete;
+  o.tscs.complete.start = start;
+  o.tscs.complete.end = end;
+  return o;

nit: if you switch the union for two optional values you can remove a lot of 
the redundancy between these methods.



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:136-141
+  if (record.IsIn() && prev.IsIn()) {
+// We found two consecutive ins, which means that we didn't capture
+// the end of the previous execution.
+
on_new_thread_execution(ThreadContinuousExecution::CreateHintedEndExecution(
+core_id, prev.tid, prev.tsc, record.tsc - 1));
+  } else if (record.IsOut() && prev.IsOut()) {

can you help me understand how these two cases could happen? they seem 
fundamentally impossible given the nature of context switching - shouldn't all 
"continuous" execution be disjoint?

My current understanding is as follows:

Expected:
i1 o1 i4 o4 i9 o9

Impossible:
and this is not possible:
i1 i4 o4 o1  9 o9

Let me know if I'm missing something 🙂




Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:146
+ThreadContinuousExecution::CreateHintedStartExecution(
+core_id, record.tid, prev.tsc + 1, record.tsc));
+  } else if (record.IsOut() && prev.IsIn()) {

why the + 1?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:181
+///   the trace.
+static Error DecodePerfContextSwitchTrace(
+ArrayRef data, core_id_t core_id,

Do you think any of the general perf logic related to "decoding" the records 
should be moved to `Perf.h/cpp`?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:196
+const PerfContextSwitchRecord &perf_record =
+*reinterpret_cast(data.data() +
+   offset);

nit: it feels weird casting to `PerfContextSwitchRecord` when we don't yet know 
if this is actually a context switch event without first looking at the header.
casting to `perf_event_header` and checking that first before interpreting the 
record as a context switch record seems like a better approach.
Given that currently only intelpt is using the LLDB's perf "library" this isn't 
a big deal, but if we wanted to make it more complete/robust, we should revisit 
this and improve our record handling design so it could be easily extended to 
support any record types.



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:198
+   offset);
+// A record of 1000 uint64_t's or more should mean that the data is wrong
+if (perf_record.header.size == 0 ||

can you link the documentation that states this?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:199-200
+// A record of 1000 uint64_t's or more should mean that the data is wrong
+if (perf_record.header.size == 0 ||
+perf_record.header.size > sizeof(uint64_t) * 1000)
+  return CreateError(offset, formatv("A record of {0} bytes was found.",

using sizeof on uint64_t feels weird since the typename already implies the 
name. I think moving this value to a constant and explaining its value would 
make things cleaner.



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:204-205
+
+// We add + 100 to this record because some systems might have custom
+// records. In any case, we are looking only for abnormal data.
+if (perf_record.header.type >= PERF_RECORD_MAX + 100)

same as above, can we link the docs. alternatively, link the docs at the top of 
the function or in the header and then reference that link at the appropriate 
spots in the code



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:263-272
+  auto do_decode = [&]() -> Error {
+// We'll decode all context switch traces, identify continuous executions
+// and grou

[Lldb-commits] [PATCH] D125932: [trace][intelpt] Support system-wide tracing [10] - Return warnings and tsc information from lldb-server.

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:40
+IntelPTCollector::FetchPerfTscConversionParameters() {
+  if (!m_cached_tsc_conversion) {
+if (Expected tsc_conversion =

Don't we want to always reload the parameters here (ie not do this check) since 
they could potentially change  or is that not the case?
We had discussions about this when I initially added the tsc conversion logic 
but I can't remember so maybe you can help refresh my memory:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125932

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D125943: [trace][intelpt] Support system-wide tracing [11] - Read warnings and perf conversion in the client

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jsji.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125943

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

feedback-v1




Comment at: lldb/include/lldb/Target/Trace.h:520
+  /// core id -> data kind -> size
+  llvm::DenseMap>
+  m_live_core_data;

Would this work instead? I noticed that the several other maps around this code 
also use unordered_map and string, maybe we could update those too at some 
point.



Comment at: lldb/include/lldb/Target/Trace.h:538
+  /// core id -> data kind -> file
+  llvm::DenseMap>
+  m_postmortem_core_data;

same as comment above related to using LLVM types rather than std types 
wherever possible.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:193
 
-Expected>
-IntelPTMultiCoreTrace::GetBinaryData(const TraceGetBinaryDataRequest &request) 
{
-  return createStringError(inconvertibleErrorCode(), "Unimplemented");
+Expected>>
+IntelPTMultiCoreTrace::TryGetBinaryData(

`Expected>` feels weird. Are both of these "layers" needed?
I noticed this in a couple places but I'm just leaving a single comment here.



Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:191
+   * with respect to their definition of head pointer. In the case
+   * of Aux data buffer the head always wraps around the aux buffer
+   * and we don't need to care about it, whereas the data_head keeps

Be careful with the "strong" language here. The AUX head only wraps around when 
the AUX buffer is mmaped with read-only, if it is mmapped with write perms, 
then the AUX head behaves like the data head (it doesn't auto wrap)

See this snipped of kernel code, the overwrite flag is set based on the mmap 
PROT and that overwrite flag is passed to the intelpt code.
https://github.com/torvalds/linux/blob/df0cc57e/kernel/events/ring_buffer.c#L670-L733


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126015

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

feedback-v2




Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:183
+Expected>
+PerfEvent::ReadFlushedOutDataCyclicBuffer(size_t offset, size_t size) {
+  CollectionState previous_state = m_collection_state;

Do we need the offset/size? These arguments make the reading logic much more 
involved versus reading the whole buffer.
I understand that this is a more "general" method, but iiuc currently we only 
read the entire buffer. I guess in the future somebody could want the 
flexibility of providing an offset/size, but I don't see the need currently.
wdyt?
Same comment applies to the AUX buffer read method



Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:206-212
+for (uint64_t i = actual_data_head + offset;
+ i < data_size && output.size() < size; i++)
+  output.push_back(data[i]);
+
+// We need to find the starting position for the left part if the offset 
was
+// too big
+uint64_t left_part_start = actual_data_head + offset >= data_size

What happens if an offset larger then the size of the buffer is provided? 
should that offset be wrapped or not?
What happens if a size larger than the size of the buffer is provided? should 
we only copy up to buffer size bytes or should we do multiple copies of the 
buffer to satisfy the request?

As I mentioned above, imo the ambiguity/complexity these arguments introduce is 
not worth the value add (which currently is none since we are always copying 
the whole buffer).

If we do stick with these args, we should make it very clear what happens in 
the cases I mentioned above and/or put restrictions on the ranges of these 
values (ie they must always be less than the buffer size)



Comment at: lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h:47
   ///   The trace file of this thread.
-  const FileSpec &GetTraceFile() const;
+  const llvm::Optional &GetTraceFile() const;
 

why is this now optional?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:169
+  ///
+  /// \param[in] traces_proceses
+  /// The processes traced in the live session.





Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:176-177
+  TraceIntelPT(JSONTraceSession &session,
+   llvm::ArrayRef traced_processes,
+   llvm::ArrayRef traced_threads);
 

do we need the threads explicitly or does `traced_processes` implicitly contain 
this info?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:53
+  return json::Object{{"tid", thread.tid},
+  {"traceBuffer", thread.trace_buffer}};
+}

if this is none, does this still create a key value pair?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:88-90
+  if (!o || !o.map("coreId", core_id) ||
+  !o.map("traceBuffer", core.trace_buffer) ||
+  !o.map("contextSwitchTrace", core.context_switch_trace))

nit: use what De Morgan taught us (;
Having to add a `!` for each new addition to the predicate is very error prone 
and would be a pain to debug if you accidentally forgot to add the negation.



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:109-110
+  uint64_t family, model, stepping;
+  if (!o || !o.map("vendor", vendor) || !o.map("family", family) ||
+  !o.map("model", model) || !o.map("stepping", stepping))
+return false;

same comment as above related to De Morgan's law



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:131-133
+  if (!o || !o.map("processes", session.processes) ||
+  !o.map("type", session.type) || !o.map("cores", session.cores) ||
+  !o.map("tscPerfZeroConversion", session.tsc_perf_zero_conversion))

same comment as above related to De Morgan's law



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:32-40
+struct JSONThread {
+  int64_t tid;
+  llvm::Optional trace_buffer;
 };
 
-struct JSONTraceIntelPTTrace {
-  std::string type;
-  JSONTraceIntelPTCPUInfo cpuInfo;
+struct JSONProcess {
+  int64_t pid;

If the intention here is for system wide tracing to not provide threads while 
single thread tracing does then should we change this like so?
(i know you're currently working on the thread detection so potentially this 
will be addressed then?)



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:31
+
+Error TraceIntelPTSessionFileParser::ParseModule(lldb::TargetSP &target_sp,
+ const JSONModule &module) {

take a reference to the underlying object directly?



Comment at: 
lldb/source/Plugin

[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

feeback-v3 - completed review




Comment at: lldb/docs/use/intel_pt.rst:168
 ::
   {
+"type": "intel-pt",

Consider adding a section on the perfTscConversion parameters while we're 
editing this file as I don't currently see that in this file. Also, it doesn't 
appear the "cores" section is here either



Comment at: lldb/source/Target/Trace.cpp:30-32
 struct JSONSimpleTraceSession {
-  JSONSimplePluginSettings trace;
+  std::string type;
 };

do we need this struct or can we just use a string directly since the struct 
only has a single member?



Comment at: lldb/source/Target/Trace.cpp:134
+return None;
+  std::unordered_map &single_core_data = it->second;
+  auto single_thread_data_it = single_core_data.find(kind.str());





Comment at: lldb/source/Target/Trace.cpp:135
+  std::unordered_map &single_core_data = it->second;
+  auto single_thread_data_it = single_core_data.find(kind.str());
+  if (single_thread_data_it == single_core_data.end())

why is this named thread_data, isn't this the size of the core's buffer?



Comment at: lldb/source/Target/Trace.cpp:325
+
+  std::unordered_map &data_kind_to_file_spec_map =
+  it->second;





Comment at: lldb/unittests/Process/Linux/PerfTests.cpp:9
 
-#ifdef __x86_64__
-

isn't this needed since `readTsc()` uses intel specific assembly instructions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126015

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits