[Lldb-commits] [PATCH] D105741: [trace] Introduce Hierarchical Trace Representation (HTR) and add `thread trace export ctf` command for Intel PT trace visualization

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

Hey @jj10306, thanks for working on this.

@wallace @clayborg -- It seems like there are a ton of logic changes introduced in this patch that are tested at too coarse a level. I'm not confident in the changes being made here. For example, there's a bunch of subtle work being done in `BasicSuperBlockMerge`, but only ~one opaque high-level check that attempts to validate any of the logic: `self.assertTrue(num_units_by_layer[1] == 383)`. Where does 383 come from? How do we know that that's the right result? If there's a bug in `BasicSuperBlockMerge`, would the regression test just look like changing the magic 383 number?

I'm not really comfortable with this being checked in, and would suggest a revert until some more granular unit tests can be added to the patch. (Also paging @JDevlieghere in case he has thoughts on the testing.)



================
Comment at: lldb/docs/htr.rst:27
+.. image:: media/htr-example.png
+
+Passes
----------------
Would be helpful to describe a brief feature roadmap here, explaining what debugger functionality can be built on top of this HTR concept.


================
Comment at: lldb/source/Plugins/TraceExporter/common/TraceHTR.h:38
+                   size_t num_instructions,
+                   llvm::DenseMap<ConstString, size_t> &func_calls)
+      : m_first_instruction_load_address(first_instruction_load_address),
----------------
Why not move 'func_calls' instead of copying it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105741