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

[Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG05b4bf257124: [trace][intelpt] Introduce instruction Ids 
(authored by Walter Erquinigo ).

Changed prior to commit:
  https://reviews.llvm.org/D122254?vs=418680&id=420969#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTSC.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTSC.py
===
--- lldb/test/API/commands/trace/TestTraceTSC.py
+++ lldb/test/API/commands/trace/TestTraceTSC.py
@@ -19,7 +19,35 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+
+@testSBAPIAndCommands
+@skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
+def testMultipleTscsPerThread(self):
+self.expect("file " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+self.expect("b main")
+self.expect("r")
+
+self.traceStartThread(enableTsc=True)
+
+# After each stop there'll be a new TSC
+self.expect("n")
+self.expect("n")
+self.expect("n")
+
+# We'll get the most recent instructions, with at least 3 different TSCs
+self.runCmd("thread trace dump instructions --tsc --raw")
+id_to_tsc = {}
+for line in self.res.GetOutput().splitlines():
+m = re.search("(.+): \[tsc=(.+)\].*", line)
+if m:
+id_to_tsc[int(m.group(1))] = m.group(2)
+self.assertEqual(len(id_to_tsc), 6)
+
+# We check that the values are right when dumping a specific id
+for id in range(0, 6):
+self.expect(f"thread trace dump instructions --tsc --id {id} -c 1",
+substrs=[f"{id}: [tsc={id_to_tsc[id]}]"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +60,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +73,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
 self.expect("thread trace dump instructi

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

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 418680.
wallace added a comment.

- Bring back the --continue command.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 32 at main.cpp:4
-\[  0\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
-\[ -1\] {ADDRESS_REGEX}cmpl .*
-\[ -2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ -3\] {ADDRESS_REGEX}movl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
+3: {ADDRESS_REGEX}cmpl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+1: {ADDRESS_REGEX}movl .*
   a.out`main \+ 4 at main.cpp:2
-\[ -4\] {ADDRESS_REGEX}movl .* '''])
+0: {ADDRESS_REGEX}movl .* '''])
 
 # We stop tracing
 self.expect("thread trace stop")
@@ -152,12 +152,12 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[ 0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[  0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("c")
 # Now the process has finished, so the commands should fail
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInstructions.py
+++ lldb/test/API/commands/trace/TestTraceDumpInstructions.py
@@ -37,49 +37,67 @@
 
 

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2201
+// command
+if (cmd.find(" repeat") == std::string::npos)
+  cmd += " repeat";

This " repeat" is pretty hacky here. If we can't get away without adding 
anything, I would prefer the --continue option we had before



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2254-2256
+  m_options.m_dumper_options.skip = 1;
+  m_options.m_dumper_options.id = m_last_id;
 }

to make repeat work, you are filling in the dumper options, can't we just store 
a copy of the old thread options and update the "id" field?

You also mentioned:



> I couldn't find a simple way to create the repeat command directly from 
> GetRepeatCommand because it's not easy to quickly find what the next 
> instruction is going to be in the repeat command. In fact, finding the id of 
> that future instruction requires to actually traverse the instructions up to 
> that point, but the traversal only happens in DoExecute, which is invoked 
> after GetRepeatCommand. As a simple workarou nd, I'm adding a " repeat" 
> positional argument that won't be parsed by CommandOptions but will still 
> indicate that a repeat is happening. I was able to remove the --continue flag 
> from Options.td, thus reducing the amount of code needed to handle the 
> repeat. Not only that, I was able to get rid of the map TraceInstructionDumper> that I had in the CommandObject that I was using to 
> continue the iteration in the repeat commands. Now I'm using the last 
> iterated id to computed where the next one will be, thus creating a new brand 
> new dumper with each command making this class simpler.

You seem to have the right "id" to start at here with m_last_id? Why can't we 
just store this in an extra copy of TraceInstructionDumperOptions and have it 
create the right command?


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

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 418361.
wallace marked 2 inline comments as done.
wallace added a comment.

Some updates:

- Modified `thread trace dump instructions` to accept one single thread instead 
of many. The reason is that, with the new --id argument, traversing multiple 
threads doesn't make sense anymore, as the id might only exist in one single 
thread. Besides that, it's really not very useful to analyze multiple threads 
in parallel by chunks. You normally want to analyze one of them at a time.
- I couldn't find a simple way to create the repeat command directly from 
`GetRepeatCommand` because it's not easy to quickly find what the next 
instruction is going to be in the repeat command. In fact, finding the id of 
that future instruction requires to actually traverse the instructions up to 
that point, but the traversal only happens in DoExecute, which is invoked after 
GetRepeatCommand. As a simple workarou nd, I'm adding a " repeat" positional 
argument that won't be parsed by CommandOptions but will still indicate that a 
repeat is happening. I was able to remove the --continue flag from Options.td, 
thus reducing the amount of code needed to handle the repeat. Not only that, I 
was able to get rid of the map that I had in the 
CommandObject that I was using to continue the iteration in the repeat 
commands. Now I'm using the last iterated id to computed where the next one 
will be, thus creating a new brand new dumper with each command making this 
class simpler.
- I can't pass the Stream to TraceInstructionDumperOptions because when that 
object is crea ted the Stream is not yet available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .

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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:50
+  /// Additional options for configuring the dumping.
+  TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up, Stream &s,
+ const TraceInstructionDumperOptions &options);

clayborg wrote:
> Do we want the stream in the options?
we can't do it nicely, because we are adding a TraceInstructionDumper variable 
in CommandObjectTraceDumpInstructions::CommandOptions, where we don't have a 
stream available. I imagine this being the only variable that we can't put in 
the options, so it should be okay


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

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 418032.
wallace added a comment.

fix some comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 32 at main.cpp:4
-\[  0\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
-\[ -1\] {ADDRESS_REGEX}cmpl .*
-\[ -2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ -3\] {ADDRESS_REGEX}movl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
+3: {ADDRESS_REGEX}cmpl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+1: {ADDRESS_REGEX}movl .*
   a.out`main \+ 4 at main.cpp:2
-\[ -4\] {ADDRESS_REGEX}movl .* '''])
+0: {ADDRESS_REGEX}movl .* '''])
 
 # We stop tracing
 self.expect("thread trace stop")
@@ -152,12 +152,12 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[ 0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[  0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("c")
 # Now the process has finished, so the commands should fail
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInstructions.py
+++ lldb/test/API/commands/trace/TestTraceDumpInstruct

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/Target/TraceCursor.h:162-166
+  /// - In terms of efficiency, moving the cursor to a given id should be as
+  /// fast
+  ///   as possible, but not necessarily O(1). That's why the recommended way 
to
+  ///   traverse sequential instructions is to use the \a TraceCursor::Next()
+  ///   method and only use \a TraceCursor::GoToId(id) sparingly.

fix wrap here. Might have been a clang format change



Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:50
+  /// Additional options for configuring the dumping.
+  TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up, Stream &s,
+ const TraceInstructionDumperOptions &options);

Do we want the stream in the options?



Comment at: lldb/source/Commands/Options.td:1126-1128
+Desc<"Continue dumping instructions right where the previous invocation of 
"
+"this command was left, or from the beginning if this is the first "
+"invocation. The --skip and --id arguments are discared if provided.">;

this option might not be needed. Each LLDB command line command can save 
information for how to continue a subsequent command. For example:

```(lldb) memory read $pc
0x12ecf: 48 8d 7d f0 48 8d 35 a6 ff ff ff e8 51 00 00 00  H.}.H.5.Q...
0x12edf: c7 45 ec 01 00 00 00 8b 75 ec 48 8d 3d 1e 10 00  .E..u.H.=...
(lldb) 
0x12eef: 00 31 c0 e8 f5 0e 00 00 e9 00 00 00 00 e9 00 00  .1..
0x12eff: 00 00 8b 45 ec 83 c0 01 89 45 ec e9 d7 ff ff ff  ...E.E..
(lldb) 
0x12f0f: 48 89 c1 89 d0 48 89 4d e0 89 45 dc 48 8d 7d f0  HH.M..E.H.}.
0x12f1f: e8 aa 0e 00 00 48 8b 7d e0 e8 7d 0e 00 00 0f 1f  .H.}..}.
```
Note that I just hit enter after the first read memory. Can we just take 
advantage of this feature instead of addind the "--continue" option?



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:28
+if (!m_cursor_up->GoToId(*m_options.id)) {
+  s.Printf("invalid instruction id\n");
+  SetNoMoreData();

When there is no formatter, you can just use Stream::PutCString(...)


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

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 417828.
wallace added a comment.

nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 32 at main.cpp:4
-\[  0\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
-\[ -1\] {ADDRESS_REGEX}cmpl .*
-\[ -2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ -3\] {ADDRESS_REGEX}movl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
+3: {ADDRESS_REGEX}cmpl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+1: {ADDRESS_REGEX}movl .*
   a.out`main \+ 4 at main.cpp:2
-\[ -4\] {ADDRESS_REGEX}movl .* '''])
+0: {ADDRESS_REGEX}movl .* '''])
 
 # We stop tracing
 self.expect("thread trace stop")
@@ -152,12 +152,12 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[ 0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[  0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("c")
 # Now the process has finished, so the commands should fail
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInstructions.py
+++ lldb/test/API/commands/trace/TestTraceDumpInstructions.py
@@ -3

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

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 417827.
wallace added a comment.

- improve documentation
- use lldb::user_id_t
- add the new TraceInstructionDumperOptions struct


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 32 at main.cpp:4
-\[  0\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
-\[ -1\] {ADDRESS_REGEX}cmpl .*
-\[ -2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ -3\] {ADDRESS_REGEX}movl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
+3: {ADDRESS_REGEX}cmpl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+1: {ADDRESS_REGEX}movl .*
   a.out`main \+ 4 at main.cpp:2
-\[ -4\] {ADDRESS_REGEX}movl .* '''])
+0: {ADDRESS_REGEX}movl .* '''])
 
 # We stop tracing
 self.expect("thread trace stop")
@@ -152,12 +152,12 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[ 0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[  0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("c")
 # Now the process has finished, so the commands should fail
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- lldb/test/API/commands/trace/Test

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

[Greg Clayton via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Target/TraceCursor.h:211-214
+  /// A unique identifier for the instruction or error this cursor is
+  /// pointing to. Each Trace plug-in can decide the nature of these
+  /// identifiers and thus no assumptions can be made regarding its 
ordering
+  /// and sequentiality.

might be nice to clarify this definition a bit with info like:
- does this need to be unique only within a single thread or does it need to be 
unique globally for any instruction in any thread within the process?
- a bit of info on why this is needed and what it will be used for as this will 
help people know how to create it so they can make sure it is efficient.



Comment at: lldb/include/lldb/Target/TraceCursor.h:215
+  /// and sequentiality.
+  virtual uint64_t GetId() const = 0;
   /// \}





Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:44-47
+  TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up, bool forwards,
+ bool raw, bool show_tsc, llvm::Optional id,
+ llvm::Optional skip, Stream &s);
 

Since we are adding new options, and if this is going to go in the public API 
at some point, it might be nice to make a "TraceInstructionDumperOptions class 
which default constructs with good default values. This way when you need to 
add a new option, it won't change the API



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2171-2177
   m_count = kDefaultCount;
-  m_skip = 0;
+  m_skip = llvm::None;
+  m_id = llvm::None;
   m_raw = false;
   m_forwards = false;
   m_show_tsc = false;
   m_continue = false;

use TraceInstructionDumperOptions?



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2187-2193
 size_t m_count;
-size_t m_skip;
+llvm::Optional m_skip;
+llvm::Optional m_id;
 bool m_raw;
 bool m_forwards;
 bool m_show_tsc;
 bool m_continue;

Use TraceInstructionDumperOptions?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:105
+
+bool TraceCursorIntelPT::GoToId(uint64_t id) {
+  if (m_decoded_thread_sp->GetInstructions().size() <= id)





Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:112
+
+uint64_t TraceCursorIntelPT::GetId() const { return m_pos; }





Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h:38
 
+  bool GoToId(uint64_t id) override;
+





Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h:40
+
+  uint64_t GetId() const override;
+




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] D122254: [trace][intelpt] Introduce instruction Ids

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 417389.
wallace marked 2 inline comments as done.
wallace added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/TraceExporter/common/TraceHTR.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 32 at main.cpp:4
-\[  0\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
-\[ -1\] {ADDRESS_REGEX}cmpl .*
-\[ -2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ -3\] {ADDRESS_REGEX}movl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
+3: {ADDRESS_REGEX}cmpl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+1: {ADDRESS_REGEX}movl .*
   a.out`main \+ 4 at main.cpp:2
-\[ -4\] {ADDRESS_REGEX}movl .* '''])
+0: {ADDRESS_REGEX}movl .* '''])
 
 # We stop tracing
 self.expect("thread trace stop")
@@ -152,12 +152,12 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[ 0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[  0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])
 
 self.expect("c")
 # Now the process has finished, so the commands should fail
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInstructions.py
+++ lldb/test

[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

[walter erquinigo via Phabricator via lldb-commits]

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

In order to support quick arbitrary access to instructions in the trace, we need
each instruction to have an id. It could be an index or any other value that the
trace plugin defines.

This will be useful for reverse debugging or for creating callstacks, as each
frame will need an instruction id associated with them.

I've updated the `thread trace dump instructions` command accordingly. It now
prints the instruction id instead of relative offset. I've also added a new --id
argument that allows starting the dump from an arbitrary position.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122254

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  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/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  lldb/test/API/commands/trace/TestTraceTimestampCounters.py

Index: lldb/test/API/commands/trace/TestTraceTimestampCounters.py
===
--- lldb/test/API/commands/trace/TestTraceTimestampCounters.py
+++ lldb/test/API/commands/trace/TestTraceTimestampCounters.py
@@ -19,7 +19,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -32,7 +32,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
+patterns=["0: \[tsc=0x[0-9a-fA-F]+\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -45,7 +45,7 @@
 
 self.expect("n")
 self.expect("thread trace dump instructions --tsc -c 1",
-patterns=["\[0\] \[tsc=unavailable\] 0x00400511movl"])
+patterns=["0: \[tsc=unavailable\] 0x00400511movl"])
 
 @testSBAPIAndCommands
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -114,29 +114,29 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl'''])
+0: {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[ 0\] {ADDRESS_REGEX}movl .*
+0: {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[ 1\] {ADDRESS_REGEX}movl .*
-\[ 2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ 3\] {ADDRESS_REGEX}cmpl .*
-\[ 4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
+1: {ADDRESS_REGEX}movl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+3: {ADDRESS_REGEX}cmpl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
 self.expect("thread trace dump instructions",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 32 at main.cpp:4
-\[  0\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
-\[ -1\] {ADDRESS_REGEX}cmpl .*
-\[ -2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-\[ -3\] {ADDRESS_REGEX}movl .*
+4: {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5
+3: {ADDRESS_REGEX}cmpl .*
+2: {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
+1: {ADDRESS_REGEX}movl .*
   a.out`main \+ 4 at main.cpp:2
-\[ -4\] {ADDRESS_REGEX}movl .* '''])
+0: {ADDRESS_REGEX}movl .* '''])
 
 # We stop tracing
 self.expect("thread trace stop")
@@ -152,12 +152,12 @@
 self.expect("thread trace dump instructions -f",
 patterns=[f'''thread #1: tid = .*
   a.out`main \+ 20 at main.cpp:5
-\[ 0\] {ADDRESS_REGEX}xorl'''])
+0: {ADDRESS_REGEX}xorl'''])