Trim reversed stacks to prevent sort check from failing

[bobrik]

If the last line of the stack starts with a space, then the space is used in sorting. However, the space is removed when the line is then fed into the flamegraph, which makes the sorting invalid. This commit adds a trim to the reverse process to fix this.

[jonhoo]

Thank you! I'm curious though — how can this happen in practice? Like, what causes a function to have a leading space in its name? How do you end up with the file that you included in this PR?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.38%. Comparing base (306edee) to head (40a4ba9).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #338   +/-   ##
=======================================
  Coverage   91.37%   91.38%           
=======================================
  Files          20       20           
  Lines        4440     4444    +4     
=======================================
+ Hits         4057     4061    +4     
  Misses        383      383           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bobrik]

I think some symbols can have ;:

0000000000a759a0 l     F .text	0000000000000008              core::array::<impl core::default::Default for [T; _]>::default::h67c9877e6f2a615c
0000000000a759b0 l     F .text	0000000000000008              core::array::<impl core::default::Default for [T; _]>::default::h8cc48dfe0d9bff78
0000000000a75800 l     F .text	0000000000000097              core::array::<impl core::fmt::Debug for [T; N]>::fmt::h24e996bfa7154ca1
0000000000a759d0 l     F .text	0000000000000008              core::array::<impl core::default::Default for [T; _]>::default::he4723b7c57f6b881
0000000000a00af0 g     F .text	0000000000000074              <[u8; 19] as phf_shared::FmtConst>::fmt_const::hb6b803208e5640c0
0000000000a002f0 g     F .text	0000000000000074              <[u8; 3] as phf_shared::FmtConst>::fmt_const::h524dac4335e1334b
0000000000a00370 g     F .text	0000000000000074              <[u8; 4] as phf_shared::FmtConst>::fmt_const::h117038a50e8f7ca3
0000000000a009f0 g     F .text	0000000000000074              <[u8; 17] as phf_shared::FmtConst>::fmt_const::h4e993ac5b3e99e7c

Since the stack frame separator in collapsed files is ;, they split into two, with the second one having a space before the symbol name.

In our script we have:

perf script --no-demangle -i perf.data | ... | c++filt | /usr/bin/inferno-collapse-perf --all | ...  > perf.stacks.collapsed

Arguably, these collapsed stacks are somewhat incorrect, but having a somewhat incorrect flamegraph beats having none.

[jonhoo]

Oh iiiinteresting. That is indeed pretty broken. I wonder if @brendangregg has thought about how the collapse format for flamegraph more broadly should handle symbols with ; in them?

In the meantime, would you mind making the test collapse hold lines more along the lines of what you'd realistically get with a Rust type like [u8; 3]? The current one feels hand-edited and isn't intuitively obvious how it can happen (which is what prompted this discussion).

[bobrik]

Oh iiiinteresting. That is indeed pretty broken. I wonder if @brendangregg has thought about how the collapse format for flamegraph more broadly should handle symbols with ; in them?

Internally we use → when we do some pre-processing to prepend cgroup hierarchy to the stack trace. There's little chance of it being in the raw data.

In the meantime, would you mind making the test collapse hold lines more along the lines of what you'd realistically get with a Rust type like [u8; 3]? The current one feels hand-edited and isn't intuitively obvious how it can happen (which is what prompted this discussion).

Good idea, updated with a realistic name.

[jonhoo]

Thanks!

[jonhoo]

Releasing in #339