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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

In D105741#2920588 <https://reviews.llvm.org/D105741#2920588>, @vsk wrote:

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

Apologies for this - will submit a patch with more comprehensive tests and other minor fixes this week!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105741