[Lldb-commits] [PATCH] D44042: Ensure that trailing characters aren't included in PECOFF section names

[walter erquinigo via Phabricator via lldb-commits]

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

good catch


https://reviews.llvm.org/D44042



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

[Lldb-commits] [PATCH] D154030: [lldb-vscode] Creating a new flag for adjusting the behavior of evaluation repl expressions to allow users to more easily invoke lldb commands.

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Could you split the changes to the selected thread out of this? I'm seeing two 
features being implemented in this patch.
Other than that, it looks pretty good!




Comment at: lldb/tools/lldb-vscode/Options.td:20-38
+def port: S<"port">,
   MetaVarName<"">,
   HelpText<"Communicate with the lldb-vscode tool over the defined port.">;
 def: Separate<["-"], "p">,
   Alias,
   HelpText<"Alias for --port">;
 

+1



Comment at: lldb/tools/lldb-vscode/VSCode.cpp:768
 
+  return true; 
+}

use clang-format please



Comment at: lldb/tools/lldb-vscode/VSCode.h:89
+
+/// A huersitic for determining the context of an evaluation.
+enum class ExpressionContext {

typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154030

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

[Lldb-commits] [PATCH] D154030: [lldb-vscode] Creating a new flag for adjusting the behavior of evaluation repl expressions to allow users to more easily invoke lldb commands.

[walter erquinigo via Phabricator via lldb-commits]

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

nice!!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154030

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

[Lldb-commits] [PATCH] D154989: [lldb-vsocde] Add a 'continued' event for programmatic continue events.

[walter erquinigo via Phabricator via lldb-commits]

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

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154989

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

[Lldb-commits] [PATCH] D154990: [lldb-vsocde] Cleaning up the usage of the Separate helper in Options.td.

[walter erquinigo via Phabricator via lldb-commits]

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

wooo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154990

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

[Lldb-commits] [PATCH] D155248: [lldb-vscode] Creating a new flag for adjusting the behavior of evaluation repl expressions to allow users to more easily invoke lldb commands.

[walter erquinigo via Phabricator via lldb-commits]

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

fingers crossed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155248

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

[Lldb-commits] [PATCH] D154989: [lldb-vsocde] Add a 'continued' event for programmatic continue events.

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154989

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

[Lldb-commits] [PATCH] D156493: [lldb-vsocde] Adding support for the "disassemble" request.

[walter erquinigo via Phabricator via lldb-commits]

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

you are my hero


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156493

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

[Lldb-commits] [PATCH] D156465: [lldb-vscode] Adding support for displaying backtraces.

[walter erquinigo via Phabricator via lldb-commits]

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

fancy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156465

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

[Lldb-commits] [PATCH] D140630: [lldb-vscode] Add data breakpoint support

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Greg hasn't been working for a while. You can go ahead and land this, and when 
he gets back he can share some feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140630

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

[Lldb-commits] [PATCH] D156977: [lldb][lldb-vscode] Fix nullptr dereference when JSON is not an object

[walter erquinigo via Phabricator via lldb-commits]

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

thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156977

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

[Lldb-commits] [PATCH] D156979: [lldb][lldb-vscode] Pretty print JSON to log files

[walter erquinigo via Phabricator via lldb-commits]

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

nice improvement for sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156979

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

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added a reviewer: bulbazord.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

https://github.com/llvm/llvm-project/commit/59237bb52c9483fce395428bfab5996eabe54ed0
 changed the behavior of the `SetTimeout` and `GetTimeout` methods of 
`EvaluateExpressionOptions`, which broke the Mojo REPL and related services 
(https://docs.modular.com/mojo/) because it relies on having infinite timeouts. 
That's a necessity because developers often use the REPL for executing 
extremely long-running numeric jobs. Having said that, 
`EvaluateExpressionOptions` shouldn't be that opinionated on this matter 
anyway. Instead, it should be the responsibility of the evaluator to define 
which timeout to use for each specific case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157764

Files:
  lldb/include/lldb/Target/Target.h


Index: lldb/include/lldb/Target/Target.h
===
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -346,16 +346,9 @@
 m_use_dynamic = dynamic;
   }
 
-  const Timeout &GetTimeout() const {
-assert(m_timeout && m_timeout->count() > 0);
-return m_timeout;
-  }
+  const Timeout &GetTimeout() const { return m_timeout; }
 
-  void SetTimeout(const Timeout &timeout) {
-// Disallow setting a non-zero timeout.
-if (timeout && timeout->count() > 0)
-  m_timeout = timeout;
-  }
+  void SetTimeout(const Timeout &timeout) { m_timeout = timeout; }
 
   const Timeout &GetOneThreadTimeout() const {
 return m_one_thread_timeout;


Index: lldb/include/lldb/Target/Target.h
===
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -346,16 +346,9 @@
 m_use_dynamic = dynamic;
   }
 
-  const Timeout &GetTimeout() const {
-assert(m_timeout && m_timeout->count() > 0);
-return m_timeout;
-  }
+  const Timeout &GetTimeout() const { return m_timeout; }
 
-  void SetTimeout(const Timeout &timeout) {
-// Disallow setting a non-zero timeout.
-if (timeout && timeout->count() > 0)
-  m_timeout = timeout;
-  }
+  void SetTimeout(const Timeout &timeout) { m_timeout = timeout; }
 
   const Timeout &GetOneThreadTimeout() const {
 return m_one_thread_timeout;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Btw, the SBAPI expects the old behavior.

  // Set the timeout for the expression, 0 means wait forever.
  void SetTimeoutInMicroSeconds(uint32_t timeout = 0);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157764

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

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

[walter erquinigo via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa4dbdf474993: [LLDB] Allow expression evaluators to set 
arbitrary timeouts (authored by wallace).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157764

Files:
  lldb/include/lldb/Target/Target.h


Index: lldb/include/lldb/Target/Target.h
===
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -346,16 +346,9 @@
 m_use_dynamic = dynamic;
   }
 
-  const Timeout &GetTimeout() const {
-assert(m_timeout && m_timeout->count() > 0);
-return m_timeout;
-  }
+  const Timeout &GetTimeout() const { return m_timeout; }
 
-  void SetTimeout(const Timeout &timeout) {
-// Disallow setting a non-zero timeout.
-if (timeout && timeout->count() > 0)
-  m_timeout = timeout;
-  }
+  void SetTimeout(const Timeout &timeout) { m_timeout = timeout; }
 
   const Timeout &GetOneThreadTimeout() const {
 return m_one_thread_timeout;


Index: lldb/include/lldb/Target/Target.h
===
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -346,16 +346,9 @@
 m_use_dynamic = dynamic;
   }
 
-  const Timeout &GetTimeout() const {
-assert(m_timeout && m_timeout->count() > 0);
-return m_timeout;
-  }
+  const Timeout &GetTimeout() const { return m_timeout; }
 
-  void SetTimeout(const Timeout &timeout) {
-// Disallow setting a non-zero timeout.
-if (timeout && timeout->count() > 0)
-  m_timeout = timeout;
-  }
+  void SetTimeout(const Timeout &timeout) { m_timeout = timeout; }
 
   const Timeout &GetOneThreadTimeout() const {
 return m_one_thread_timeout;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D158010: [lldb] Allow synthetic providers in C++ and fix linking problems

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Commands/CommandObjectType.cpp:2174
 
-#if LLDB_ENABLE_PYTHON
-

electriclilies wrote:
> jingham wrote:
> > electriclilies wrote:
> > > rriddle wrote:
> > > > Why is this dropped?
> > > Walter and I want to use the synthetic types from C++, but right now it's 
> > > only supported in Python. The motivation behind this is to make it so 
> > > that we can actually use the synthetic types. 
> > Just to be clear.  There are already lots of synthetic child providers 
> > implemented in C++ in in lldb already (look for CXXSyntheticChildren 
> > constructors in CPlusPlusLanguage.cpp for instance).  
> > 
> > I think what you are saying is that you are removing the restriction that 
> > you have to BUILD lldb with Python in order to add C++ implemented 
> > summaries.  If that's indeed what you are saying, then this is fine, since 
> > that seems an odd restriction...
> > 
> > But it would be nice to have some kind of testing that this actually works, 
> > since I don't think any of our bots build lldb w/o Python.
> Yes that's exactly it! I agree it would be nice to have a test for it but I 
> don't know how to add it. 
@jingham , indeed, we couldn't add buildbot tests for this they all build with 
python support.

However, a good compromise is to enable all these commands except for the `add` 
ones, because they have the option to use python-defined extensions. `clear`, 
`delete`, `info` and `list` should be fine even without python scripting 
support.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158010

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

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

@jasonmolenda , yep. I have already reverted this patch. I'll figure out how to 
make that work nicely with these changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157764

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

[Lldb-commits] [PATCH] D157764: [LLDB] Allow expression evaluators to set arbitrary timeouts

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

@JDevlieghere , that worked. Thanks!!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157764

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

[Lldb-commits] [PATCH] D158010: [lldb] Allow synthetic providers in C++ and fix linking problems

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Commands/CommandObjectType.cpp:2174
 
-#if LLDB_ENABLE_PYTHON
-

wallace wrote:
> electriclilies wrote:
> > jingham wrote:
> > > electriclilies wrote:
> > > > rriddle wrote:
> > > > > Why is this dropped?
> > > > Walter and I want to use the synthetic types from C++, but right now 
> > > > it's only supported in Python. The motivation behind this is to make it 
> > > > so that we can actually use the synthetic types. 
> > > Just to be clear.  There are already lots of synthetic child providers 
> > > implemented in C++ in in lldb already (look for CXXSyntheticChildren 
> > > constructors in CPlusPlusLanguage.cpp for instance).  
> > > 
> > > I think what you are saying is that you are removing the restriction that 
> > > you have to BUILD lldb with Python in order to add C++ implemented 
> > > summaries.  If that's indeed what you are saying, then this is fine, 
> > > since that seems an odd restriction...
> > > 
> > > But it would be nice to have some kind of testing that this actually 
> > > works, since I don't think any of our bots build lldb w/o Python.
> > Yes that's exactly it! I agree it would be nice to have a test for it but I 
> > don't know how to add it. 
> @jingham , indeed, we couldn't add buildbot tests for this they all build 
> with python support.
> 
> However, a good compromise is to enable all these commands except for the 
> `add` ones, because they have the option to use python-defined extensions. 
> `clear`, `delete`, `info` and `list` should be fine even without python 
> scripting support.
> 
> What do you think?
After a second inspection of this diff, I want to rephrase my previous comment:

This patch is only enabling the `type synthetic` commands, and `type synthetic 
add` is the only one that can receive a python input. In fact, it only works 
with python, so @electriclilies, please keep `type synthetic add` guarded by 
`#if LLDB_ENABLE_PYTHON`, because it just doesn't work otherwise.

@jingham , we can't really test this in the buildbots, so, all good if Lily 
does some basic manual testing showing that these commands work even when the 
python scripting support is not enabled? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158010

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

[Lldb-commits] [PATCH] D158788: [lldb-vscode] Use a switch to avoid else-after-return (NFC)

[walter erquinigo via Phabricator via lldb-commits]

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

Thanks!


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

https://reviews.llvm.org/D158788

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

[Lldb-commits] [PATCH] D158801: [lldb-vscode] Update package.json

[walter erquinigo via Phabricator via lldb-commits]

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

Awesome!


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

https://reviews.llvm.org/D158801

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

[Lldb-commits] [PATCH] D158010: [lldb] Allow synthetic providers in C++ and fix linking problems

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

In D158010#4617611 , @electriclilies 
wrote:

> @wallace The type synthetic add command already disables itself if python is 
> not enabled (the check is at CommandObjectType::1564), so I think there 
> aren't any changes I need to make

Great! Let's wait for Jim then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158010

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

[Lldb-commits] [PATCH] D158893: [lldb] Fix TestVSCode_completions on Darwin

[walter erquinigo via Phabricator via lldb-commits]

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

Awesome!!


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

https://reviews.llvm.org/D158893

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

[Lldb-commits] [PATCH] D158958: [LLDB][REPL] Change the default tab size

[walter erquinigo via Phabricator via lldb-commits]

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

The REPL has a default tab size of 4 spaces, which seems to be a bit too much. 
The reason is that the REPL transforms tabs into spaces, and therefore whenever 
you want to manually deindent, you need to delete at least 4 characters. On the 
other hand, using 2 as default results in less keystrokes, without hurting 
readability.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158958

Files:
  lldb/source/Core/CoreProperties.td


Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -173,8 +173,8 @@
 Desc<"If true, LLDB will print the values of variables declared in an 
expression. Currently only supported in the REPL (default: true).">;
   def TabSize: Property<"tab-size", "UInt64">,
 Global,
-DefaultUnsignedValue<4>,
-Desc<"The tab size to use when indenting code in multi-line input mode 
(default: 4).">;
+DefaultUnsignedValue<2>,
+Desc<"The tab size to use when indenting code in multi-line input mode 
(default: 2).">;
   def EscapeNonPrintables: Property<"escape-non-printables", "Boolean">,
 Global,
 DefaultTrue,


Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -173,8 +173,8 @@
 Desc<"If true, LLDB will print the values of variables declared in an expression. Currently only supported in the REPL (default: true).">;
   def TabSize: Property<"tab-size", "UInt64">,
 Global,
-DefaultUnsignedValue<4>,
-Desc<"The tab size to use when indenting code in multi-line input mode (default: 4).">;
+DefaultUnsignedValue<2>,
+Desc<"The tab size to use when indenting code in multi-line input mode (default: 2).">;
   def EscapeNonPrintables: Property<"escape-non-printables", "Boolean">,
 Global,
 DefaultTrue,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D158958: [LLDB][REPL] Change the default tab size

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 553804.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158958

Files:
  lldb/source/Core/CoreProperties.td


Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -173,8 +173,8 @@
 Desc<"If true, LLDB will print the values of variables declared in an 
expression. Currently only supported in the REPL (default: true).">;
   def TabSize: Property<"tab-size", "UInt64">,
 Global,
-DefaultUnsignedValue<4>,
-Desc<"The tab size to use when indenting code in multi-line input mode 
(default: 4).">;
+DefaultUnsignedValue<2>,
+Desc<"The tab size to use when indenting code in multi-line input mode 
(default: 2).">;
   def EscapeNonPrintables: Property<"escape-non-printables", "Boolean">,
 Global,
 DefaultTrue,


Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -173,8 +173,8 @@
 Desc<"If true, LLDB will print the values of variables declared in an expression. Currently only supported in the REPL (default: true).">;
   def TabSize: Property<"tab-size", "UInt64">,
 Global,
-DefaultUnsignedValue<4>,
-Desc<"The tab size to use when indenting code in multi-line input mode (default: 4).">;
+DefaultUnsignedValue<2>,
+Desc<"The tab size to use when indenting code in multi-line input mode (default: 2).">;
   def EscapeNonPrintables: Property<"escape-non-printables", "Boolean">,
 Global,
 DefaultTrue,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D158958: [LLDB][REPL] Change the default tab size

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

@aprantl, how should I proceed with the swift branch? Should I create a new 
branch in https://github.com/apple/swift and share it here so it's available 
for whoever does the merge?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158958

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

[Lldb-commits] [PATCH] D159031: [LLDB] Fix IOHandlerEditline::GetCurrentLines()

[walter erquinigo via Phabricator via lldb-commits]

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

This method was working as expected if `LLDB_ENABLE_LIBEDIT` is false, however, 
if it was true, then the variable `m_current_lines_ptr` was always pointing to 
an empty list, because Editline only updates its contents once the full input 
has been completed (see 
https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/common/Editline.cpp#L1576).

A simple fix is to invoke `Editline::GetInputAsStringList()` from 
`GetCurrentLines()`, which is already used in many places as the common way to 
get the full input list.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159031

Files:
  lldb/include/lldb/Core/IOHandler.h
  lldb/include/lldb/Host/Editline.h
  lldb/source/Core/IOHandler.cpp
  lldb/source/Expression/REPL.cpp


Index: lldb/source/Expression/REPL.cpp
===
--- lldb/source/Expression/REPL.cpp
+++ lldb/source/Expression/REPL.cpp
@@ -528,17 +528,15 @@
   current_code.append(m_code.CopyList());
 
   IOHandlerEditline &editline = static_cast(io_handler);
-  const StringList *current_lines = editline.GetCurrentLines();
-  if (current_lines) {
-const uint32_t current_line_idx = editline.GetCurrentLineIndex();
-
-if (current_line_idx < current_lines->GetSize()) {
-  for (uint32_t i = 0; i < current_line_idx; ++i) {
-const char *line_cstr = current_lines->GetStringAtIndex(i);
-if (line_cstr) {
-  current_code.append("\n");
-  current_code.append(line_cstr);
-}
+  StringList current_lines = editline.GetCurrentLines();
+  const uint32_t current_line_idx = editline.GetCurrentLineIndex();
+
+  if (current_line_idx < current_lines.GetSize()) {
+for (uint32_t i = 0; i < current_line_idx; ++i) {
+  const char *line_cstr = current_lines.GetStringAtIndex(i);
+  if (line_cstr) {
+current_code.append("\n");
+current_code.append(line_cstr);
   }
 }
   }
Index: lldb/source/Core/IOHandler.cpp
===
--- lldb/source/Core/IOHandler.cpp
+++ lldb/source/Core/IOHandler.cpp
@@ -508,6 +508,16 @@
   return m_curr_line_idx;
 }
 
+StringList IOHandlerEditline::GetCurrentLines() const {
+#if LLDB_ENABLE_LIBEDIT
+  if (m_editline_up)
+return m_editline_up->GetInputAsStringList();
+#endif
+  if (m_current_lines_ptr)
+return *m_current_lines_ptr;
+  return StringList();
+}
+
 bool IOHandlerEditline::GetLines(StringList &lines, bool &interrupted) {
   m_current_lines_ptr = &lines;
 
Index: lldb/include/lldb/Host/Editline.h
===
--- lldb/include/lldb/Host/Editline.h
+++ lldb/include/lldb/Host/Editline.h
@@ -227,6 +227,9 @@
 
   void PrintAsync(Stream *stream, const char *s, size_t len);
 
+  /// Convert the current input lines into a UTF8 StringList
+  StringList GetInputAsStringList(int line_count = UINT32_MAX);
+
 private:
   /// Sets the lowest line number for multi-line editing sessions.  A value of
   /// zero suppresses
@@ -282,9 +285,6 @@
   /// Save the line currently being edited
   void SaveEditedLine();
 
-  /// Convert the current input lines into a UTF8 StringList
-  StringList GetInputAsStringList(int line_count = UINT32_MAX);
-
   /// Replaces the current multi-line session with the next entry from history.
   unsigned char RecallHistory(HistoryOperation op);
 
Index: lldb/include/lldb/Core/IOHandler.h
===
--- lldb/include/lldb/Core/IOHandler.h
+++ lldb/include/lldb/Core/IOHandler.h
@@ -403,7 +403,7 @@
 
   void SetInterruptExits(bool b) { m_interrupt_exits = b; }
 
-  const StringList *GetCurrentLines() const { return m_current_lines_ptr; }
+  StringList GetCurrentLines() const;
 
   uint32_t GetCurrentLineIndex() const;
 


Index: lldb/source/Expression/REPL.cpp
===
--- lldb/source/Expression/REPL.cpp
+++ lldb/source/Expression/REPL.cpp
@@ -528,17 +528,15 @@
   current_code.append(m_code.CopyList());
 
   IOHandlerEditline &editline = static_cast(io_handler);
-  const StringList *current_lines = editline.GetCurrentLines();
-  if (current_lines) {
-const uint32_t current_line_idx = editline.GetCurrentLineIndex();
-
-if (current_line_idx < current_lines->GetSize()) {
-  for (uint32_t i = 0; i < current_line_idx; ++i) {
-const char *line_cstr = current_lines->GetStringAtIndex(i);
-if (line_cstr) {
-  current_code.append("\n");
-  current_code.append(line_cstr);
-}
+  StringList current_lines = editline.GetCurrentLines();
+  const uint32_t current_line_idx = editline.GetCurrentLineIndex();
+
+  if (current_line_idx < 

[Lldb-commits] [PATCH] D159031: [LLDB] Fix IOHandlerEditline::GetCurrentLines()

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Expression/REPL.cpp:535
+  if (current_line_idx < current_lines.GetSize()) {
+for (uint32_t i = 0; i < current_line_idx; ++i) {
+  const char *line_cstr = current_lines.GetStringAtIndex(i);

aprantl wrote:
> `for (const char *line_cstr : current_lines)` ?
actually the current code is fine because we don't want to iterate through the 
entire set, we just want the first `current_line_idx + 1` elements


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159031

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

[Lldb-commits] [PATCH] D159031: [LLDB] Fix IOHandlerEditline::GetCurrentLines()

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 554712.

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

https://reviews.llvm.org/D159031

Files:
  lldb/include/lldb/Core/IOHandler.h
  lldb/include/lldb/Host/Editline.h
  lldb/source/Core/IOHandler.cpp
  lldb/source/Expression/REPL.cpp


Index: lldb/source/Expression/REPL.cpp
===
--- lldb/source/Expression/REPL.cpp
+++ lldb/source/Expression/REPL.cpp
@@ -528,17 +528,15 @@
   current_code.append(m_code.CopyList());
 
   IOHandlerEditline &editline = static_cast(io_handler);
-  const StringList *current_lines = editline.GetCurrentLines();
-  if (current_lines) {
-const uint32_t current_line_idx = editline.GetCurrentLineIndex();
-
-if (current_line_idx < current_lines->GetSize()) {
-  for (uint32_t i = 0; i < current_line_idx; ++i) {
-const char *line_cstr = current_lines->GetStringAtIndex(i);
-if (line_cstr) {
-  current_code.append("\n");
-  current_code.append(line_cstr);
-}
+  StringList current_lines = editline.GetCurrentLines();
+  const uint32_t current_line_idx = editline.GetCurrentLineIndex();
+
+  if (current_line_idx < current_lines.GetSize()) {
+for (uint32_t i = 0; i < current_line_idx; ++i) {
+  const char *line_cstr = current_lines.GetStringAtIndex(i);
+  if (line_cstr) {
+current_code.append("\n");
+current_code.append(line_cstr);
   }
 }
   }
Index: lldb/source/Core/IOHandler.cpp
===
--- lldb/source/Core/IOHandler.cpp
+++ lldb/source/Core/IOHandler.cpp
@@ -508,6 +508,21 @@
   return m_curr_line_idx;
 }
 
+StringList IOHandlerEditline::GetCurrentLines() const {
+#if LLDB_ENABLE_LIBEDIT
+  if (m_editline_up)
+return m_editline_up->GetInputAsStringList();
+#endif
+  // When libedit is not used, the current lines can be gotten from
+  // `m_current_lines_ptr`, which is updated whenever a new line is processed.
+  // This doesn't happen when libedit is used, in which case
+  // `m_current_lines_ptr` is only updated when the full input is terminated.
+
+  if (m_current_lines_ptr)
+return *m_current_lines_ptr;
+  return StringList();
+}
+
 bool IOHandlerEditline::GetLines(StringList &lines, bool &interrupted) {
   m_current_lines_ptr = &lines;
 
Index: lldb/include/lldb/Host/Editline.h
===
--- lldb/include/lldb/Host/Editline.h
+++ lldb/include/lldb/Host/Editline.h
@@ -227,6 +227,9 @@
 
   void PrintAsync(Stream *stream, const char *s, size_t len);
 
+  /// Convert the current input lines into a UTF8 StringList
+  StringList GetInputAsStringList(int line_count = UINT32_MAX);
+
 private:
   /// Sets the lowest line number for multi-line editing sessions.  A value of
   /// zero suppresses
@@ -282,9 +285,6 @@
   /// Save the line currently being edited
   void SaveEditedLine();
 
-  /// Convert the current input lines into a UTF8 StringList
-  StringList GetInputAsStringList(int line_count = UINT32_MAX);
-
   /// Replaces the current multi-line session with the next entry from history.
   unsigned char RecallHistory(HistoryOperation op);
 
Index: lldb/include/lldb/Core/IOHandler.h
===
--- lldb/include/lldb/Core/IOHandler.h
+++ lldb/include/lldb/Core/IOHandler.h
@@ -403,7 +403,7 @@
 
   void SetInterruptExits(bool b) { m_interrupt_exits = b; }
 
-  const StringList *GetCurrentLines() const { return m_current_lines_ptr; }
+  StringList GetCurrentLines() const;
 
   uint32_t GetCurrentLineIndex() const;
 


Index: lldb/source/Expression/REPL.cpp
===
--- lldb/source/Expression/REPL.cpp
+++ lldb/source/Expression/REPL.cpp
@@ -528,17 +528,15 @@
   current_code.append(m_code.CopyList());
 
   IOHandlerEditline &editline = static_cast(io_handler);
-  const StringList *current_lines = editline.GetCurrentLines();
-  if (current_lines) {
-const uint32_t current_line_idx = editline.GetCurrentLineIndex();
-
-if (current_line_idx < current_lines->GetSize()) {
-  for (uint32_t i = 0; i < current_line_idx; ++i) {
-const char *line_cstr = current_lines->GetStringAtIndex(i);
-if (line_cstr) {
-  current_code.append("\n");
-  current_code.append(line_cstr);
-}
+  StringList current_lines = editline.GetCurrentLines();
+  const uint32_t current_line_idx = editline.GetCurrentLineIndex();
+
+  if (current_line_idx < current_lines.GetSize()) {
+for (uint32_t i = 0; i < current_line_idx; ++i) {
+  const char *line_cstr = current_lines.GetStringAtIndex(i);
+  if (line_cstr) {
+current_code.append("\n");
+current_code.append(line_cstr);
   }
 }
   }
Index: lldb/source/Core/IOHandler.cpp
===
--- lldb/source/Core/IO

[Lldb-commits] [PATCH] D159031: [LLDB] Fix IOHandlerEditline::GetCurrentLines()

[walter erquinigo via Phabricator via lldb-commits]

wallace marked an inline comment as done.
wallace added a comment.

@aprantl , it turns out that there are no tests for this. I also don't know how 
easy it'd be to test this very specific feature, because it relies on the 
terminal not being affected by stuff like test runners.


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

https://reviews.llvm.org/D159031

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

[Lldb-commits] [PATCH] D158010: [lldb] Allow synthetic providers in C++ and fix linking problems

[walter erquinigo via Phabricator via lldb-commits]

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

For the sake of unblocking this diff, I'm accepting it because there's no clear 
actionable feedback at the moment.  @electriclilies, please follow up any 
feedback that is submitted post-landing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158010

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

[Lldb-commits] [PATCH] D158010: [lldb] Allow synthetic providers in C++ and fix linking problems

[walter erquinigo via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG79d5d9a0824d: [lldb] Allow synthetic providers in C++ and 
fix linking problems (authored by wallace).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158010

Files:
  lldb/include/lldb/DataFormatters/TypeSynthetic.h
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/DataFormatters/TypeSynthetic.cpp

Index: lldb/source/DataFormatters/TypeSynthetic.cpp
===
--- lldb/source/DataFormatters/TypeSynthetic.cpp
+++ lldb/source/DataFormatters/TypeSynthetic.cpp
@@ -84,6 +84,27 @@
   return std::string(sstr.GetString());
 }
 
+SyntheticChildren::SyntheticChildren(const Flags &flags) : m_flags(flags) {}
+
+SyntheticChildren::~SyntheticChildren() = default;
+
+CXXSyntheticChildren::CXXSyntheticChildren(
+const SyntheticChildren::Flags &flags, const char *description,
+CreateFrontEndCallback callback)
+: SyntheticChildren(flags), m_create_callback(std::move(callback)),
+  m_description(description ? description : "") {}
+
+CXXSyntheticChildren::~CXXSyntheticChildren() = default;
+
+bool SyntheticChildren::IsScripted() { return false; }
+
+std::string SyntheticChildren::GetDescription() { return ""; }
+
+SyntheticChildrenFrontEnd::AutoPointer
+SyntheticChildren::GetFrontEnd(ValueObject &backend) {
+  return nullptr;
+}
+
 std::string CXXSyntheticChildren::GetDescription() {
   StreamString sstr;
   sstr.Printf("%s%s%s %s", Cascades() ? "" : " (not cascading)",
Index: lldb/source/Core/ValueObject.cpp
===
--- lldb/source/Core/ValueObject.cpp
+++ lldb/source/Core/ValueObject.cpp
@@ -218,10 +218,8 @@
 SetValueFormat(DataVisualization::GetFormat(*this, eNoDynamicValues));
 SetSummaryFormat(
 DataVisualization::GetSummaryFormat(*this, GetDynamicValueType()));
-#if LLDB_ENABLE_PYTHON
 SetSyntheticChildren(
 DataVisualization::GetSyntheticChildren(*this, GetDynamicValueType()));
-#endif
   }
 
   return any_change;
@@ -1153,7 +1151,7 @@
 Stream &s, ValueObjectRepresentationStyle val_obj_display,
 Format custom_format, PrintableRepresentationSpecialCases special,
 bool do_dump_error) {
-
+
   // If the ValueObject has an error, we might end up dumping the type, which
   // is useful, but if we don't even have a type, then don't examine the object
   // further as that's not meaningful, only the error is.
@@ -2766,15 +2764,15 @@
 
 ValueObjectSP ValueObject::Cast(const CompilerType &compiler_type) {
   // Only allow casts if the original type is equal or larger than the cast
-  // type.  We don't know how to fetch more data for all the ConstResult types, 
+  // type.  We don't know how to fetch more data for all the ConstResult types,
   // so we can't guarantee this will work:
   Status error;
   CompilerType my_type = GetCompilerType();
 
-  ExecutionContextScope *exe_scope 
+  ExecutionContextScope *exe_scope
   = ExecutionContext(GetExecutionContextRef())
   .GetBestExecutionContextScope();
-  if (compiler_type.GetByteSize(exe_scope) 
+  if (compiler_type.GetByteSize(exe_scope)
   <= GetCompilerType().GetByteSize(exe_scope)) {
 return DoCast(compiler_type);
   }
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -2171,8 +2171,6 @@
"Show a list of current filters.") {}
 };
 
-#if LLDB_ENABLE_PYTHON
-
 // CommandObjectTypeSynthList
 
 class CommandObjectTypeSynthList
@@ -2184,8 +2182,6 @@
 "Show a list of current synthetic providers.") {}
 };
 
-#endif
-
 // CommandObjectTypeFilterDelete
 
 class CommandObjectTypeFilterDelete : public CommandObjectTypeFormatterDelete {
@@ -2197,8 +2193,6 @@
   ~CommandObjectTypeFilterDelete() override = default;
 };
 
-#if LLDB_ENABLE_PYTHON
-
 // CommandObjectTypeSynthDelete
 
 class CommandObjectTypeSynthDelete : public CommandObjectTypeFormatterDelete {
@@ -2210,7 +2204,6 @@
   ~CommandObjectTypeSynthDelete() override = default;
 };
 
-#endif
 
 // CommandObjectTypeFilterClear
 
@@ -,7 +2215,6 @@
 "Delete all existing filter.") {}
 };
 
-#if LLDB_ENABLE_PYTHON
 // CommandObjectTypeSynthClear
 
 class CommandObjectTypeSynthClear : public CommandObjectTypeFormatterClear {
@@ -2393,7 +2385,6 @@
   return true;
 }
 
-#endif
 #define LLDB_OPTIONS_type_filter_add
 #include "CommandOptions.inc"
 
@@ -2941,8 +2932,6 @@
   ~CommandObjectTypeFormat() override = default;
 };
 
-#if LLDB_ENABLE_PYTHON
-
 class CommandObjectTypeSynth : public CommandObjectMultiword {
 public:
   CommandObjectTypeSynth(CommandInterpreter &inte

[Lldb-commits] [PATCH] D158010: [lldb] Allow synthetic providers in C++ and fix linking problems

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

> add a way to make an SBTypeSynthetic from the appropriate C++ class

That's a great idea. Once we implement a couple of providers for our plugin 
I'll revisit the idea. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158010

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

[Lldb-commits] [PATCH] D115033: [lldb-vscode] Report supportsModulesRequest=true

[walter erquinigo via Phabricator via lldb-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115033

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

[Lldb-commits] [PATCH] D115137: [formatters] Add a pointer and reference tests for a list and forward_list formatters for libstdcpp and libcxx

[walter erquinigo via Phabricator via lldb-commits]

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

thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115137

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

[Lldb-commits] [PATCH] D115178: Unify libstdcpp and libcxx formatters for `std::optional`

[walter erquinigo via Phabricator via lldb-commits]

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

back to your queue until you get things working


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115178

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

[Lldb-commits] [PATCH] D115178: Unify libstdcpp and libcxx formatters for `std::optional`

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

good job! Only some cosmetic changes are needed and you also need to delete the 
python code




Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:775-779
+  // AddCXXSummary(cpp_category_sp,
+  //   lldb_private::formatters::LibcxxOptionalSummaryProvider,
+  //   "libc++ std::optional summary provider",
+  //   ConstString("^std::__[[:alnum:]]+::optional<.+>(( )?&)?$"),
+  //   stl_summary_flags, true);

remove this



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:922-926
+  // cpp_category_sp->GetRegexTypeSyntheticsContainer()->Add(
+  // RegularExpression("^std::optional<.+>(( )?&)?$"),
+  // SyntheticChildrenSP(new ScriptedSyntheticChildren(
+  // stl_synth_flags,
+  // "lldb.formatters.cpp.gnu_libstdcpp.StdOptionalSynthProvider")));

same here



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:950-954
+  // cpp_category_sp->GetRegexTypeSummariesContainer()->Add(
+  // RegularExpression("^std::optional<.+>(( )?&)?$"),
+  // TypeSummaryImplSP(new ScriptSummaryFormat(
+  // stl_summary_flags,
+  // "lldb.formatters.cpp.gnu_libstdcpp.StdOptionalSummaryProvider")));

same



Comment at: lldb/source/Plugins/Language/CPlusPlus/Generic.h:1-2
+//===-- LibCxx.h ---*- C++
+//-*-===//
+//

make this one line (=80 chars)



Comment at: lldb/source/Plugins/Language/CPlusPlus/Generic.h:10-11
+
+// #ifndef LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_LIBCXX_H
+// #define LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_LIBCXX_H
+

this shouldn't be a comment, and use
  LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_GENERIC_H



Comment at: lldb/source/Plugins/Language/CPlusPlus/Generic.h:26
+
+// #endif // LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_LIBCXX_H

LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_GENERIC_H



Comment at: lldb/source/Plugins/Language/CPlusPlus/GenericOptional.cpp:1-2
+//===-- GenericOptional.cpp
+---===//
+//

80 chars



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp:78-84
+// SyntheticChildrenFrontEnd *
+// formatters::LibcxxOptionalFrontEndCreator(CXXSyntheticChildren *,
+//   lldb::ValueObjectSP valobj_sp) {
+//   if (valobj_sp)
+// return new OptionalFrontEnd(*valobj_sp);
+//   return nullptr;
+// }

delete this entire file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115178

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

[Lldb-commits] [PATCH] D115178: Unify libstdcpp and libcxx formatters for `std::optional`

[walter erquinigo via Phabricator via lldb-commits]

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

great job!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115178

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

[Lldb-commits] [PATCH] D114008: [formatters] Add a deque formatter for libstdcpp and fix the libcxx one

[walter erquinigo via Phabricator via lldb-commits]

wallace closed this revision.
wallace added a comment.

committed as 2ea3c8a50add5436cf939d59c3235408ca0255c1 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114008

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

[Lldb-commits] [PATCH] D115178: Unify libstdcpp and libcxx formatters for `std::optional`

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

committed as cfb075089128b2e5918afd0ce21ec10bf455e5ab 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115178

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

[Lldb-commits] [PATCH] D115324: Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster.

[walter erquinigo via Phabricator via lldb-commits]

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

much nicer than the first version. I'm just asking a few minor things.




Comment at: lldb/include/lldb/Host/FileSystem.h:147
+  ///
+  /// The path must specify a file an not a directory.
+  /// \{





Comment at: lldb/include/lldb/Symbol/Symbol.h:256
+  ///   and cache file size reduction. Strings are stored as uint32_t string
+  ///   table offsets in the cache data.
+  bool Decode(const DataExtractor &data, lldb::offset_t *offset_ptr,

better mention that the return value means



Comment at: lldb/include/lldb/Symbol/Symtab.h:148
+  ///   otherwise.
+  bool Decode(const DataExtractor &data, lldb::offset_t *offset_ptr,
+  bool &uuid_mismatch);

ditto



Comment at: lldb/include/lldb/Utility/DataFileCache.h:59
+  ///   A valid unique pointer to a memory buffer if the data is available, or
+  ///   an shared pointer that contains NULL if the data is not available.
+  std::unique_ptr GetCachedData(llvm::StringRef key);





Comment at: lldb/include/lldb/Utility/DataFileCache.h:175
+/// class helps create string tables.
+class StringTableCreator {
+public:

I'd just call this ConstStringTable for callers to know that this is using 
ConstString as its storage pool and that the data doesn't just go away when the 
object is disposed. 



Comment at: lldb/source/Utility/DataFileCache.cpp:58
+  else
+consumeError(cache_or_err.takeError());
+}

could you create a new lldb log channel where this information is logged? it'll 
help investigating issues with the cache (if they ever happen). Same for 
everywhere llvm::Error's or lldb::Status messages are lost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115324

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

[Lldb-commits] [PATCH] D115324: Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster.

[walter erquinigo via Phabricator via lldb-commits]

wallace accepted this revision.
wallace 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/D115324/new/

https://reviews.llvm.org/D115324

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

[Lldb-commits] [PATCH] D115951: Cache the manual DWARF index out to the LLDB cache directory when the LLDB index cache is enabled.

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Symbol/Symtab.h:279
   m_mutex; // Provide thread safety for this symbol table
-  bool m_file_addr_to_index_computed : 1, m_name_indexes_computed : 1;
+  bool m_file_addr_to_index_computed : 1, m_name_indexes_computed : 1,
+m_loaded_from_cache : 1, m_saved_to_cache : 1;

teach me C++. What is this? Is everything stored as part of a single 8 byte 
section in memory?



Comment at: 
lldb/test/API/functionalities/module_cache/debug_index/TestDebugIndexCache.py:40-41
+command = "statistics dump "
+if f:
+f.write('(lldb) %s\n' % (command))
+self.ci.HandleCommand(command, return_obj, False)

i'm curious, why do you do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115951

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

[Lldb-commits] [PATCH] D115974: [formatters] Improve documentation

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added a reviewer: clayborg.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This adds some important remarks to the data formatter documentation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115974

Files:
  lldb/docs/use/variable.rst

Index: lldb/docs/use/variable.rst
===
--- lldb/docs/use/variable.rst
+++ lldb/docs/use/variable.rst
@@ -29,6 +29,52 @@
(uint8_t) x = chr='a' dec=65 hex=0x41
(intptr_t) y = 0x76f919f
 
+In addition, some data structures can encode their data in a way that is not easily
+readable to the user, in which case a data formatter could be used to show
+the data in a human readable way. For example, without a formatter, printing a
+``std::deque`` with the elements ``{2, 3, 4, 5, 6}`` would result in something
+like:
+
+::
+
+   (lldb) p a_deque
+   (std::deque >) $0 = {
+  std::_Deque_base > = {
+ _M_impl = {
+_M_map = 0x0062ceb0
+_M_map_size = 8
+_M_start = {
+   _M_cur = 0x0062cf00
+   _M_first = 0x0062cf00
+   _M_last = 0x0062d2f4
+   _M_node = 0x0062cec8
+}
+_M_finish = {
+   _M_cur = 0x0062d300
+   _M_first = 0x0062d300
+   _M_last = 0x0062d6f4
+   _M_node = 0x0062ced0
+}
+ }
+  }
+   }
+
+which is very hard to make sense of. On the other hand, a proper formatter could
+produce the following output
+
+::
+
+   (lldb) p a_deque
+   (std::deque >) $0 = size=5 {
+  [0] = 2
+  [1] = 3
+  [2] = 4
+  [3] = 5
+  [4] = 6
+   }
+
+which is what the user would expect from a good debugger.
+
 There are several features related to data visualization: formats, summaries,
 filters, synthetic children.
 
@@ -871,18 +917,23 @@
  this call should return a new LLDB SBValue object representing the child at the index given as argument
   def update(self):
  this call should be used to update the internal state of this Python object whenever the state of the variables in LLDB changes.[1]
+ Also, this method is invoked before any other method in the interface.
   def has_children(self):
  this call should return True if this object might have children, and False if this object can be guaranteed not to have children.[2]
   def get_value(self):
  this call can return an SBValue to be presented as the value of the synthetic value under consideration.[3]
 
-[1] This method is optional. Also, it may optionally choose to return a value
-(starting with SVN rev153061/LLDB-134). If it returns a value, and that value
-is True, LLDB will be allowed to cache the children and the children count it
-previously obtained, and will not return to the provider class to ask. If
-nothing, None, or anything other than True is returned, LLDB will discard the
-cached information and ask. Regardless, whenever necessary LLDB will call
-update.
+As a warning, exceptions that are thrown by python formatters are caught silently by LLDB and should be handled appropriately
+by the formatter itself. Being more specific, in case of exceptions, LLDB might assume that the given object has no children
+or it might skip printing some children.
+
+[1] This method is optional. Also, a boolean value must be returned
+(starting with SVN rev153061/LLDB-134). If ``False`` is returned, then whenever
+the process reaches a new stop, this method will be invoked again to generate an
+updated list of the children for a given variable. Otherwise, if ``True`` is returned,
+then the value is cached and this method won't be called again, effectively freezing
+the state of the value in subsequent stops. Beware that returning ``True`` incorrectly
+could show misleading information to the user.
 
 [2] This method is optional (starting with SVN rev166495/LLDB-175). While
 implementing it in terms of num_children is acceptable, implementors are
@@ -972,6 +1023,22 @@
   (int) [3] = 1234
}
 
+It's worth mentioning that LLDB invokes the synthetic child provider before invoking the
+summary string provider, which allows the latter to have access to the actual displayable
+children. This applies to both inlined summary strings and python-based summary providers.
+
+
+As a warning, when programmatically accessing the children or children count of a
+variable that has a synthetic child provider, notice that LLDB hides the actual
+raw children. For example, suppose we have a ``std::vector``, which has an actual in-memory property
+``__begin`` marking the beginning of its data. After the synthetic child provider is
+executed, the ``std::vector`` variable won't show ``__begin`` as child anymore, even through
+the SB API. It will have

[Lldb-commits] [PATCH] D115951: Cache the manual DWARF index out to the LLDB cache directory when the LLDB index cache is enabled.

[walter erquinigo via Phabricator via lldb-commits]

wallace accepted this revision.
wallace 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/D115951/new/

https://reviews.llvm.org/D115951

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

[Lldb-commits] [PATCH] D115974: [formatters] Improve documentation

[walter erquinigo via Phabricator via lldb-commits]

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

per comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115974

Files:
  lldb/docs/use/variable.rst

Index: lldb/docs/use/variable.rst
===
--- lldb/docs/use/variable.rst
+++ lldb/docs/use/variable.rst
@@ -7,7 +7,7 @@
 LLDB has a data formatters subsystem that allows users to define custom display
 options for their variables.
 
-Usually, when you type frame variable or run some expression LLDB will
+Usually, when you type ``frame variable`` or run some expression LLDB will
 automatically choose the way to display your results on a per-type basis, as in
 the following example:
 
@@ -17,7 +17,11 @@
(uint8_t) x = 'a'
(intptr_t) y = 124752287
 
-However, in certain cases, you may want to associate a different style to the display for certain datatypes. To do so, you need to give hints to the debugger
+Note: ``frame variable`` without additional arguments prints the list of
+variables of the current frame.
+
+However, in certain cases, you may want to associate a different style to the
+display for certain datatypes. To do so, you need to give hints to the debugger
 as to how variables should be displayed. The LLDB type command allows you to do
 just that.
 
@@ -29,6 +33,63 @@
(uint8_t) x = chr='a' dec=65 hex=0x41
(intptr_t) y = 0x76f919f
 
+In addition, some data structures can encode their data in a way that is not
+easily readable to the user, in which case a data formatter can be used to
+show the data in a human readable way. For example, without a formatter,
+printing a ``std::deque`` with the elements ``{2, 3, 4, 5, 6}`` would
+result in something like:
+
+::
+
+   (lldb) frame variable a_deque
+   (std::deque >) $0 = {
+  std::_Deque_base > = {
+ _M_impl = {
+_M_map = 0x0062ceb0
+_M_map_size = 8
+_M_start = {
+   _M_cur = 0x0062cf00
+   _M_first = 0x0062cf00
+   _M_last = 0x0062d2f4
+   _M_node = 0x0062cec8
+}
+_M_finish = {
+   _M_cur = 0x0062d300
+   _M_first = 0x0062d300
+   _M_last = 0x0062d6f4
+   _M_node = 0x0062ced0
+}
+ }
+  }
+   }
+
+which is very hard to make sense of.
+
+Note: ``frame variable `` prints out the variable  in the current
+frame.
+
+On the other hand, a proper formatter is able to produce the following output:
+
+::
+
+   (lldb) frame variable a_deque
+   (std::deque >) $0 = size=5 {
+  [0] = 2
+  [1] = 3
+  [2] = 4
+  [3] = 5
+  [4] = 6
+   }
+
+which is what the user would expect from a good debugger.
+
+Note: you can also use ``v `` instead of ``frame variable ``.
+
+It's worth mentioning that the ``size=5`` string is produced by a summary
+provider and the list of children is produced by a synthetic child provider.
+More information about these providers is available later in this document.
+
+
 There are several features related to data visualization: formats, summaries,
 filters, synthetic children.
 
@@ -871,18 +932,26 @@
  this call should return a new LLDB SBValue object representing the child at the index given as argument
   def update(self):
  this call should be used to update the internal state of this Python object whenever the state of the variables in LLDB changes.[1]
+ Also, this method is invoked before any other method in the interface.
   def has_children(self):
  this call should return True if this object might have children, and False if this object can be guaranteed not to have children.[2]
   def get_value(self):
  this call can return an SBValue to be presented as the value of the synthetic value under consideration.[3]
 
-[1] This method is optional. Also, it may optionally choose to return a value
-(starting with SVN rev153061/LLDB-134). If it returns a value, and that value
-is True, LLDB will be allowed to cache the children and the children count it
-previously obtained, and will not return to the provider class to ask. If
-nothing, None, or anything other than True is returned, LLDB will discard the
-cached information and ask. Regardless, whenever necessary LLDB will call
-update.
+As a warning, exceptions that are thrown by python formatters are caught
+silently by LLDB and should be handled appropriately by the formatter itself.
+Being more specific, in case of exceptions, LLDB might assume that the given
+object has no children or it might skip printing some children, as they are
+printed one by one.
+
+[1] This method is optional. Also, a boolean value must be returned
+(starting with SVN rev153061/LLDB-134). If ``False`` is returned, then
+whenever the process reaches

[Lldb-commits] [PATCH] D117288: [LLDB][NFC] Fix a typo in comment

[walter erquinigo via Phabricator via lldb-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117288

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

[Lldb-commits] [PATCH] D119400: Fix a double debug info size counting in top level stats for "statistics dump".

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

good catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119400

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

[Lldb-commits] [PATCH] D119797: Fix race condition when launching and attaching.

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

I agree with labath. Specially because I wouldn't like the user having aborted 
debug sessions if their setup takes longer than 10 seconds to get the process 
ready, which I imagine might happen with android or oculus devices using custom 
launch configs. What about using a condition variable and at the same time 
issue a progress notification mentioning that the LLDB is waiting for the 
process to stop? They could stop LLDB manually if something is not working 
properly for them in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119797

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

[Lldb-commits] [PATCH] D119831: [lldb] Add support for a "system-wide" lldbinit file

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:256
   void SourceInitFileHome(CommandReturnObject &result, bool is_repl);
+  void SourceSystemInitFile(CommandReturnObject &result);
 

JDevlieghere wrote:
> `SourceInitFileSystem` for consistency with the other two? Or maybe you were 
> planning to change the other two in another patch?
calling it SourceInitFileSystem is a bit confusing. What about using Global 
instead of System to avoid naming issues?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119831

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

[Lldb-commits] [PATCH] D119797: Fix race condition when launching and attaching.

[walter erquinigo via Phabricator via lldb-commits]

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

lgtm




Comment at: lldb/tools/lldb-vscode/VSCode.cpp:533
+  // Wait for the process hit a stopped state. When running a launch (with or
+  // without "launchCommands") or attach (with or without)= "attachCommands"),
+  // the calls might take some time to stop at the entry point since the 
command




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119797

___
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

[walter erquinigo via Phabricator via lldb-commits]

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

you got the flow correct. I left comments regarding specific details, but good 
job overall!!




Comment at: lldb/docs/lldb-gdb-remote.txt:454-479
+//}],
+//... other parameters specific to the provided tracing type
 //  }
 //
 // NOTES
 //   - "traceThreads" includes all thread traced by both "process tracing" and
 // "thread tracing".

don't forget to f



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

jj10306 wrote:
> What is the suggested way to serialize a u64 into JSON? I thought of two 
> approaches:
> 1. Store two separate u32
> 2. store the integer as a string
> 
> @wallace wdyt?
you don't need to use int64_t here. You can use the real types here and do the 
conversion in the fromJson and toJson methods



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:55
+/// TSC to nanosecond conversion.
+struct TscConverter {
+  virtual ~TscConverter() = default;

TscToWallTimeConverter is a better name



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:55
+/// TSC to nanosecond conversion.
+struct TscConverter {
+  virtual ~TscConverter() = default;

wallace wrote:
> TscToWallTimeConverter is a better name
use a class and not a struct. A struct has all its member public and we don't 
want that



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:58
+  /// Convert TSC value to nanoseconds.
+  virtual uint64_t TscToNanos(uint64_t tsc) = 0;
+  virtual llvm::json::Value toJSON() = 0;

ToNanos() is also okay. Up to you



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:63
+/// Conversion based on values define in perf_event_mmap_page.
+struct PerfTscConverter : TscConverter {
+  PerfTscConverter() = default;





Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:64-66
+  PerfTscConverter() = default;
+  PerfTscConverter(uint32_t time_mult, uint16_t time_shift, uint64_t 
time_zero) :
+m_perf_conversion{time_mult, time_shift, static_cast(time_zero)} 
{}

you can get rid of the constructors a



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:70
+
+  PerfTscConversionValues m_perf_conversion;
+};

you don't need this struct. You should just put the 3 fields here.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:86
+  /// `nullptr` if no TscConverter exists.
+  std::shared_ptr tsc_converter;
+};

it's weird for a bag of data to contain a field transmitted by json called 
coverter. Just call it TimestampCounterRate. Because that's what it is. Then 
each TimestampCounterRate subclass can have a ToNanos(uint64_t tsc) method that 
converts to that.
Your documentation should specific if this ToNanos method returns the since the 
start of the computer, or since the start of the program, or anything else



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:89
+
+bool fromJSON(const llvm::json::Value &value, std::shared_ptr 
&tsc_converter, llvm::json::Path path);
+

Create a typedef in this file called TimestampCounterRateSP for readability. 
You can use tsc_rate as your variable names



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:45
 
+std::shared_ptr IntelPTManager::tsc_converter = nullptr;
+

remove it from here



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257-259
+  if (m_mmap_meta->cap_user_time_zero) {
+IntelPTManager::tsc_converter = 
std::make_shared(PerfTscConverter(m_mmap_meta->time_mult, 
m_mmap_meta->time_shift, m_mmap_meta->time_zero));
+  }

notice the case is which g_tsc_rate is not None but nullptr. That means that 
the computation was performed but no rate was found, so the computation won't 
happen again. We have to make the code in that way because it's possible that 
checking for some rates is not a no-op. E.g. you can run a little 10ms program 
and calculate the difference in TSC to get an estimated rate.

Besides that, put all of this in a new function



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:238
 
+  static std::shared_ptr tsc_converter;
+

static llvm::Optional g_tsc_rate; // by default this is 
None



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:22
 #include "llvm/ADT/None.h"
+#include 
 

jj10306 wrote:
> 
memory should be defined as a new include block between lines 9 and 11. That's 
just the patt

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

[walter erquinigo via Phabricator via lldb-commits]

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

in terms of functionality, everything is right, but you need to do some 
improvements




Comment at: lldb/docs/lldb-gdb-remote.txt:454-479
+//}],
+//... other parameters specific to the provided tracing type
 //  }
 //
 // NOTES
 //   - "traceThreads" includes all thread traced by both "process tracing" and
 // "thread tracing".

you have to improve the documentation and formatting following the existing 
documentation's format



Comment at: lldb/include/lldb/Target/Trace.h:306
+  /// \param[in] json_string
+  /// String representation of the jLLDBTraceGetState response.
+  ///

JSON string representing the jLLDBTraceGetState response. It might be invalid.



Comment at: lldb/include/lldb/Target/Trace.h:310
+  /// Unique pointer to the packet response, nullptr if response parsing 
failed.
+  virtual std::unique_ptr
+  DoRefreshLiveProcessState(llvm::Expected json_string) = 0;

you don't need to return a unique_ptr for this. There's nothing that you want 
to forbid copies of. Just return

  llvm::Optional





Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:14
 
+
 /// See docs/lldb-gdb-remote.txt for more information.

undo this



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

TSC is not a very well known concept, so better be very explanatory in the 
documentation

  Class that can be extended to represent different CPU Time Stamp Counter 
(TSC) frequency rates, which can be used to convert TSCs to common units of 
time, such as nanoseconds. More on TSCs can be found here 
https://en.wikipedia.org/wiki/Time_Stamp_Counter.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:47
+virtual ~TimestampCounterRate() = default;
+/// Convert TSC value to nanoseconds. This represents the number of 
nanoseconds since
+/// the TSC register's reset.





Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:53
+
+typedef std::shared_ptr TimestampCounterRateSP;
+

modern c++ uses the `using` keyword.

  using TimestampCounterRateSP = std::shared_ptr;

use the version you want



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:55-56
+
+/// TSC to nanoseconds conversion values defined in struct 
perf_event_mmap_page.
+/// See https://man7.org/linux/man-pages/man2/perf_event_open.2.html for more 
information.
+class PerfTimestampCounterRate : public TimestampCounterRate {

You have to make the documentation very friendly to newcomers.




Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:59
+  public:
+  PerfTimestampCounterRate() = default;
+  PerfTimestampCounterRate(uint32_t time_mult, uint16_t time_shift, uint64_t 
time_zero) :

do you need this?



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:61
+  PerfTimestampCounterRate(uint32_t time_mult, uint16_t time_shift, uint64_t 
time_zero) :
+m_time_mult(time_mult), m_time_shift(time_shift), 
m_time_zero(static_cast(time_zero)) {}
+  uint64_t ToNanos(uint64_t tsc) override;

why do you do static_cast?



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:66-68
+  uint32_t m_time_mult;
+  uint16_t m_time_shift;
+  uint64_t m_time_zero;

having the correct types here is the right thing to do



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:72
+/// jLLDBTraceGetState gdb-remote packet
+/// Contains additional information related to TSC -> nanosecond conversion.
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {

The response contains the frequency rate. Whether it's used to convert to nanos 
or not is a different topic.

Just delete this line.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:74
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
+  /// `nullptr` if no tsc conversion rate exists.
+  llvm::Optional tsc_rate;

avoid using nullptr when you have an Optional. 



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

jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > What is the suggested way to serialize a u64 into JSON? I thought of two 
> > > approaches:
> > > 1. Store two separate u32
> > > 2. store the integer as a string
> > > 
> > > @wallace wdyt?
> > you don't need to use int64_t here. You can us

[Lldb-commits] [PATCH] D120755: Fix race condition when launching and attaching.This is a modified version of a previous patch that was reverted: https://reviews.llvm.org/D119797This version only wait

[walter erquinigo via Phabricator via lldb-commits]

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

I'm assuming this passes all tests locally


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120755

___
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

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Thanks @davidca and @labath for chiming in.
@jj10306, please rename IntelPTManager to IntelPTCollector for clarity.




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

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



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

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



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.
Good idea



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:78-80
+bool fromJSON(const llvm::json::Value &value, TimestampCounterRateSP 
&tsc_converter, llvm::json::Path path);
+
+bool fromJSON(const llvm::json::Value &value, TraceIntelPTGetStateResponse 
&packet, llvm::json::Path path);

davidca wrote:
> should this two be static factory methods?
That's just the pattern most json conversion utils follow. It helps with some 
templating stuff



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

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 created.
Can tsc frequency rates change even in modern cpus?. In this case, it's no 
brainer we need to calculate this number all the time.

I also agree with point number 1. 



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:211
   static bool IsSupported();
+  static void GetPerfTscRate(perf_event_mmap_page &mmap_meta);
+

davidca wrote:
> wallace wrote:
> > don't forget to add documentation here
> how is ti
@davidca, i think you didn't finish your comment


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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/docs/lldb-gdb-remote.txt:455
+//}],
+//"tsc_conversion"?: Optional<{
+//  "kind": ,

The ? After the key name makes Optional<> redundant. Just keep the ?



Comment at: lldb/docs/lldb-gdb-remote.txt:457
+//  "kind": ,
+//   Timestamp counter conversion rate kind, e.g. perf.
+//   The kind strings can be found in the overriden toJSON method

perf-linux might be a better name



Comment at: lldb/docs/lldb-gdb-remote.txt:458
+//   Timestamp counter conversion rate kind, e.g. perf.
+//   The kind strings can be found in the overriden toJSON method
+//   of each TimestampCounterRate subclass. The kind determines

All the supported kinds must be present below in the documentation, so no need 
to mention c++ classes here



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;
+

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



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

This class is generic, so better not enforce the meaning of the zero tsc. Each 
trace plugin might be opinionated regarding this timestamp 



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:64
+  m_time_mult(time_mult), m_time_shift(time_shift), m_time_zero(time_zero) 
{}
+uint64_t ToNanos(uint64_t tsc) override;
+llvm::json::Value toJSON() override;

Return a std:: Chrono type. You might want rename this to ToWallTime



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:53
+
+} // namespace resource_handle
+

Nice



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:223
+  /// \a The timestamp counter rate if one exists, \a nullptr if no 
conversion exists.
+  static TimestampCounterRateSP GetTscRate(lldb::pid_t pid);
+

Now that we don't have caching, try to move this method to Process Linux, as an 
static, so that others can use that method more easily. 



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:253
+  /// Server-side cache of timestamp counter rate
+  static llvm::Optional g_tsc_rate;
+

We don't need caching anymore. Just calculate it every time we request the 
state, following the information that David gave that the rate might change 
over time. 


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

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

The main change is to make the counter conversion generic, so that we don't 
need to add more APIs in the future. Besides that, you don't need to make any 
drastic change. Good job so far!




Comment at: lldb/docs/lldb-gdb-remote.txt:455
+//}],
+//"tsc_conversion"?: Optional<{
+//  "kind": ,

wallace wrote:
> wallace wrote:
> > The ? After the key name makes Optional<> redundant. Just keep the ?
> 
let's make this more generic for any kind of counter we might want to include 
in a trace, including ones that could come from PT_WRITE, so let's call it 
`counter_conversion`



Comment at: lldb/docs/lldb-gdb-remote.txt:455-463
+//"tsc_conversion"?: Optional<{
+//  "kind": ,
+//   Timestamp counter conversion rate kind, e.g. perf.
+//   The kind strings can be found in the overriden toJSON method
+//   of each TimestampCounterRate subclass. The kind determines
+//   what fields are available in the remainder of the response.
+//

wallace wrote:
> The ? After the key name makes Optional<> redundant. Just keep the ?




Comment at: lldb/docs/lldb-gdb-remote.txt:476-502
+//  Additional params in the tsc_conversion output schema when kind == "perf":
+//   {
+// "time_mult": ,
+// time_mult field of the Linux perf_event_mmap_page struct available 
when
+// cap_usr_time is set. Used in TSC to nanosecond conversion 
calculation defined
+// by the Linux perf_event API.
+//

someone who wants to know more about these fields can simply open the 
perf_event documentation, so let's just point to it to reduce the amount of 
text here



Comment at: lldb/include/lldb/Target/Trace.h:312
+  virtual llvm::Optional
+  DoRefreshLiveProcessState(llvm::Expected json_string) = 0;
 

`json_string` is not a good name. Let's call it `json_response`



Comment at: lldb/include/lldb/Target/TraceCursor.h:183-190
   /// Get the timestamp counter associated with the current instruction.
   /// Modern Intel, ARM and AMD processors support this counter. However, a
   /// trace plugin might decide to use a different time unit instead of an
   /// actual TSC.
   ///
   /// \return
   /// The timestamp or \b llvm::None if not available.

refactor this into a new function generic for all possible counters

you will need to create a new enum in `lldb-enumerations.h`

enum TraceCounter {
  // Timestamp counter (TSC), like the one offered by Intel CPUs.
  eTraceCounterTSC,
};




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;
+

jj10306 wrote:
> 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.
just replied above about having a generic method for all kinds of counter. 
Regarding this one, let's refactor it into

  // Get the timestamp associated with the current instruction.
  //
  // Each trace plug-in might use a different 0 timestamp, e.g. time since last 
clock reset, time since the program started, etc.
  // Also, it might happen that only some instructions of the entire trace have 
a timestamp associated with them.
  //
  //   \return
  //  The timestamp associated with the current instruction, or llvm::None 
if not available.
  virtual llvm::Optional GetTimestamp() = 0;



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

jj10306 wrote:
> 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?
I imagine that with generic counter conversions we can't have a simple class 
that works for everything, but we might be able to do this:

  template
  class TraceCounterConversion {
virtual ~TraceCounterConversion() = default;
virtual ToType ToNanos(uint64_t raw_counter) = 0;
virtual llvm::json::Value toJSON() = 0;
  }

also move it to TraceGDBRemotePackets as you mentioned.

Then the

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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/docs/lldb-gdb-remote.txt:455
+//}],
+//"tsc_conversion"?: Optional<{
+//  "kind": ,

wallace wrote:
> wallace wrote:
> > wallace wrote:
> > > The ? After the key name makes Optional<> redundant. Just keep the ?
> > 
> let's make this more generic for any kind of counter we might want to include 
> in a trace, including ones that could come from PT_WRITE, so let's call it 
> `counter_conversion`
After some more thoughts, let's just call this field 'counters'. And we can put 
any information related to any kind of counters.

So, we have something like

"counters"?: [{
  ... The tsc conversion object
}]



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] D128484: [NFC][lldb][trace] Rename trace session to trace bundle

[walter erquinigo via Phabricator via lldb-commits]

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

As previously discussed with @jj10306, we didn't really have a name for
the post-mortem (or offline) trace session representation, which is in
fact a folder with a bunch of files. We decided to call this folder
"trace bundle", and the main JSON file in it "trace bundle description
file". This naming is pretty decent, so I'm refactoring all the existing
code to account for that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128484

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Core/PluginManager.cpp
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  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/TraceIntelPTBundleLoader.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.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/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.h
  lldb/source/Target/Trace.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
@@ -98,7 +98,7 @@
 
 # We test first an invalid type
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad.json")
-expected_substrs = ['''error: expected object at traceSession.processes[0]
+expected_substrs = ['''error: expected object at traceBundle.processes[0]
 
 Context:
 {
@@ -124,15 +124,15 @@
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 
 
-# Now we test a wrong cpu family field in the global session file
+# Now we test a wrong cpu family field in the global bundle description file
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad2.json")
-expected_substrs = ['error: expected uint64_t at traceSession.cpuInfo.family', "Context", "Schema"]
+expected_substrs = ['error: expected uint64_t at traceBundle.cpuInfo.family', "Context", "Schema"]
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 
 
 # Now we test a missing field in the intel-pt settings
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad4.json")
-expected_substrs = ['''error: missing value at traceSession.cpuInfo.family
+expected_substrs = ['''error: missing value at traceBundle.cpuInfo.family
 
 Context:
 {
@@ -149,7 +149,7 @@
 
 # Now we test an incorrect load address in the intel-pt settings
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad5.json")
-expected_substrs = ['error: missing value at traceSession.processes[1].pid', "Schema"]
+expected_substrs = ['error: missing value at traceBundle.processes[1].pid', "Schema"]
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 
 
@@ -157,6 +157,6 @@
 # no targets should be created.
 self.assertEqual(self.dbg.GetNumTargets(), 0)
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad3.json")
-expected_substrs = ['error: missing value at traceSession.processes[1].pid']
+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)
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -24,19 +24,20 @@
 using namespace lldb_private;
 using namespace llvm;
 
-// Helper structs used to extract the type of a trace session json without
-// having to parse the entire object.
+//

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

[walter erquinigo via Phabricator via lldb-commits]

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

@jingham, that's a pretty good idea. We'll see how to implement it


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] D128484: [NFC][lldb][trace] Rename trace session to trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



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

jj10306 wrote:
> nit:  `FromBundle` makes more sense than `ForBundle` imo
makes sense! will do


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] D128543: [trace] Improve the TraceCursor iteration API

[walter erquinigo via Phabricator via lldb-commits]

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

The current way ot traversing the cursor is a bit uncommon and it can't handle 
empty traces, in fact, its invariant is that it shold always point to a valid 
item. This diff simplifies the cursor API and allows it to point to invalid 
items, thus being able to handle empty traces or to know it ran out of data.

- Removed all the granularity functionalities, because we are not actually 
making use of that. We can bring them back when they are actually needed.
- change the looping logic to the following:

  for (; cursor->HasValue(); cursor->Next()) {
 if (cursor->IsError()) {
   .. do something for error
   continue;
 }
 .. do something for instruction
  }



- added a HasValue method that can be used to identify if the cursor ran out of 
data, the trace is empty, or the user tried to move to an invalid position via 
SetId() or Seek()
- made several simplifications to severals parts of the code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128543

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  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/TraceCursor.cpp
  lldb/source/Target/TraceInstructionDumper.cpp

Index: lldb/source/Target/TraceInstructionDumper.cpp
===
--- lldb/source/Target/TraceInstructionDumper.cpp
+++ lldb/source/Target/TraceInstructionDumper.cpp
@@ -261,33 +261,20 @@
 : m_cursor_up(std::move(cursor_up)), m_options(options),
   m_writer_up(CreateWriter(s, m_options)) {
 
-  if (m_options.id) {
-if (!m_cursor_up->GoToId(*m_options.id)) {
-  m_writer_up->InfoMessage("invalid instruction id");
-  SetNoMoreData();
-}
-  } else if (m_options.forwards) {
+  if (m_options.id)
+m_cursor_up->GoToId(*m_options.id);
+  else if (m_options.forwards)
 m_cursor_up->Seek(0, TraceCursor::SeekType::Beginning);
-  } else {
+  else
 m_cursor_up->Seek(0, TraceCursor::SeekType::End);
-  }
 
   m_cursor_up->SetForwards(m_options.forwards);
   if (m_options.skip) {
-uint64_t to_skip = *m_options.skip;
-if (m_cursor_up->Seek((m_options.forwards ? 1 : -1) * to_skip,
-  TraceCursor::SeekType::Current) < to_skip) {
-  // This happens when the skip value was more than the number of
-  // available instructions.
-  SetNoMoreData();
-}
+m_cursor_up->Seek((m_options.forwards ? 1 : -1) * *m_options.skip,
+  TraceCursor::SeekType::Current);
   }
 }
 
-void TraceInstructionDumper::SetNoMoreData() { m_no_more_data = true; }
-
-bool TraceInstructionDumper::HasMoreData() { return !m_no_more_data; }
-
 TraceInstructionDumper::InstructionEntry
 TraceInstructionDumper::CreatRawInstructionEntry() {
   InstructionEntry insn;
@@ -375,11 +362,8 @@
   ExecutionContext exe_ctx;
   thread_sp->GetProcess()->GetTarget().CalculateExecutionContext(exe_ctx);
 
-  for (size_t i = 0; i < count; i++) {
-if (!HasMoreData()) {
-  m_writer_up->InfoMessage("no more data");
-  break;
-}
+  for (size_t i = 0; i < count && m_cursor_up->HasValue();
+   m_cursor_up->Next(), i++) {
 last_id = m_cursor_up->GetId();
 
 if (m_options.forwards) {
@@ -418,9 +402,8 @@
   // makes sense.
   PrintEvents();
 }
-
-if (!m_cursor_up->Next())
-  SetNoMoreData();
   }
+  if (!m_cursor_up->HasValue())
+m_writer_up->InfoMessage("no more data");
   return last_id;
 }
Index: lldb/source/Target/TraceCursor.cpp
===
--- lldb/source/Target/TraceCursor.cpp
+++ lldb/source/Target/TraceCursor.cpp
@@ -21,15 +21,6 @@
   return m_exe_ctx_ref;
 }
 
-void TraceCursor::SetGranularity(
-lldb::TraceInstructionControlFlowType granularity) {
-  m_granularity = granularity;
-}
-
-void TraceCursor::SetIgnoreErrors(bool ignore_errors) {
-  m_ignore_errors = ignore_errors;
-}
-
 void TraceCursor::SetForwards(bool forwards) { m_forwards = forwards; }
 
 bool TraceCursor::IsForwards() const { return m_forwards; }
Index: lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
===
--- lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
+++ lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
@@ -154,7 +154,8 @@
   // TODO: Make distinction between errors by storing the error messages.
   // Currently, all errors are treated the same.
   m_instruction_layer_up->AppendInstruction(0);
- 

[Lldb-commits] [PATCH] D128707: [lldb] Fix build on older Linux kernel versions

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.
Herald added a subscriber: JDevlieghere.

thanks a good point, Jakob. Let's keep it in mind. Maybe we can bootcamp that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128707

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

[Lldb-commits] [PATCH] D128707: [lldb] Fix build on older Linux kernel versions

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

@kongyi thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128707

___
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

[walter erquinigo via Phabricator via lldb-commits]

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

remove unwanted files


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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



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

jj10306 wrote:
> 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?
i think that's a very extreme case =P



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

jj10306 wrote:
> why is this new scope introduced here?
i'll remove it


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] D128477: [trace] Add a flag to the decoder to output the instruction type

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Core/Disassembler.h:82-86
+  lldb::InstructionControlFlowType
+  instruction_decode(uint8_t opcode, uint8_t modrm, uint8_t map);
+
+  lldb::InstructionControlFlowType
+  GetControlFlowInstructionKind(const ExecutionContext *exe_ctx);

as this code is new, please write documentation and function names should be in 
PascalCase. Try to have a more descriptive name as well, because 
`instruction_decode` is very vague. 



Comment at: lldb/include/lldb/Core/Disassembler.h:152
   virtual void Dump(Stream *s, uint32_t max_opcode_byte_size, bool 
show_address,
-bool show_bytes, const ExecutionContext *exe_ctx,
+bool show_bytes, bool show_kind,
+const ExecutionContext *exe_ctx,

let's try to be more explicit with the naming



Comment at: lldb/include/lldb/Core/Disassembler.h:226-233
+  enum {
+PTI_MAP_0,
+PTI_MAP_1,
+PTI_MAP_2,
+PTI_MAP_3,
+PTI_MAP_AMD3DNOW,
+PTI_MAP_INVALID

add documentation. Also, if this mapping is intel-specific, I recommend moving 
this to the cpp file so that it's not exposed publicly. The Disassembler is 
architecture agnostic, so its API should try to remain that way



Comment at: lldb/include/lldb/Core/Disassembler.h:341
 
-  void Dump(Stream *s, bool show_address, bool show_bytes,
+  void Dump(Stream *s, bool show_address, bool show_bytes, bool show_kind,
 const ExecutionContext *exe_ctx);

name



Comment at: lldb/include/lldb/Core/Disassembler.h:398
+(1u << 3), // Mark the disassembly line the contains the PC
+eOptionShowKind = (1u << 4),
   };





Comment at: lldb/include/lldb/Target/TraceCursor.h:35
 ///  point at them. The consumer should invoke \a TraceCursor::GetError() to
 ///  check if the cursor is pointing to either a valid instruction or an error.
 ///

this file has changed, could you rebase from upstream/main. 



Comment at: lldb/include/lldb/Target/TraceCursor.h:230-234
   /// \return
-  /// The \a lldb::TraceInstructionControlFlowType categories the
+  /// The \a lldb::InstructionControlFlowType categories the
   /// instruction the cursor is pointing at falls into. If the cursor 
points
   /// to an error in the trace, return \b 0.
+  virtual lldb::InstructionControlFlowType GetInstructionControlFlowType() = 0;

when you rebase, could you delete any references to this method and its 
implementation as well? We won't use it anymore



Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:38
+  /// For each instruction, print the instruction type.
+  bool show_kind = false;
   /// Optional custom id to start traversing from.

name



Comment at: lldb/include/lldb/lldb-enumerations.h:973
 /// A single instruction can match one or more of these categories.
-FLAGS_ENUM(TraceInstructionControlFlowType){
-/// Any instruction.
-eTraceInstructionControlFlowTypeInstruction = (1u << 1),
-/// A conditional or unconditional branch/jump.
-eTraceInstructionControlFlowTypeBranch = (1u << 2),
-/// A conditional or unconditional branch/jump that changed
-/// the control flow of the program.
-eTraceInstructionControlFlowTypeTakenBranch = (1u << 3),
-/// A call to a function.
-eTraceInstructionControlFlowTypeCall = (1u << 4),
-/// A return from a function.
-eTraceInstructionControlFlowTypeReturn = (1u << 5)};
-
-LLDB_MARK_AS_BITMASK_ENUM(TraceInstructionControlFlowType)
+FLAGS_ENUM(InstructionControlFlowType){
+/// The instruction could not be classified.

this doesn't need to be a FLAGS_ENUM anymore (i.e. bitmask), you can use a 
simple enum instead, like the `ExpressionEvaluationPhase` above



Comment at: lldb/source/API/SBInstruction.cpp:244
 FormatEntity::Parse("${addr}: ", format);
-inst_sp->Dump(&s.ref(), 0, true, false, nullptr, &sc, nullptr, &format, 0);
+inst_sp->Dump(&s.ref(), 0, true, false, false, nullptr, &sc, nullptr,
+  &format, 0);

include this kind of comment wherever you have added this parameter



Comment at: lldb/source/Commands/CommandObjectDisassemble.h:49
 bool show_bytes;
+bool show_kind;
 uint32_t num_lines_context = 0;

ame


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] D128477: [trace] Add a flag to the decoder to output the instruction type

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

nice job!!




Comment at: lldb/source/Commands/Options.td:304
+  def disassemble_options_kind : Option<"kind", "k">,
+Desc<"Show instruction control flow type.">;
   def disassemble_options_context : Option<"context", "C">, Arg<"NumLines">,

mention here the enum to inspect for documentation. Besides that, you can 
quickly mention here that far jumps and far returns often indicate a kernel 
calls and returns. That might be enough because the other categories are pretty 
intuitive



Comment at: lldb/source/Commands/Options.td:1156
+  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>,

same here



Comment at: lldb/source/Core/Disassembler.cpp:680
+Instruction::GetControlFlowInstructionKind(const ExecutionContext *exe_ctx) {
+  uint8_t *opcode_bytes = (uint8_t *)m_opcode.GetOpcodeBytes();
+  uint8_t opcode, modrm, map;

you have to check here that the triple of the current architecture of the 
current target (you can get that from exe_ctx) is x86, otherwise directly 
return eInstructionControlFlowTypeUnknown. You can just rely on the check you 
are doing in line 685 because that check might fail in the future



Comment at: lldb/source/Core/Disassembler.cpp:681
+  uint8_t *opcode_bytes = (uint8_t *)m_opcode.GetOpcodeBytes();
+  uint8_t opcode, modrm, map;
+  int op_idx = 0;

from this point on, the code is specifically meant for x86, so I recommend 
creating a x86 namespace in this file and include a 
GetControlFlowInstructionKind method along with the instruction_decode method 
there. That way we organize the code in a more cleaner manner



Comment at: lldb/source/Core/Disassembler.cpp:681
+  uint8_t *opcode_bytes = (uint8_t *)m_opcode.GetOpcodeBytes();
+  uint8_t opcode, modrm, map;
+  int op_idx = 0;

wallace wrote:
> from this point on, the code is specifically meant for x86, so I recommend 
> creating a x86 namespace in this file and include a 
> GetControlFlowInstructionKind method along with the instruction_decode method 
> there. That way we organize the code in a more cleaner manner
add a comment explaining what these variables are



Comment at: lldb/source/Core/Disassembler.cpp:692
+  // Get opcode and modrm
+  map = PTI_MAP_INVALID;
+  while (!prefix_done) {

add a comment explaining what this prefix handling is actually doing



Comment at: lldb/source/Core/Disassembler.cpp:715
+case 0x40 ... 0x4f:
+  if (exe_ctx->GetTargetRef().GetArchitecture().GetMachine() ==
+  llvm::Triple::ArchType::x86_64)

you can move this to the beginning of the function and assume that this mapping 
is x86 only, which is how it should be



Comment at: lldb/source/Core/Disassembler.cpp:773
+
+  if (opcode == 0x0F) {
+if (opcode_bytes[op_idx + 1] == 0x38) {

you can also mention here how this mapping is actually generated and were you 
got inspiration from, like libipt



Comment at: lldb/source/Core/Disassembler.cpp:846
+  if (show_kind) {
+// TODO-SUJIN
+switch (GetControlFlowInstructionKind(exe_ctx)) {

should you remove this?



Comment at: lldb/source/Core/Disassembler.cpp:849
+case eInstructionControlFlowTypeUnknown:
+  ss.Printf("%-12s", "unknwon");
+  break;

unknown



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:63
+InstructionControlFlowType
 DecodedThread::GetInstructionControlFlowType(size_t insn_index) const {
   if (IsInstructionAnError(insn_index))

remove this, because we won't use it anymore


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] D128775: [trace] Fix errors when handling command arguments

[walter erquinigo via Phabricator via lldb-commits]

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

https://reviews.llvm.org/D128453 recently added some safety checks for
command arguments. Unfortunately, some few commands started failing due
to that, and this diff fixes it. But fortunately, the fix is trivial, which is
simply declaring the argument that these commands will receive.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128775

Files:
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/CommandObjectThreadUtil.cpp
  lldb/source/Commands/CommandObjectThreadUtil.h


Index: lldb/source/Commands/CommandObjectThreadUtil.h
===
--- lldb/source/Commands/CommandObjectThreadUtil.h
+++ lldb/source/Commands/CommandObjectThreadUtil.h
@@ -80,7 +80,9 @@
 /// an action on multiple threads at once instead of iterating over each 
thread.
 class CommandObjectMultipleThreads : public CommandObjectParsed {
 public:
-  using CommandObjectParsed::CommandObjectParsed;
+  CommandObjectMultipleThreads(CommandInterpreter &interpreter,
+   const char *name, const char *help,
+   const char *syntax, uint32_t flags);
 
   bool DoExecute(Args &command, CommandReturnObject &result) override;
 
Index: lldb/source/Commands/CommandObjectThreadUtil.cpp
===
--- lldb/source/Commands/CommandObjectThreadUtil.cpp
+++ lldb/source/Commands/CommandObjectThreadUtil.cpp
@@ -25,6 +25,15 @@
   m_arguments.push_back({thread_arg});
 }
 
+CommandObjectMultipleThreads::CommandObjectMultipleThreads(
+CommandInterpreter &interpreter, const char *name, const char *help,
+const char *syntax, uint32_t flags)
+: CommandObjectParsed(interpreter, name, help, syntax, flags) {
+  // These commands all take thread ID's as arguments.
+  CommandArgumentData thread_arg{eArgTypeThreadIndex, eArgRepeatStar};
+  m_arguments.push_back({thread_arg});
+}
+
 bool CommandObjectIterateOverThreads::DoExecute(Args &command,
 CommandReturnObject &result) {
   result.SetStatus(m_success_return);
Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -2219,7 +2219,10 @@
 nullptr,
 eCommandRequiresProcess | eCommandRequiresThread |
 eCommandTryTargetAPILock | eCommandProcessMustBeLaunched |
-eCommandProcessMustBePaused | eCommandProcessMustBeTraced) {}
+eCommandProcessMustBePaused | eCommandProcessMustBeTraced) {
+CommandArgumentData thread_arg{eArgTypeThreadIndex, eArgRepeatOptional};
+m_arguments.push_back({thread_arg});
+  }
 
   ~CommandObjectTraceDumpInstructions() override = default;
 


Index: lldb/source/Commands/CommandObjectThreadUtil.h
===
--- lldb/source/Commands/CommandObjectThreadUtil.h
+++ lldb/source/Commands/CommandObjectThreadUtil.h
@@ -80,7 +80,9 @@
 /// an action on multiple threads at once instead of iterating over each thread.
 class CommandObjectMultipleThreads : public CommandObjectParsed {
 public:
-  using CommandObjectParsed::CommandObjectParsed;
+  CommandObjectMultipleThreads(CommandInterpreter &interpreter,
+   const char *name, const char *help,
+   const char *syntax, uint32_t flags);
 
   bool DoExecute(Args &command, CommandReturnObject &result) override;
 
Index: lldb/source/Commands/CommandObjectThreadUtil.cpp
===
--- lldb/source/Commands/CommandObjectThreadUtil.cpp
+++ lldb/source/Commands/CommandObjectThreadUtil.cpp
@@ -25,6 +25,15 @@
   m_arguments.push_back({thread_arg});
 }
 
+CommandObjectMultipleThreads::CommandObjectMultipleThreads(
+CommandInterpreter &interpreter, const char *name, const char *help,
+const char *syntax, uint32_t flags)
+: CommandObjectParsed(interpreter, name, help, syntax, flags) {
+  // These commands all take thread ID's as arguments.
+  CommandArgumentData thread_arg{eArgTypeThreadIndex, eArgRepeatStar};
+  m_arguments.push_back({thread_arg});
+}
+
 bool CommandObjectIterateOverThreads::DoExecute(Args &command,
 CommandReturnObject &result) {
   result.SetStatus(m_success_return);
Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -2219,7 +2219,10 @@
 nullp

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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



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;

jj10306 wrote:
> 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?
It seems that we can use unique_ptrs! SBMemoryRegionInfoList is an example of a 
SB wrapper of a unique_ptr. It seems that python reuses the same reference of 
the SB object with all the variables that use it. On the other hand, if you use 
c++, the unique_ptr would be enforced strictly



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.

jj10306 wrote:
> do you want to include pointers to the methods you can use to check/get 
> events similar to what you did for errors above?
good idea!



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

jj10306 wrote:
> 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?
check the new version of the code. I'm now using a tagged union for the low 
level storage, and then using an enum for the item kinds in the TraceCursor. I 
think this version introduces more safety and it's easier to handle the low 
level storage, not to mention that we now use less memory!



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

jj10306 wrote:
> 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?
I really like your idea, but that's not that trivial, so let's try to come up 
with a nice design. What about the following?

- We create a new event `TraceEventHWClockTick`. This will help use identify 
quickly anytime we see a different TSC. That way we don't need to keep asking 
every instruction for its TSC even when it doesn't change. The new name also 
makes it more generic.
- In order to access the TSC value, the user could invoke `uint64_t 
GetEventValueHWClockTick()`. I'd imagine that each event type would have its 
own data accessor. 
- We create another method `optional GetEventValueLastHWClockTick()`, 
which returns the most recent value for this event type that happened on or 
before the cursor's current point. I wouldn't make this kind of functionality 
generic for all event kinds because for some it might not make sense at all. 
Anyway, the reason is the following code

In the dumper, we would like to support this flow:
   cursor->GoTo(starting_id)
   hw_timestamp = cursor->GetEventValueLastHWClockTick();

   for (; cursor->HasValue(); cursor->Next()) {
 if (cursor->GetItemKind() == eTraceItemKindEvent && cursor->GetEventKind() 
== eTraceEventHWClockTick)
   hw_timestamp = cursor->GetEventValueHWClockTick();

 cout << "current clock (tsc) " << hw_timestamp << endl;
  }

so, basically we need a way to know the starting tsc for a given instruction 
id, and then to identify changes.

Later, we can implement a new method `nanoseconds GetEstimat

[Lldb-commits] [PATCH] D128775: [trace] Fix errors when handling command arguments

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

what happens is that the specific failures are triggered by tests that require 
the cmake flag LIBIPT_INCLUDE_PATH to be configured, but all good


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128775

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

[Lldb-commits] [PATCH] D128874: [lldb] Fix unused variable warning in TraceHTR (NFC)

[walter erquinigo via Phabricator via lldb-commits]

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

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128874

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

[Lldb-commits] [PATCH] D128874: [lldb] Fix unused variable warning in TraceHTR (NFC)

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

i can do it right now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128874

___
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

[walter erquinigo via Phabricator via lldb-commits]

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

much better! Thanks for doing this.

There are two main things to do.

1. The algorithm you are using to classify the instructions uses too many 
acronyms and it's very well documented. Try to make it understandable enough so 
that anyone with little familiarity can have an idea of what is happening
2. write a unit test. For this, you can follow any of the tests in 
`lldb/unittests/Disassembler/`. In order to run it, you'll need to do `ninja 
DisassemblerTests` and then run the output path printed by that command. It 
uses gtest. You can see the tests in that folder for inspiration.




Comment at: lldb/include/lldb/Core/Disassembler.h:82
 
+  /// Return InstructionControlFlowType of this instruction.
+  lldb::InstructionControlFlowType

better this



Comment at: lldb/include/lldb/Core/Disassembler.h:84
+  lldb::InstructionControlFlowType
+  GetControlFlowInstructionKind(const ExecutionContext *exe_ctx);
+

Rename this to
  GetControlFlowKind
because we are already inside the Instruction 



Comment at: lldb/include/lldb/lldb-enumerations.h:976
+  eInstructionControlFlowTypeUnknown = 0,
+  /// The instruction is something not listed below.
+  eInstructionControlFlowTypeOther,





Comment at: lldb/source/API/SBInstruction.cpp:244
 FormatEntity::Parse("${addr}: ", format);
-inst_sp->Dump(&s.ref(), 0, true, false, nullptr, &sc, nullptr, &format, 0);
+inst_sp->Dump(&s.ref(), 0, true, false, /*show_control_flow_type*/ false,
+  nullptr, &sc, nullptr, &format, 0);

the llvm style requires a =



Comment at: lldb/source/API/SBInstruction.cpp:279
 FormatEntity::Parse("${addr}: ", format);
-inst_sp->Dump(&out_stream, 0, true, false, nullptr, &sc, nullptr, &format,
-  0);
+inst_sp->Dump(&out_stream, 0, true, false, /*show_control_flow_type*/ 
false,
+  nullptr, &sc, nullptr, &format, 0);

same here



Comment at: lldb/source/API/SBInstructionList.cpp:169
+inst->Dump(&sref, max_opcode_byte_size, true, false,
+   /*show_control_flow_type*/ false, nullptr, &sc, &prev_sc,
+   &format, 0);

ditto



Comment at: lldb/source/Commands/Options.td:306
+"`InstructionControlFlowType` for a list of control flow kind. "
+"far jump and far return often indicate syscall and return.">;
   def disassemble_options_context : Option<"context", "C">, Arg<"NumLines">,





Comment at: lldb/source/Commands/Options.td:1160
+"`InstructionControlFlowType` enum has a list of control flow kind. "
+"far jump and far return often indicate syscall and return.">;
   def thread_trace_dump_instructions_show_tsc : Option<"tsc", "t">, Group<1>,

copy the changes from above



Comment at: lldb/source/Core/Disassembler.cpp:575
+namespace x86 {
+enum PTI_MAP {
+  PTI_MAP_0,/* 1-byte opcodes.   may have modrm */

add documentation mentioning what a PTI_MAP is. Try to make the documentation 
clear enough for anyone without knowledge of this matter. Also, mention what 
modrm is why it's important here.
Also, it's better if don't use the PTI acronym but instead use the full name. 
It'd make it easier to read.
Also, this is an enum and not a map, so better use an identifier different than 
a map



Comment at: lldb/source/Core/Disassembler.cpp:584-586
+/// Decide InstructionControlFlowType based on bytes of instruction.
+/// The insruction bytes are parsed beforehand into opcode, modrm and map byte
+/// to be passed as parameters.

let's be more specific



Comment at: lldb/source/Core/Disassembler.cpp:695-696
+  Opcode m_opcode) {
+  // inst_bytes will be parsed into opcode, modrm and map bytes.
+  // These are the three values deciding instruction control flow type.
+  uint8_t inst_bytes[16];

another reference to opcode, modrm, and map bytes. I recommend writing 
extensive documentation in PTI_MAP and then mention here that the full 
documentation can be found in the PTI_MAP definition



Comment at: lldb/source/Core/Disassembler.cpp:703-704
+  if (m_opcode.GetOpcodeBytes() == nullptr || m_opcode.GetByteSize() <= 0) {
+// x86_64 and i386 are the only ones that use bytes right now
+// Else, we have ARM or MIPS, not yet implemented
+return lldb::eInstructionControlFlowTypeUnknown;

this comment is not useful because this method is in the x86 namespace



Comment at: lldb/source/Core/Disassembler.cpp:710-713
+  // In most cases, opcode is the first byte of instruction (a

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

[walter erquinigo via Phabricator via lldb-commits]

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

let's try to have tests soon. It seems that the code can be simplified and 
tests will be very handy




Comment at: lldb/include/lldb/lldb-enumerations.h:976
+  eInstructionControlFlowKindUnknown = 0,
+  /// The instruction is something not listed below, i.e. it's sequential
+  /// instruction that doesn't affect the control flow of the program.





Comment at: lldb/source/Core/Disassembler.cpp:730-740
+/// \param[out] opcode
+///Primary opcode of the instruction.
+///
+/// \param[out] modrm
+///ModR/M byte of the instruction.
+///Bit[7:6] indicates MOD. Bit[5:3] specifies a register and R/M bit[2:0]
+///may contain a register or specify an addressing mode, depending on MOD.

I'm noticing that MapOpcodeIntoControlFlowKind only uses PTI_MAP_0 and 
PTI_MAP_1, which are opcodes of 1 and 2 bytes. Any opcode of 3 bytes and even 
amd3dnow is not used at all. That means that you don't need that enum, so 
please delete it. Instead, use the length of the opcode throughout the code.




Comment at: lldb/source/Core/Disassembler.cpp:882
+
+  if (m_opcode.GetOpcodeBytes() == nullptr || m_opcode.GetByteSize() <= 0) {
+return lldb::eInstructionControlFlowKindUnknown;

does this mean that all x86 and x86_64 instructions are categorized as 
Opcode::Type::eTypeBytes?
I'm asking because after reading GetOpcodeBytes, it only returns data if the 
type is eTypeBytes. Did you check that?



Comment at: lldb/source/Core/Disassembler.cpp:885
+  }
+  memcpy(inst_bytes, m_opcode.GetOpcodeBytes(), m_opcode.GetByteSize());
+

you don't need to copy GetOpcodeBytes, you can just pass it directly to 
InstructionLengthDecode


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] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

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

A trace bundle contains many trace files, and, in the case of intel pt, the
largest files are often the context switch traces because they are not
compressed by default. As a way to improve this, I'm adding a --compact option
to the `trace save` command that filters out unwanted processes from the
context switch traces. Eventually we can do the same for intel pt traces as
well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
   /// \a llvm::success if the operation was successful, or an \a llvm::Error
   /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory,
+ bool compact);
 };
 
 } // namespace trace_intel_pt
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cp

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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Commands/CommandObjectProcess.cpp:582
   }
-  
+
   Target *target = m_exe_ctx.GetTargetPtr();

many formatting changes sneaked in. The actual changes are in the end of this 
file



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp:241
+if (std::error_code ec =
+llvm::sys::fs::copy_file(file, path_to_copy_module.GetPath()))
   return createStringError(

this was a bug in the existing code that prevented copying a module that was 
loaded from a bundle


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] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

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

minor improvements for autocompletion in the CLI


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
-  /// \a llvm::success if the operation was successful, or an \a llvm::Error
-  /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  ///   A \a FileSpec pointing to the bundle description file, or an \a
+  ///   llvm::Error otherwise.
+  llvm::Expected SaveToDisk(TraceIntelPT &trace_ipt,
+  FileSpec directory, bool compact);
 };
 
 } // namespace trace_intel_pt
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
@@ -7,8 +7,11 @@
 //===--===//
 
 #include "TraceIntelPTBundleSaver.h"
+
+#include "PerfContextSwitchDecoder.h"

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

[walter erquinigo via Phabricator via lldb-commits]

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

make relative all paths being returned in the description file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
-  /// \a llvm::success if the operation was successful, or an \a llvm::Error
-  /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  ///   A \a FileSpec pointing to the bundle description file, or an \a
+  ///   llvm::Error otherwise.
+  llvm::Expected SaveToDisk(TraceIntelPT &trace_ipt,
+  FileSpec directory, bool compact);
 };
 
 } // namespace trace_intel_pt
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
@@ -7,8 +7,11 @@
 //===--===//
 
 #include "TraceIntelPTBundleSaver.h"
+
+#include "PerfContextSw

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

[walter erquinigo via Phabricator via lldb-commits]

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

improve command handling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  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
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace dump instructions 2 -t",
+  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+   "m.out`foo() + 65 at multi_thread.cpp:12:21",
+   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+self.expect("thread trace dump instructions 3 -t",
+  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+   "m.out`bar() + 26 at multi_thread.cpp:20:6"])
+
+# This reduced the number of continuous executions to look at
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 3"])
+
+# We clean up for the next run of this test
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
@@ -31,10 +31,16 @@
   /// \param[in] directory
   /// The directory where the trace bundle will be created.
   ///
+  /// \param[in] compact
+  /// Filter out information irrelevant to the traced processes in the
+  /// context switch and intel pt traces when using per-cpu mode. This
+  /// effectively reduces the size of those traces.
+  ///
   /// \return
-  /// \a llvm::success if the operation was successful, or an \a llvm::Error
-  /// otherwise.
-  llvm::Error SaveToDisk(TraceIntelPT &trace_ipt, FileSpec directory);
+  ///   A \a FileSpec pointing to the bundle description file, or an \a
+  ///   llvm::Error otherwise.
+  llvm::Expected SaveToDisk(TraceIntelPT &trace_ipt,
+  FileSpec directory, bool compact);
 };
 
 } // namespace trace_intel_pt
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
@@ -7,8 +7,11 @@
 //===--===//
 
 #include "TraceIntelPTBundleSaver.h"
+
+#include "PerfContextSwitchDecoder.h"
 #include "TraceIntelPT

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

[walter erquinigo via Phabricator via lldb-commits]

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

fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSave.py

Index: lldb/test/API/commands/trace/TestTraceSave.py
===
--- lldb/test/API/commands/trace/TestTraceSave.py
+++ lldb/test/API/commands/trace/TestTraceSave.py
@@ -43,12 +43,12 @@
 self.expect("n")
 
 # Check the output when saving without providing the directory argument
-self.expect("process trace save -d",
-substrs=["error: last option requires an argument"],
+self.expect("process trace save ",
+substrs=["error: a single path to a directory where the trace bundle will be created is required"],
 error=True)
 
 # Check the output when saving to an invalid directory
-self.expect("process trace save -d /",
+self.expect("process trace save /",
 substrs=["error: couldn't write to the file"],
 error=True)
 
@@ -58,7 +58,7 @@
 substrs=["intel-pt"])
 
 # Check the output when not doing live tracing
-self.expect("process trace save -d " +
+self.expect("process trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_not_live_dir"))
 
 def testSaveMultiCpuTrace(self):
@@ -76,7 +76,7 @@
 self.expect("b 7")
 
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
-self.expect("process trace save -d " + output_dir)
+self.expect("process trace save " + output_dir)
 
 def checkSessionBundle(session_file_path):
 with open(session_file_path) as session_file:
@@ -107,7 +107,7 @@
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
 self.expect("trace load " + os.path.join(output_dir, "trace.json"))
 output_copy_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "copy_trace_save")
-self.expect("process trace save -d " + output_copy_dir)
+self.expect("process trace save " + output_copy_dir)
 
 # We now check that the new bundle is correct on its own
 copied_trace_session_file = os.path.join(output_copy_dir, "trace.json")
@@ -153,7 +153,7 @@
 last_ten_instructions = res.GetOutput()
 
 # Now, save the trace to 
-self.expect("process trace save -d " +
+self.expect("process trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_copy_dir"))
 
 # Load the trace just saved
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
+self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
+
+self.expect("thread trace dump info 2", substrs=["Total number of continuous executions found: 153"])
+
+# we'll save the trace in compact format
+compact_trace_bundle_dir = os.path.join(self.getBuildDir(), "intelpt-multi-core-trace-compact")
+self.traceSave(compact_trace_bundle_dir, compact=True)
+
+# we'll delete the previous target and make sure it's trace object is deleted
+self.dbg.DeleteTarget(self.dbg.GetTargetAtIndex(0))
+self.expect("thread trace dump instructions 2 -t", substrs=["error: invalid target"], error=True)
+
+# we'll load the compact trace and make sure it works
+self.traceLoad(os.path.join(compact_trace_bundle_dir, "trace.json"), substrs=["intel-pt"])
+self.expect("thread trace

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

[walter erquinigo via Phabricator via lldb-commits]

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

rename 'process trace save' to 'trace save' because it can actually dump the 
information of multiple processes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129239

Files:
  lldb/bindings/interface/SBTrace.i
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/Target/Trace.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBTrace.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSave.py

Index: lldb/test/API/commands/trace/TestTraceSave.py
===
--- lldb/test/API/commands/trace/TestTraceSave.py
+++ lldb/test/API/commands/trace/TestTraceSave.py
@@ -14,7 +14,7 @@
 
 def testErrorMessages(self):
 # We first check the output when there are no targets
-self.expect("process trace save",
+self.expect("trace save",
 substrs=["error: invalid target, create a target using the 'target create' command"],
 error=True)
 
@@ -22,7 +22,7 @@
 self.expect("target create " +
 os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
 
-self.expect("process trace save",
+self.expect("trace save",
 substrs=["error: Command requires a current process."],
 error=True)
 
@@ -30,7 +30,7 @@
 self.expect("b main")
 self.expect("run")
 
-self.expect("process trace save",
+self.expect("trace save",
 substrs=["error: Process is not being traced"],
 error=True)
 
@@ -43,12 +43,12 @@
 self.expect("n")
 
 # Check the output when saving without providing the directory argument
-self.expect("process trace save -d",
-substrs=["error: last option requires an argument"],
+self.expect("trace save ",
+substrs=["error: a single path to a directory where the trace bundle will be created is required"],
 error=True)
 
 # Check the output when saving to an invalid directory
-self.expect("process trace save -d /",
+self.expect("trace save /",
 substrs=["error: couldn't write to the file"],
 error=True)
 
@@ -58,7 +58,7 @@
 substrs=["intel-pt"])
 
 # Check the output when not doing live tracing
-self.expect("process trace save -d " +
+self.expect("trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_not_live_dir"))
 
 def testSaveMultiCpuTrace(self):
@@ -76,7 +76,7 @@
 self.expect("b 7")
 
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
-self.expect("process trace save -d " + output_dir)
+self.expect("trace save " + output_dir)
 
 def checkSessionBundle(session_file_path):
 with open(session_file_path) as session_file:
@@ -107,7 +107,7 @@
 output_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "trace_save")
 self.expect("trace load " + os.path.join(output_dir, "trace.json"))
 output_copy_dir = os.path.join(self.getBuildDir(), "intelpt-trace", "copy_trace_save")
-self.expect("process trace save -d " + output_copy_dir)
+self.expect("trace save " + output_copy_dir)
 
 # We now check that the new bundle is correct on its own
 copied_trace_session_file = os.path.join(output_copy_dir, "trace.json")
@@ -153,7 +153,7 @@
 last_ten_instructions = res.GetOutput()
 
 # Now, save the trace to 
-self.expect("process trace save -d " +
+self.expect("trace save " +
 os.path.join(self.getBuildDir(), "intelpt-trace", "trace_copy_dir"))
 
 # Load the trace just saved
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -20,6 +20,39 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+@testSBAPIAndCommands
+def testLoadCompactMultiCoreTrace(self):
+src_dir = self.getSourceDir()
+   

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

[walter erquinigo via Phabricator via lldb-commits]

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

This metric was missing. We were only measuring in per-thread mode, and
this completes the work.

For a sample trace I have, the `dump info` command shows

  Timing for this thread:
  Decoding instructions: 0.12s

I also improved a bit the TaskTime function so that callers don't need to
specify the template argument


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129249

Files:
  lldb/source/Plugins/Trace/intel-pt/TaskTimer.h
  lldb/source/Plugins/Trace/intel-pt/ThreadDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
@@ -39,30 +39,35 @@
   if (Error err = CorrelateContextSwitchesAndIntelPtTraces())
 return std::move(err);
 
-  auto it = m_decoded_threads.find(thread.GetID());
-  if (it != m_decoded_threads.end())
-return it->second;
-
-  DecodedThreadSP decoded_thread_sp =
-  std::make_shared(thread.shared_from_this());
-
   TraceIntelPTSP trace_sp = GetTrace();
 
-  Error err = trace_sp->OnAllCpusBinaryDataRead(
-  IntelPTDataKinds::kIptTrace,
-  [&](const DenseMap> &buffers) -> Error {
-auto it = m_continuous_executions_per_thread->find(thread.GetID());
-if (it != m_continuous_executions_per_thread->end())
-  return DecodeSystemWideTraceForThread(*decoded_thread_sp, *trace_sp,
-buffers, it->second);
-
-return Error::success();
+  return trace_sp
+  ->GetThreadTimer(thread.GetID())
+  .TimeTask("Decoding instructions", [&]() -> Expected {
+auto it = m_decoded_threads.find(thread.GetID());
+if (it != m_decoded_threads.end())
+  return it->second;
+
+DecodedThreadSP decoded_thread_sp =
+std::make_shared(thread.shared_from_this());
+
+Error err = trace_sp->OnAllCpusBinaryDataRead(
+IntelPTDataKinds::kIptTrace,
+[&](const DenseMap> &buffers) -> Error {
+  auto it =
+  m_continuous_executions_per_thread->find(thread.GetID());
+  if (it != m_continuous_executions_per_thread->end())
+return DecodeSystemWideTraceForThread(
+*decoded_thread_sp, *trace_sp, buffers, it->second);
+
+  return Error::success();
+});
+if (err)
+  return std::move(err);
+
+m_decoded_threads.try_emplace(thread.GetID(), decoded_thread_sp);
+return decoded_thread_sp;
   });
-  if (err)
-return std::move(err);
-
-  m_decoded_threads.try_emplace(thread.GetID(), decoded_thread_sp);
-  return decoded_thread_sp;
 }
 
 static Expected>
@@ -153,7 +158,7 @@
   if (m_continuous_executions_per_thread)
 return Error::success();
 
-  Error err = GetTrace()->GetTimer().ForGlobal().TimeTask(
+  Error err = GetTrace()->GetGlobalTimer().TimeTask(
   "Context switch and Intel PT traces correlation", [&]() -> Error {
 if (auto correlation = DoCorrelateContextSwitchesAndIntelPtTraces()) {
   m_continuous_executions_per_thread.emplace(std::move(*correlation));
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
@@ -157,6 +157,14 @@
   /// The timer object for this trace.
   TaskTimer &GetTimer();
 
+  /// \return
+  /// The ScopedTaskTimer object for the given thread in this trace.
+  ScopedTaskTimer &GetThreadTimer(lldb::tid_t tid);
+
+  /// \return
+  /// The global copedTaskTimer object for this trace.
+  ScopedTaskTimer &GetGlobalTimer();
+
   TraceIntelPTSP GetSharedPtr();
 
 private:
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -199,10 +199,10 @@
   std::chrono::milliseconds duration) {
   s.Format("{0}: {1:2}s\n", name, duration.count() / 1000.0);
 };
-GetTimer().ForThread(tid).ForEachTimedTask(print_duration);
+GetThreadTimer(tid).ForEachTimedTask(print_duration);
 
 s << "\n  Timing for global tasks:\n";
-GetTimer().ForGlobal().ForEachTimedTask(print_duration);
+GetGlobalTimer().ForEachTimedTask(print_duration);
  

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

[walter erquinigo via Phabricator via lldb-commits]

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

It turns out that cgroup filtering is relatively trivial and works
really nicely. Thid diffs adds automatic cgroup filtering when in
per-cpu mode, unless a new --disable-cgroup-filtering flag is passed in
the start command. At least on Meta machines, all processes are spawned
inside a cgroup by default, which comes super handy, because per cpu
tracing is now much more precise.

A manual test gave me this result

- Without filtering: Total number of trace items: 36083 Total number of 
continuous executions found: 229 Number of continuous executions for this 
thread: 2 Total number of PSB blocks found: 98 Number of PSB blocks for this 
thread 2 Total number of unattributed PSB blocks found: 38

- With filtering: Total number of trace items: 87756 Total number of continuous 
executions found: 123 Number of continuous executions for this thread: 2 Total 
number of PSB blocks found: 10 Number of PSB blocks for this thread 3 Total 
number of unattributed PSB blocks found: 2

Filtering gives us great results. The number of instructions collected
more than double (probalby because we have less noise in the trace), and
we have much less unattributed PSBs blocks and unrelated PSBs in
general. The ones that are unrelated probably belong to other processes
in the same cgroup.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129257

Files:
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp
  lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.h
  lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp
  lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp

Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -53,7 +53,8 @@
 
   if (packet.IsProcessTracing()) {
 if (!o.map("processBufferSizeLimit", packet.process_buffer_size_limit) ||
-!o.map("perCpuTracing", packet.per_cpu_tracing))
+!o.map("perCpuTracing", packet.per_cpu_tracing) ||
+!o.map("disableCgroupTracing", packet.disable_cgroup_filtering))
   return false;
   }
   return true;
@@ -67,6 +68,7 @@
   obj.try_emplace("psbPeriod", packet.psb_period);
   obj.try_emplace("enableTsc", packet.enable_tsc);
   obj.try_emplace("perCpuTracing", packet.per_cpu_tracing);
+  obj.try_emplace("disableCgroupTracing", packet.disable_cgroup_filtering);
   return base;
 }
 
@@ -108,13 +110,15 @@
   json::Path path) {
   ObjectMapper o(value, path);
   return o && fromJSON(value, (TraceGetStateResponse &)packet, path) &&
- o.map("tscPerfZeroConversion", packet.tsc_perf_zero_conversion);
+ o.map("tscPerfZeroConversion", packet.tsc_perf_zero_conversion) &&
+ o.map("usingCgroupFiltering", packet.using_cgroup_filtering);
 }
 
 json::Value toJSON(const TraceIntelPTGetStateResponse &packet) {
   json::Value base = toJSON((const TraceGetStateResponse &)packet);
-  base.getAsObject()->insert(
-  {"tscPerfZeroConversion", packet.tsc_perf_zero_conversion});
+  json::Object &obj = *base.getAsObject();
+  obj.insert({"tscPerfZeroConversion", packet.tsc_perf_zero_conversion});
+  obj.insert({"usingCgroupFiltering", packet.using_cgroup_filtering});
   return base;
 }
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
@@ -83,6 +83,11 @@
  "converted to the approximate number of raw trace bytes between PSB "
  "packets as: 2 ^ (value + 11), e.g. value 3 means 16KiB between PSB "
  "packets. Defaults to 0 if supported.">;
+  def process_trace_start_intel_pt_disable_cgroup_filtering:
+Option<"disable-cgroup-filtering", "d">,
+Desc<"Disable the automatic cgroup filtering that is applied if --per-cpu "
+ "is provided. Cgroup filtering allows collecting intel pt data "
+   

[Lldb-commits] [PATCH] D129332: [trace][intel pt] Support dumping the trace info in json

[walter erquinigo via Phabricator via lldb-commits]

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

Thanks to ym...@fb.com for coming up with this change.

`thread trace dump info` can dump some metrics that can be useful for
analyzing the performance and quality of a trace. This diff adds a --json
option for dumping this information in json format that can be easily
understood my machines.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129332

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
  lldb/test/API/commands/trace/TestTraceDumpInfo.py
  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
@@ -20,6 +20,72 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+self.expect("thread trace dump info --json",
+  substrs=['''{
+  "traceTechnology": "intel-pt",
+  "threadStats": {
+"tid": 3497234,
+"traceItemsCount": 0,
+"memoryUsage": {
+  "totalInBytes": "0",
+  "avgPerItemInBytes": null
+},
+"timingInSeconds": {},
+"events": {
+  "totalCount": 0,
+  "individualCounts": {}
+},
+"continuousExecutions": 0,
+"PSBBlocks": 0,
+"errorItems": {
+  "total": 0,
+  "individualErrors": {}
+}
+  },
+  "globalStats": {
+"timingInSeconds": {
+  "Context switch and Intel PT traces correlation": 0
+},
+"totalUnattributedPSBBlocks": 0,
+"totalCountinuosExecutions": 153,
+"totalPSBBlocks": 5,
+"totalContinuousExecutions": 153
+  }
+}'''])
+
+self.expect("thread trace dump info 2 --json",
+  substrs=['''{
+  "traceTechnology": "intel-pt",
+  "threadStats": {
+"tid": 3497496,
+"traceItemsCount": 19523,
+"memoryUsage": {
+  "totalInBytes": "175739",
+  "avgPerItemInBytes": 9.00''', '''},
+"timingInSeconds": {},
+"events": {
+  "totalCount": 1,
+  "individualCounts": {
+"software disabled tracing": 1
+  }
+},
+"continuousExecutions": 1,
+"PSBBlocks": 1,
+"errorItems": {
+  "total": 0,
+  "individualErrors": {}
+}
+  },
+  "globalStats": {
+"timingInSeconds": {
+  "Context switch and Intel PT traces correlation": 0''', '''},
+"totalUnattributedPSBBlocks": 0,
+"totalCountinuosExecutions": 153,
+"totalPSBBlocks": 5,
+"totalContinuousExecutions": 153
+  }
+}'''])
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
@@ -71,9 +137,10 @@
 # check that the Process and Thread objects were created correctly
 self.expect("thread info", substrs=["tid = 3842849"])
 self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
-self.expect("thread trace dump info", substrs=['''Trace technology: intel-pt
+self.expect("thread trace dump info", substrs=['''thread #1: tid = 3842849
+
+  Trace technology: intel-pt
 
-thread #1: tid = 3842849
   Total number of trace items: 23
 
   Memory usage:
Index: lldb/test/API/commands/trace/TestTraceDumpInfo.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInfo.py
+++ lldb/test/API/commands/trace/TestTraceDumpInfo.py
@@ -34,9 +34,10 @@
 substrs=["intel-pt"])
 
 self.expect("thread trace dump info",
-substrs=['''Trace technology: intel-pt
+substrs=['''thread #1: tid = 3842849
+
+  Trace technology: intel-pt
 
-thread #1: tid = 3842849
   Total number of trace items: 23
 
   Memory usage:
@@ -54,3 +55,37 @@
   Errors:
 Number of TSC decoding errors: 0'''],
 patterns=["Decoding instructions: \d.\d\ds"])
+
+def testDumpRawTraceSizeJSON(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump info --json ",
+substrs=['''{
+  "traceTechnology": "intel-pt",
+  "threadStats": {
+"tid": 3842849,
+"traceItemsCount": 23,
+"memoryUsage": {
+  "totalInBytes": "207",
+  "avgPerItemInBytes": 9
+},
+"timingInSeconds": {
+  "Decoding instructions": 0''', '''
+},
+"events": {
+  "totalCount": 2,
+  "individualCounts": {
+"software disable

[Lldb-commits] [PATCH] D129332: [trace][intel pt] Support dumping the trace info in json

[walter erquinigo via Phabricator via lldb-commits]

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

improve tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129332

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
  lldb/test/API/commands/trace/TestTraceDumpInfo.py
  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
@@ -20,6 +20,72 @@
   substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
+self.expect("thread trace dump info --json",
+  substrs=['''{
+  "traceTechnology": "intel-pt",
+  "threadStats": {
+"tid": 3497234,
+"traceItemsCount": 0,
+"memoryUsage": {
+  "totalInBytes": "0",
+  "avgPerItemInBytes": null
+},
+"timingInSeconds": {},
+"events": {
+  "totalCount": 0,
+  "individualCounts": {}
+},
+"continuousExecutions": 0,
+"PSBBlocks": 0,
+"errorItems": {
+  "total": 0,
+  "individualErrors": {}
+}
+  },
+  "globalStats": {
+"timingInSeconds": {
+  "Context switch and Intel PT traces correlation": 0
+},
+"totalUnattributedPSBBlocks": 0,
+"totalCountinuosExecutions": 153,
+"totalPSBBlocks": 5,
+"totalContinuousExecutions": 153
+  }
+}'''])
+
+self.expect("thread trace dump info 2 --json",
+  substrs=['''{
+  "traceTechnology": "intel-pt",
+  "threadStats": {
+"tid": 3497496,
+"traceItemsCount": 19523,
+"memoryUsage": {
+  "totalInBytes": "175739",
+  "avgPerItemInBytes": 9.00''', '''},
+"timingInSeconds": {},
+"events": {
+  "totalCount": 1,
+  "individualCounts": {
+"software disabled tracing": 1
+  }
+},
+"continuousExecutions": 1,
+"PSBBlocks": 1,
+"errorItems": {
+  "total": 0,
+  "individualErrors": {}
+}
+  },
+  "globalStats": {
+"timingInSeconds": {
+  "Context switch and Intel PT traces correlation": 0''', '''},
+"totalUnattributedPSBBlocks": 0,
+"totalCountinuosExecutions": 153,
+"totalPSBBlocks": 5,
+"totalContinuousExecutions": 153
+  }
+}'''])
+
 @testSBAPIAndCommands
 def testLoadMultiCoreTraceWithStringNumbers(self):
 src_dir = self.getSourceDir()
@@ -71,9 +137,10 @@
 # check that the Process and Thread objects were created correctly
 self.expect("thread info", substrs=["tid = 3842849"])
 self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
-self.expect("thread trace dump info", substrs=['''Trace technology: intel-pt
+self.expect("thread trace dump info", substrs=['''thread #1: tid = 3842849
+
+  Trace technology: intel-pt
 
-thread #1: tid = 3842849
   Total number of trace items: 23
 
   Memory usage:
Index: lldb/test/API/commands/trace/TestTraceDumpInfo.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInfo.py
+++ lldb/test/API/commands/trace/TestTraceDumpInfo.py
@@ -34,9 +34,10 @@
 substrs=["intel-pt"])
 
 self.expect("thread trace dump info",
-substrs=['''Trace technology: intel-pt
+substrs=['''thread #1: tid = 3842849
+
+  Trace technology: intel-pt
 
-thread #1: tid = 3842849
   Total number of trace items: 23
 
   Memory usage:
@@ -54,3 +55,37 @@
   Errors:
 Number of TSC decoding errors: 0'''],
 patterns=["Decoding instructions: \d.\d\ds"])
+
+def testDumpRawTraceSizeJSON(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump info --json ",
+substrs=['''{
+  "traceTechnology": "intel-pt",
+  "threadStats": {
+"tid": 3842849,
+"traceItemsCount": 23,
+"memoryUsage": {
+  "totalInBytes": "207",
+  "avgPerItemInBytes": 9
+},
+"timingInSeconds": {
+  "Decoding instructions": 0''', '''
+},
+"events": {
+  "totalCount": 2,
+  "individualCounts": {
+"software disabled tracing": 2
+  }
+},
+"errorItems": {
+  "total": 0,
+  "individualErrors": {}
+}
+  },
+  "globalStats": {
+"timingInSeconds": {}
+  }
+}'''])
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceInt

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

[walter erquinigo via Phabricator via lldb-commits]

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

Thanks to fredz...@fb.com for coming up with this feature.

When tracing in per-cpu mode, we have information of in which cpu we are 
execution each instruction, which comes from the context switch trace. This 
diff makes this information available as a `cpu changed event`, which an 
additional accessor in the cursor `GetCPU()`. As cpu changes are very 
infrequent, any consumer should listen to cpu change events instead of querying 
the actual cpu of a trace item. Once a cpu change event is seen, the consumer 
can invoke GetCPU() to get that information. Also, it's possible to invoke 
GetCPU() on an arbitrary instruction item, which will return the last cpu seen. 
However, this call is O(logn) and should be used sparingly.

Manually tested with a sample program that starts on cpu 52, then goes to 18, 
and then goes back to 52.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129340

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceDumper.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceDumper.cpp
  lldb/test/API/commands/trace/TestTraceEvents.py
  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
@@ -13,11 +13,11 @@
 trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace.json")
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
 self.expect("thread trace dump instructions 2 -t",
-  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+  substrs=["19523: [tsc=40450075478109270] (error) expected tracing enabled event",
"m.out`foo() + 65 at multi_thread.cpp:12:21",
-   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+   "19521: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
 self.expect("thread trace dump instructions 3 -t",
-  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+  substrs=["67912: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
 self.expect("thread trace dump info --json",
@@ -58,15 +58,16 @@
   "traceTechnology": "intel-pt",
   "threadStats": {
 "tid": 3497496,
-"traceItemsCount": 19523,
+"traceItemsCount": 19524,
 "memoryUsage": {
-  "totalInBytes": "175739",
+  "totalInBytes": "175760",
   "avgPerItemInBytes": 9.00''', '''},
 "timingInSeconds": {},
 "events": {
-  "totalCount": 1,
+  "totalCount": 2,
   "individualCounts": {
-"software disabled tracing": 1
+"software disabled tracing": 1,
+"CPU core changed": 1
   }
 },
 "continuousExecutions": 1,
@@ -92,11 +93,11 @@
 trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace_with_string_numbers.json")
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
 self.expect("thread trace dump instructions 2 -t",
-  substrs=["19522: [tsc=40450075478109270] (error) expected tracing enabled event",
+  substrs=["19523: [tsc=40450075478109270] (error) expected tracing enabled event",
"m.out`foo() + 65 at multi_thread.cpp:12:21",
-   "19520: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
+   "19521: [tsc=40450075477657246] 0x00400ba7jg 0x400bb3"])
 self.expect("thread trace dump instructions 3 -t",
-  substrs=["67911: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
+  substrs=["67912: [tsc=40450075477799536] 0x00400bd7addl   $0x1, -0x4(%rbp)",
"m.out`bar() + 26 at multi_thread.cpp:20:6"])
 
 @testSBAPIAndCommands
@@ -105,11 +106,11 @@
 trace_description_file_path = os.path.join(src_dir, "intelpt-multi-core-trace", "trace_missing_threads.json")
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, substrs=["intel-pt"])
 self.expect("thread trace dump instruct

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

[walter erquinigo via Phabricator via lldb-commits]

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

almost there! I like the tests btw :)




Comment at: lldb/source/Commands/Options.td:304
+  def disassemble_options_kind : Option<"kind", "k">,
+Desc<"Show instruction control flow kind. Refer enum "
+"`InstructionControlFlowKind` for a list of control flow kind. "





Comment at: lldb/source/Commands/Options.td:1159
+  def thread_trace_dump_instructions_show_kind : Option<"kind", "k">, Group<1>,
+Desc<"Show instruction control flow kind. Refer enum "
+"`InstructionControlFlowKind` for a list of control flow kind. "

same



Comment at: lldb/source/Core/Disassembler.cpp:578
+/// instruction, which are represented as the following parameters.
+/// Refer http://ref.x86asm.net/coder.html to see the full list of
+/// opcode and instruction set.





Comment at: lldb/source/Core/Disassembler.cpp:588
+/// \param[in] opcode_len
+///The length of opcode. Valid opcode lengths are 1, 2, or 3.
+///





Comment at: lldb/source/Core/Disassembler.cpp:592-594
+///   eInstructionControlFlowKindOther if the instruction is not interesting.
+///   i.e. a sequential instruction that doesn't affect the control flow of
+///   the program.





Comment at: lldb/source/Core/Disassembler.cpp:705
+/// Decode an instruction into opcode, modrm and opcode_len.
+/// Refer http://ref.x86asm.net/coder.html for the instruction bytes layout.
+/// Opcodes in x86 are generally the first byte of instruction, though two-byte





Comment at: lldb/source/Core/Disassembler.cpp:734-736
+bool InstructionLengthDecode(const uint8_t *inst_bytes, bool is_exec_mode_64b,
+ int bytes_len, uint8_t *primary_opcode,
+ uint8_t *modrm, uint8_t *opcode_len) {

We try not to use out parameters this way. Instead, create a new struct 
InstructionOpcodeAndModrm with the three values that are important to you. 
Then, change this function to return Optional, where 
the None case means that decoding failed



Comment at: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp:28-29
+
+  //  virtual void SetUp() override { }
+  //  virtual void TearDown() override { }
+

delete if not used


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] D128477: [trace] Add a flag to the decoder to output the instruction type

[walter erquinigo via Phabricator via lldb-commits]

wallace accepted this revision.
wallace added a comment.

I'll commit this for you


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] D129588: [trace] Avoid a crash in the dumper when disassembling fails

[walter erquinigo via Phabricator via lldb-commits]

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

In rare situations, disassemblying would fail that produce an invalid
InstructionSP object. We need to check that it's valid before using.

With this change, now the dumper doesn't crash with dumping instructions of
ioctl. In fact, it now dumps this output

{

  "id": 6135,
  "loadAddress": "0x7f4bfe5c7515",
  "module": "libc.so.6",
  "symbol": "ioctl",
  "source": "glibc/2.34/src/glibc-2.34/sysdeps/unix/syscall-template.S",
  "line": 120,
  "column": 0

}

Anyway, we need to investigate why the diassembler failed disassembling that
instruction. From over 2B instructions I was disassembling today, just this
one failed, so this could be a bug in LLVM's core disassembler.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129588

Files:
  lldb/source/Target/TraceDumper.cpp


Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -143,7 +143,7 @@
   m_s << "(error) " << *item.error;
 } else {
   m_s.Format("{0:x+16}", item.load_address);
-  if (item.symbol_info) {
+  if (item.symbol_info && item.symbol_info->instruction) {
 m_s << "";
 item.symbol_info->instruction->Dump(&m_s, /*max_opcode_byte_size=*/0,
 /*show_address=*/false,
@@ -213,9 +213,12 @@
   m_j.attribute(
   "symbol",
   
ToOptionalString(item.symbol_info->sc.GetFunctionName().AsCString()));
-  m_j.attribute("mnemonic",
-
ToOptionalString(item.symbol_info->instruction->GetMnemonic(
-&item.symbol_info->exe_ctx)));
+
+  if (item.symbol_info->instruction) {
+m_j.attribute("mnemonic",
+  
ToOptionalString(item.symbol_info->instruction->GetMnemonic(
+  &item.symbol_info->exe_ctx)));
+  }
 
   if (IsLineEntryValid(item.symbol_info->sc.line_entry)) {
 m_j.attribute(


Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -143,7 +143,7 @@
   m_s << "(error) " << *item.error;
 } else {
   m_s.Format("{0:x+16}", item.load_address);
-  if (item.symbol_info) {
+  if (item.symbol_info && item.symbol_info->instruction) {
 m_s << "";
 item.symbol_info->instruction->Dump(&m_s, /*max_opcode_byte_size=*/0,
 /*show_address=*/false,
@@ -213,9 +213,12 @@
   m_j.attribute(
   "symbol",
   ToOptionalString(item.symbol_info->sc.GetFunctionName().AsCString()));
-  m_j.attribute("mnemonic",
-ToOptionalString(item.symbol_info->instruction->GetMnemonic(
-&item.symbol_info->exe_ctx)));
+
+  if (item.symbol_info->instruction) {
+m_j.attribute("mnemonic",
+  ToOptionalString(item.symbol_info->instruction->GetMnemonic(
+  &item.symbol_info->exe_ctx)));
+  }
 
   if (IsLineEntryValid(item.symbol_info->sc.line_entry)) {
 m_j.attribute(
___
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

[walter erquinigo via Phabricator via lldb-commits]

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

Thanks to rnofe...@fb.com for coming up with these changes.

This diff adds support for passing units in the trace size inputs. For example,
it's now possible to specify 64KB as the trace size, instead of the
problematic 65536. This makes the user experience a bit friendlier.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129613

Files:
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/test/API/commands/trace/TestTraceStartStop.py

Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -46,6 +46,62 @@
 
 self.traceStartThread(iptTraceSize=1048576)
 
+@testSBAPIAndCommands
+def testStartSessionWithSizeDeclarationInUnits(self):
+self.expect("file " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+self.expect("b main")
+self.expect("r")
+
+self.traceStartThread(
+error=True, iptTraceSize="abc",
+substrs=["invalid bytes expression for 'abc'"])
+
+self.traceStartThread(
+error=True, iptTraceSize="123.12",
+substrs=["invalid bytes expression for '123.12'"])
+
+self.traceStartThread(
+error=True, iptTraceSize="\"\"",
+substrs=["invalid bytes expression for ''"])
+
+self.traceStartThread(
+error=True, iptTraceSize="2000B",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 2000"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3MB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3MiB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3mib",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3M",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3KB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3072"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3KiB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3072"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3K",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3072"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3MS",
+substrs=["invalid bytes expression for '3MS'"])
+
+self.traceStartThread(iptTraceSize="1048576")
+
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 def testSBAPIHelp(self):
 self.expect("file " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
@@ -11,7 +11,9 @@
 Arg<"Value">,
 Desc<"Trace size in bytes per thread. It must be a power of 2 greater "
  "than or equal to 4096 (2^12). The trace is circular keeping "
- "the most recent data. Defaults to 4096 bytes.">;
+ "the most recent data. Defaults to 4096 bytes. It's possible to "
+ "specify size using multiples of unit bytes, e.g., 4KB, 1MB, 1MiB, "
+ "where 1K is 1024 bytes and 1M is 1048576 bytes.">;
   def thread_trace_start_intel_pt_tsc: Option<"tsc", "t">,
 Group<1>,
 Desc<"Enable the use of TSC timestamps. This is supported on all devices "
@@ -40,7 +42,9 @@
 Arg<"Value">,
 Desc<"Size in bytes used by each individual per-thread or per-cpu trace "
  "buffer. It must be a power of 2 greater than or equal to 4096 (2^12) "
- 

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

[walter erquinigo via Phabricator via lldb-commits]

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

format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129613

Files:
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/test/API/commands/trace/TestTraceStartStop.py

Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -46,6 +46,62 @@
 
 self.traceStartThread(iptTraceSize=1048576)
 
+@testSBAPIAndCommands
+def testStartSessionWithSizeDeclarationInUnits(self):
+self.expect("file " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+self.expect("b main")
+self.expect("r")
+
+self.traceStartThread(
+error=True, iptTraceSize="abc",
+substrs=["invalid bytes expression for 'abc'"])
+
+self.traceStartThread(
+error=True, iptTraceSize="123.12",
+substrs=["invalid bytes expression for '123.12'"])
+
+self.traceStartThread(
+error=True, iptTraceSize="\"\"",
+substrs=["invalid bytes expression for ''"])
+
+self.traceStartThread(
+error=True, iptTraceSize="2000B",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 2000"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3MB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3MiB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3mib",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3M",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3145728"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3KB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3072"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3KiB",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3072"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3K",
+substrs=["The intel pt trace size must be a power of 2 greater than or equal to 4096 (2^12) bytes. It was 3072"])
+
+self.traceStartThread(
+error=True, iptTraceSize="3MS",
+substrs=["invalid bytes expression for '3MS'"])
+
+self.traceStartThread(iptTraceSize="1048576")
+
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 def testSBAPIHelp(self):
 self.expect("file " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
@@ -11,7 +11,9 @@
 Arg<"Value">,
 Desc<"Trace size in bytes per thread. It must be a power of 2 greater "
  "than or equal to 4096 (2^12). The trace is circular keeping "
- "the most recent data. Defaults to 4096 bytes.">;
+ "the most recent data. Defaults to 4096 bytes. It's possible to "
+ "specify size using multiples of unit bytes, e.g., 4KB, 1MB, 1MiB, "
+ "where 1K is 1024 bytes and 1M is 1048576 bytes.">;
   def thread_trace_start_intel_pt_tsc: Option<"tsc", "t">,
 Group<1>,
 Desc<"Enable the use of TSC timestamps. This is supported on all devices "
@@ -40,7 +42,8 @@
 Arg<"Value">,
 Desc<"Size in bytes used by each individual per-thread or per-cpu trace "
  "buffer. It must be a power of 2 greater than or equal to 4096 (2^12) "
- "bytes.">;
+ "bytes. It's possible to specify a unit for these bytes, like 4KB, "
+ "16KiB or 1MB. Lower case units are allowed for convenience.">;
   def process_trace_start_intel_pt_per_cpu_tracing:
 Option<"per-cpu-tracing", "c">,
 Group<1>,
@@ -53,7 +56,8 @@
  "option forces the capture of TSC timestamps (se

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

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

I'll remove the unwanted formatting changes


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] D129239: [trace] Add an option to save a compact trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



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

jj10306 wrote:
> why do we want the error record?
i don't want to lose this information because they can show contention issues


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] D129340: [trace][intel pt] Create a CPU change event and expose it in the dumper

[walter erquinigo via Phabricator via lldb-commits]

wallace requested review of this revision.
wallace added inline comments.



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

jj10306 wrote:
> 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?
for per-cpu tracing, we'll always have the cpu change event as the first trace 
item. For per-thread tracing, we won't have this



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 {

jj10306 wrote:
> 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?
I've been trying different ways of storing the data and exposing them, so 
that's why you see some inconsistencies. And yes, we should things the same way 
we we've doing it for CPUs, but I'll leave it for a future diff, in which we 
also decide how we are going to expose TSCs and nanoseconds in the trace cursor.



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");
+  }

jj10306 wrote:
> 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
i think that won't be needed now because i don't see any other part of LLDB 
dumping this information in a string format. Other external consumers would 
probably have their own format for this data



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);

jj10306 wrote:
> 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
again, i don't see this being reused in other parts of LLDB, so I don't think 
it's worth doing the abstractions


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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



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);

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



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

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


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

[walter erquinigo via Phabricator via lldb-commits]

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

- Decouple TSCs from trace items
- Turn TSCs into events just like CPUs
- Add a GetWallTime that returns the wall time that the trace plug-in can infer 
for each trace item.
- For intel pt, we are doing the following interpolation: if an instruction 
takes less than 1 TSC, we use that duration, otherwise, we assume the 
instruction took 1 TSC. This helps us avoid having to handle context switches, 
changes to kernel, idle times, decoding errors, etc. We are just trying to show 
some approximation and not the real data. For the real data, TSCs are the way 
to go.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130054

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceDumper.h
  lldb/include/lldb/lldb-enumerations.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/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceDumper.cpp

Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -128,16 +128,27 @@
 
 m_s.Format("{0}: ", item.id);
 
-if (m_options.show_tsc) {
-  m_s.Format("[tsc={0}] ",
- item.tsc ? std::to_string(*item.tsc) : "unavailable");
+if (m_options.show_timestamps) {
+  m_s.Format("[{0}] ", item.timestamp
+   ? (std::to_string(*item.timestamp) + " ns")
+   : "unavailable");
 }
 
 if (item.event) {
   m_s << "(event) " << TraceCursor::EventKindToString(*item.event);
-  if (*item.event == eTraceEventCPUChanged) {
+  switch (*item.event) {
+  case eTraceEventCPUChanged:
 m_s.Format(" [new CPU={0}]",
item.cpu_id ? std::to_string(*item.cpu_id) : "unavailable");
+break;
+  case eTraceEventHWClockTick:
+m_s.Format(" [new HW clock tick={0}",
+   item.hw_clock ? std::to_string(*item.hw_clock)
+ : "unavailable");
+break;
+  case eTraceEventDisabledHW:
+  case eTraceEventDisabledSW:
+break;
   }
 } else if (item.error) {
   m_s << "(error) " << *item.error;
@@ -181,7 +192,8 @@
 | {
   "loadAddress": string decimal,
   "id": decimal,
-  "tsc"?: string decimal,
+  "hwClock"?: string decimal,
+  "timestamp_ns"?: string decimal,
   "module"?: string,
   "symbol"?: string,
   "line"?: decimal,
@@ -202,8 +214,17 @@
 
   void DumpEvent(const TraceDumper::TraceItem &item) {
 m_j.attribute("event", TraceCursor::EventKindToString(*item.event));
-if (item.event == eTraceEventCPUChanged)
+switch (*item.event) {
+case eTraceEventCPUChanged:
   m_j.attribute("cpuId", item.cpu_id);
+  break;
+case eTraceEventHWClockTick:
+  m_j.attribute("hwClock", item.hw_clock);
+  break;
+case eTraceEventDisabledHW:
+case eTraceEventDisabledSW:
+  break;
+}
   }
 
   void DumpInstruction(const TraceDumper::TraceItem &item) {
@@ -234,10 +255,11 @@
   void TraceItem(const TraceDumper::TraceItem &item) override {
 m_j.object([&] {
   m_j.attribute("id", item.id);
-  if (m_options.show_tsc)
-m_j.attribute(
-"tsc",
-item.tsc ? Optional(std::to_string(*item.tsc)) : None);
+  if (m_options.show_timestamps)
+m_j.attribute("timestamp_ns", item.timestamp
+  ? Optional(
+std::to_string(*item.timestamp))
+  : None);
 
   if (item.event) {
 DumpEvent(item);
@@ -289,8 +311,8 @@
   TraceItem item;
   item.id = m_cursor_up->GetId();
 
-  if (m_options.show_tsc)
-item.tsc = m_cursor_up->GetCounter(lldb::eTraceCounterTSC);
+  if (m_options.show_timestamps)
+item.timestamp = m_cursor_up->GetWallClockTime();
   return item;
 }
 
@@ -366,8 +388,17 @@
   if (!m_options.show_events)
 continue;
   item.event = m_cursor_up->GetEventType();
-  if (*item.event == eTraceEventCPUChanged)
+  switch (*item.event) {
+  case eTraceEventCPUChanged:
 item.cpu_id = m_cursor_up->GetCPU();
+break;
+  case eTraceEventHWClockTick:
+item.hw_clock = m_cursor_up->GetHWClock();
+break;
+  case eTraceEventDisabledHW:
+  case eTraceEventDisabledSW:
+break;
+  }
 } else if (m_cursor_up->IsError

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

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 445664.
wallace edited the summary of this revision.
wallace added a comment.
Herald added subscribers: jsji, pengfei.

fix nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130054

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceDumper.h
  lldb/include/lldb/lldb-enumerations.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/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceDumper.cpp

Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -128,16 +128,26 @@
 
 m_s.Format("{0}: ", item.id);
 
-if (m_options.show_tsc) {
-  m_s.Format("[tsc={0}] ",
- item.tsc ? std::to_string(*item.tsc) : "unavailable");
+if (m_options.show_timestamps) {
+  m_s.Format("[{0}] ", item.timestamp
+   ? (std::to_string(*item.timestamp) + " ns")
+   : "unavailable");
 }
 
 if (item.event) {
   m_s << "(event) " << TraceCursor::EventKindToString(*item.event);
-  if (*item.event == eTraceEventCPUChanged) {
+  switch (*item.event) {
+  case eTraceEventCPUChanged:
 m_s.Format(" [new CPU={0}]",
item.cpu_id ? std::to_string(*item.cpu_id) : "unavailable");
+break;
+  case eTraceEventHWClockTick:
+m_s.Format(" [{0}]", item.hw_clock ? std::to_string(*item.hw_clock)
+   : "unavailable");
+break;
+  case eTraceEventDisabledHW:
+  case eTraceEventDisabledSW:
+break;
   }
 } else if (item.error) {
   m_s << "(error) " << *item.error;
@@ -181,7 +191,8 @@
 | {
   "loadAddress": string decimal,
   "id": decimal,
-  "tsc"?: string decimal,
+  "hwClock"?: string decimal,
+  "timestamp_ns"?: string decimal,
   "module"?: string,
   "symbol"?: string,
   "line"?: decimal,
@@ -202,8 +213,17 @@
 
   void DumpEvent(const TraceDumper::TraceItem &item) {
 m_j.attribute("event", TraceCursor::EventKindToString(*item.event));
-if (item.event == eTraceEventCPUChanged)
+switch (*item.event) {
+case eTraceEventCPUChanged:
   m_j.attribute("cpuId", item.cpu_id);
+  break;
+case eTraceEventHWClockTick:
+  m_j.attribute("hwClock", item.hw_clock);
+  break;
+case eTraceEventDisabledHW:
+case eTraceEventDisabledSW:
+  break;
+}
   }
 
   void DumpInstruction(const TraceDumper::TraceItem &item) {
@@ -234,10 +254,11 @@
   void TraceItem(const TraceDumper::TraceItem &item) override {
 m_j.object([&] {
   m_j.attribute("id", item.id);
-  if (m_options.show_tsc)
-m_j.attribute(
-"tsc",
-item.tsc ? Optional(std::to_string(*item.tsc)) : None);
+  if (m_options.show_timestamps)
+m_j.attribute("timestamp_ns", item.timestamp
+  ? Optional(
+std::to_string(*item.timestamp))
+  : None);
 
   if (item.event) {
 DumpEvent(item);
@@ -289,8 +310,8 @@
   TraceItem item;
   item.id = m_cursor_up->GetId();
 
-  if (m_options.show_tsc)
-item.tsc = m_cursor_up->GetCounter(lldb::eTraceCounterTSC);
+  if (m_options.show_timestamps)
+item.timestamp = m_cursor_up->GetWallClockTime();
   return item;
 }
 
@@ -366,8 +387,17 @@
   if (!m_options.show_events)
 continue;
   item.event = m_cursor_up->GetEventType();
-  if (*item.event == eTraceEventCPUChanged)
+  switch (*item.event) {
+  case eTraceEventCPUChanged:
 item.cpu_id = m_cursor_up->GetCPU();
+break;
+  case eTraceEventHWClockTick:
+item.hw_clock = m_cursor_up->GetHWClock();
+break;
+  case eTraceEventDisabledHW:
+  case eTraceEventDisabledSW:
+break;
+  }
 } else if (m_cursor_up->IsError()) {
   item.error = m_cursor_up->GetError();
 } else {
Index: lldb/source/Target/TraceCursor.cpp
===
--- lldb/source/Target/TraceCursor.cpp
+++ lldb/source/Target/TraceCursor.cpp
@@ -50,5 +50,7 @@
 return "software disabled tracing";
   case lldb::eTraceEventCPUChanged:
 return "CPU core changed";
+  case lldb::eTraceEventHWClockTick:
+return "HW clock tick";
   }
 }
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
==

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

[walter erquinigo via Phabricator via lldb-commits]

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

..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130054

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceDumper.h
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/include/lldb/lldb-enumerations.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/LibiptDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h
  lldb/source/Plugins/Trace/intel-pt/ThreadDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/ThreadDecoder.h
  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/TraceIntelPTMultiCpuDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.h
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceDumper.cpp
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp

Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -79,6 +79,15 @@
   return time_zero.value + quot * time_mult + ((rem * time_mult) >> time_shift);
 }
 
+long double LinuxPerfZeroTscConversion::ToNanosFloat(long double lowest_nanos,
+ uint64_t tsc) const {
+  uint64_t quot = tsc >> time_shift;
+  uint64_t rem_flag = (((uint64_t)1 << time_shift) - 1);
+  uint64_t rem = tsc & rem_flag;
+  return (time_zero.value + quot * time_mult - lowest_nanos) +
+ (static_cast(rem * time_mult) / pow(2, time_shift));
+}
+
 uint64_t LinuxPerfZeroTscConversion::ToTSC(uint64_t nanos) const {
   uint64_t time = nanos - time_zero.value;
   uint64_t quot = time / time_mult;
Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -128,16 +128,26 @@
 
 m_s.Format("{0}: ", item.id);
 
-if (m_options.show_tsc) {
-  m_s.Format("[tsc={0}] ",
- item.tsc ? std::to_string(*item.tsc) : "unavailable");
+if (m_options.show_timestamps) {
+  m_s.Format("[{0}] ", item.timestamp
+   ? (formatv("{0:2}", *item.timestamp) + " ns")
+   : "unavailable");
 }
 
 if (item.event) {
   m_s << "(event) " << TraceCursor::EventKindToString(*item.event);
-  if (*item.event == eTraceEventCPUChanged) {
+  switch (*item.event) {
+  case eTraceEventCPUChanged:
 m_s.Format(" [new CPU={0}]",
item.cpu_id ? std::to_string(*item.cpu_id) : "unavailable");
+break;
+  case eTraceEventHWClockTick:
+m_s.Format(" [{0}]", item.hw_clock ? std::to_string(*item.hw_clock)
+   : "unavailable");
+break;
+  case eTraceEventDisabledHW:
+  case eTraceEventDisabledSW:
+break;
   }
 } else if (item.error) {
   m_s << "(error) " << *item.error;
@@ -181,7 +191,8 @@
 | {
   "loadAddress": string decimal,
   "id": decimal,
-  "tsc"?: string decimal,
+  "hwClock"?: string decimal,
+  "timestamp_ns"?: string decimal,
   "module"?: string,
   "symbol"?: string,
   "line"?: decimal,
@@ -202,8 +213,17 @@
 
   void DumpEvent(const TraceDumper::TraceItem &item) {
 m_j.attribute("event", TraceCursor::EventKindToString(*item.event));
-if (item.event == eTraceEventCPUChanged)
+switch (*item.event) {
+case eTraceEventCPUChanged:
   m_j.attribute("cpuId", item.cpu_id);
+  break;
+case eTraceEventHWClockTick:
+  m_j.attribute("hwClock", item.hw_clock);
+  break;
+case eTraceEventDisabledHW:
+case eTraceEventDisabledSW:
+  break;
+}
   }
 
   void DumpInstruction(const TraceDumper::TraceItem &item) {
@@ -234,10 +254,11 @@
   void TraceItem(const TraceDumper::TraceItem &item) override {
 m_j.object([&] {
   m_j.attribute("id", item.id);
-  if (m_options.show_tsc)
-m_j.attribute(
-"tsc",
-item.tsc ? Optional(std::to_string(*item.tsc)) : None);
+  if (m_options.show_timestamps)
+m_j.attribute("timestamp_ns", item.timestamp
+  ? Optional(
+std::to_string(*item.timestamp))
+  : None);
 
   if (item.event) {
 DumpEvent(item);
@@ -289

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

[walter erquinigo via Phabricator via lldb-commits]

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

.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130054

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceDumper.h
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/include/lldb/lldb-enumerations.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/LibiptDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h
  lldb/source/Plugins/Trace/intel-pt/ThreadDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/ThreadDecoder.h
  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/TraceIntelPTMultiCpuDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCpuDecoder.h
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceDumper.cpp

Index: lldb/source/Target/TraceDumper.cpp
===
--- lldb/source/Target/TraceDumper.cpp
+++ lldb/source/Target/TraceDumper.cpp
@@ -128,16 +128,26 @@
 
 m_s.Format("{0}: ", item.id);
 
-if (m_options.show_tsc) {
-  m_s.Format("[tsc={0}] ",
- item.tsc ? std::to_string(*item.tsc) : "unavailable");
+if (m_options.show_timestamps) {
+  m_s.Format("[{0}] ", item.timestamp
+   ? (formatv("{0:2}", *item.timestamp) + " ns")
+   : "unavailable");
 }
 
 if (item.event) {
   m_s << "(event) " << TraceCursor::EventKindToString(*item.event);
-  if (*item.event == eTraceEventCPUChanged) {
+  switch (*item.event) {
+  case eTraceEventCPUChanged:
 m_s.Format(" [new CPU={0}]",
item.cpu_id ? std::to_string(*item.cpu_id) : "unavailable");
+break;
+  case eTraceEventHWClockTick:
+m_s.Format(" [{0}]", item.hw_clock ? std::to_string(*item.hw_clock)
+   : "unavailable");
+break;
+  case eTraceEventDisabledHW:
+  case eTraceEventDisabledSW:
+break;
   }
 } else if (item.error) {
   m_s << "(error) " << *item.error;
@@ -181,7 +191,8 @@
 | {
   "loadAddress": string decimal,
   "id": decimal,
-  "tsc"?: string decimal,
+  "hwClock"?: string decimal,
+  "timestamp_ns"?: string decimal,
   "module"?: string,
   "symbol"?: string,
   "line"?: decimal,
@@ -202,8 +213,17 @@
 
   void DumpEvent(const TraceDumper::TraceItem &item) {
 m_j.attribute("event", TraceCursor::EventKindToString(*item.event));
-if (item.event == eTraceEventCPUChanged)
+switch (*item.event) {
+case eTraceEventCPUChanged:
   m_j.attribute("cpuId", item.cpu_id);
+  break;
+case eTraceEventHWClockTick:
+  m_j.attribute("hwClock", item.hw_clock);
+  break;
+case eTraceEventDisabledHW:
+case eTraceEventDisabledSW:
+  break;
+}
   }
 
   void DumpInstruction(const TraceDumper::TraceItem &item) {
@@ -234,10 +254,11 @@
   void TraceItem(const TraceDumper::TraceItem &item) override {
 m_j.object([&] {
   m_j.attribute("id", item.id);
-  if (m_options.show_tsc)
-m_j.attribute(
-"tsc",
-item.tsc ? Optional(std::to_string(*item.tsc)) : None);
+  if (m_options.show_timestamps)
+m_j.attribute("timestamp_ns", item.timestamp
+  ? Optional(
+std::to_string(*item.timestamp))
+  : None);
 
   if (item.event) {
 DumpEvent(item);
@@ -289,8 +310,8 @@
   TraceItem item;
   item.id = m_cursor_up->GetId();
 
-  if (m_options.show_tsc)
-item.tsc = m_cursor_up->GetCounter(lldb::eTraceCounterTSC);
+  if (m_options.show_timestamps)
+item.timestamp = m_cursor_up->GetWallClockTime();
   return item;
 }
 
@@ -366,8 +387,17 @@
   if (!m_options.show_events)
 continue;
   item.event = m_cursor_up->GetEventType();
-  if (*item.event == eTraceEventCPUChanged)
+  switch (*item.event) {
+  case eTraceEventCPUChanged:
 item.cpu_id = m_cursor_up->GetCPU();
+break;
+  case eTraceEventHWClockTick:
+item.hw_clock = m_cursor_up->GetHWClock();
+break;
+  case eTraceEventDisabledHW:
+  case eTraceEventDisabledSW:
+break;
+  }
 } else if (m_cursor_up->IsError()) {
   item.error = m_cursor_up->GetError();
 } else {
Index: lldb/source/Target/TraceCursor.cpp
==