Re: [PATCH v1 00/11] perf script: Refactor branch flags for Arm SPE

[Leo Yan <leo.yan@arm.com>]

Hi Ian,

On Tue, Feb 11, 2025 at 02:34:46PM -0800, Ian Rogers wrote:
> On Wed, Feb 5, 2025 at 4:16 AM Leo Yan <leo.yan@arm.com> wrote:
> >
> > This patch series refactors branch flags for support Arm SPE.  The patch
> > set is divided into two parts, the first part is for refactoring common
> > code and the second part is for enabling Arm SPE.

[...]
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> 
> Built and tested (on x86). A little strange patch 5 adds a new bit not
> at the end, but "Sample parsing" test wasn't broken so looks like it
> is good. I was surprised the use of value in the union:
> ```
> struct branch_flags {
> union {
> u64 value;
> struct {
> u64 mispred:1;
> u64 predicted:1;
> ...
> ```
> didn't get broken. Perhaps there's an opportunity for additional tests.

If the branch stack's flag sticks to a hardware format, then the patch 5
is concerned.  My understanding is the branch flag is a synthesized
value (see intel_pt_lbr_flags() for x86).  So it is fine for rearrange
the bit layout.

The "Sample parsing" test is for big/little endian test, it does not
test for specific bit ordering, this is why the test passes.

If you think it is safer to move the new added bit at the tail of the
bit definitions (just before the 'reserved' field), I can send a new
version for this.  Please let me know your preference.

Thanks for review and test!

Leo