[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 lldb-commits at lists.llvm.org
Mon Aug 2 12:09:18 PDT 2021

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
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?

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list