Re: [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST

Hello,

On Sat, Jul 25, 2026 at 08:56:50AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Medium] The test setup for grouped reads without PERF_FORMAT_LOST validates garbage data because it provides a standard C array of structs rather than a tightly packed buffer.
> --
> 
> --- Patch [1]: [PATCH] perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST ---
> commit 803c0b64d4e8c311df29991bbbdd74726b85a1dd
> Author: PVS Narasimha Rao <[email protected]>
> 
> perf test sample-parsing: Validate PERF_FORMAT_GROUP values without LOST
> 
> This patch updates the sample parsing test to validate the value and id
> fields in grouped reads even when PERF_FORMAT_LOST is absent. It uses
> next_sample_read_value() to properly navigate the variable-sized group
> entries during validation.
> 
> > diff --git a/tools/perf/tests/sample-parsing.c b/tools/perf/tests/sample-parsing.c
> > index 55f0b73ca20e0..e44b01e5cd14c 100644
> > --- a/tools/perf/tests/sample-parsing.c
> > +++ b/tools/perf/tests/sample-parsing.c
> [ ... ]
> > @@ -86,10 +86,27 @@ static bool samples_same(struct perf_sample *s1,
> >  			COMP(read.time_running);
> >  		/* PERF_FORMAT_ID is forced for PERF_SAMPLE_READ */
> >  		if (read_format & PERF_FORMAT_GROUP) {
> > +			struct sample_read_value *v1 = s1->read.group.values;
> > +			struct sample_read_value *v2 = s2->read.group.values;
> > +
> >  			for (i = 0; i < s1->read.group.nr; i++) {
> > -				/* FIXME: check values without LOST */
> > -				if (read_format & PERF_FORMAT_LOST)
> > -					MCOMP(read.group.values[i]);
> > +				if (v1->value != v2->value) {
> > +					pr_debug("Samples differ at 'read.group.values[].value'\n");
> > +					return false;
> > +				}
> > +
> > +				if (v1->id != v2->id) {
> > +					pr_debug("Samples differ at 'read.group.values[].id'\n");
> > +					return false;
> > +				}
> > +
> > +				if (read_format & PERF_FORMAT_LOST &&
> > +				    v1->lost != v2->lost) {
> > +					pr_debug("Samples differ at 'read.group.values[].lost'\n");
> > +					return false;
> > +				}
> > +				v1 = next_sample_read_value(v1, read_format);
> > +				v2 = next_sample_read_value(v2, read_format);
> 
> [Severity: Medium]
> This is a pre-existing issue, but does using next_sample_read_value() here
> inadvertently cause the test to assert equality on misaligned garbage data
> when PERF_FORMAT_LOST is omitted?
> 
> Looking at do_test(), the original sample is initialized with a standard C
> array of struct sample_read_value:
> 
> tools/perf/tests/sample-parsing.c:do_test() {
>     ...
>     struct sample_read_value values[] = {{1, 5, 0}, {9, 3, 0}, {2, 7, 0}, {6, 4, 1},};
>     ...
>     if (read_format & PERF_FORMAT_GROUP) {
>         sample.read.group.nr     = 4;
>         sample.read.group.values = values;
>     }
>     ...
> }
> 
> Since this array always has a 24-byte stride, but next_sample_read_value()
> advances by a 16-byte stride when PERF_FORMAT_LOST is absent, both the
> synthesizer and this validation loop end up reading overlapping memory
> instead of the actual struct fields.
> 
> Could the test setup be updated to provide a tightly packed buffer so that
> we validate the actual fields rather than falsely passing on identical
> misaligned reads?

I think it's a valid issue and it seems there's a bug in the
perf_event__synthesize_sample().  The input values contain the lost
field regardless of the read format.  So it should skip the field
carefully.

Thanks,
Namhyung