Re: [PATCH v3 10/19] perf test attr: Accept either sample id bit perf asks for

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf test: Event.equal() compares event against itself for unrecorded events
- [Medium] perf: false attr test failures from asymmetric config masking
--

commit 26200ca4070ba6631a880adb369c9b57d611baf6
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf test attr: Accept either sample id bit perf asks for

evlist__config() asks for a way to tell the samples apart,
PERF_SAMPLE_IDENTIFIER where available, PERF_SAMPLE_ID otherwise. This patch
updates perf test attr to accept either sample id bit when comparing expected
and recorded events by tracking which side is the recorded event.

> diff --git a/tools/perf/tests/shell/lib/attr.py b/tools/perf/tests/shell/lib/attr.py
> index ee55fbe6f07bd..da4614068772e 100644
> --- a/tools/perf/tests/shell/lib/attr.py
> +++ b/tools/perf/tests/shell/lib/attr.py
[ ... ]
> @@ -198,13 +243,33 @@ class Event(dict):
[ ... ]
>      def equal(self, other):
>          for t in Event.terms:
>              log.debug("      [%s] %s %s", t, self[t], other[t])
>              if t not in self or t not in other:
>                  return False
> -            exp = self.hide_core_pmu_type(t, self[t], other)
> -            got = self.hide_core_pmu_type(t, other[t], other)
> +            recorded = self.recorded_side(other)
> +            expected = other if recorded is self else self
> +            exp = self.hide_core_pmu_type(t, expected[t], recorded or expected)
> +            got = self.hide_core_pmu_type(t, (recorded or self)[t], expected)

[Severity: Medium]
When neither self nor other has the recorded attribute set,
recorded_side(other) will return None. In this case, expected evaluates
to self and got extracts its term from (recorded or self)[t], which is
also self[t].

Does this cause both exp and got to extract from self, ignoring other
entirely and causing the equality check to falsely succeed when comparing two
distinct unrecorded events?

[Severity: Medium]
For got, the third argument passed to hide_core_pmu_type() is hardcoded to
expected.

When self is an expected wildcard event and other is a recorded legacy
PMU event, does this pass (expected, expected) to the masking logic? If so,
could this completely ignore the recorded event's type, resulting in asymmetric
masking of the config field and false test failures?

> +            if recorded is not None:
> +                exp, got = self.hide_event_count_dependent_terms(t, exp, got)
>              if not data_equal(exp, got):
>                  return False
>          return True
> @@ -218,10 +283,15 @@ class Event(dict):
>          for t in Event.terms:
>              if t not in self or t not in other:
>                  continue
> -            exp = self.hide_core_pmu_type(t, self[t], other)
> -            got = self.hide_core_pmu_type(t, other[t], other)
> +            recorded = self.recorded_side(other)
> +            expected = other if recorded is self else self
> +            exp = self.hide_core_pmu_type(t, expected[t], recorded or expected)
> +            got = self.hide_core_pmu_type(t, (recorded or self)[t], expected)

[Severity: Medium]
The same fallback logic appears to be present in Event.diff() as well. Does
passing None for recorded here break the diff logic for unrecorded events by
causing it to diff self against self?

[Severity: Medium]
The third argument for got is again hardcoded to expected instead of
checking the recorded event. Could this asymmetric masking lead to misleading
error logs that report false mismatches for valid legacy PMU configurations?

> +            if recorded is not None:
> +                exp, got = self.hide_event_count_dependent_terms(t, exp, got)
>              if not data_equal(exp, got):
> -                log.warning("expected %s=%s, got %s", t, self[t], other[t])
> +                log.warning("expected %s=%s, got %s", t, expected[t],
> +                            (recorded or self)[t])

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016001843.71978-1-acme@kernel.org?part=10