Re: [RFC PATCH v1 4/8] perf: Refactor thread map and symbol APIs to take perf_sample

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The updated `thread__find_map` function incorrectly treats an address of `0` as missing IP data, breaking lookups for address `0` and skipping required `addr_location` field initialization.
- [Medium] Excessive and redundant zero-initialization of the large `struct perf_sample` in hot paths causes severe performance overhead.
--

--- Patch [4]: [RFC PATCH v1 4/8] perf: Refactor thread map and symbol APIs to take perf_sample ---
commit 1099356ec0523851e160af69676be71cd35a2dc7
Author: Ian Rogers <[email protected]>

perf: Refactor thread map and symbol APIs to take perf_sample

This commit refactors the thread__find_map and thread__find_symbol
family of APIs in the perf tools to accept a pointer to a perf_sample
structure instead of individual cpumode and addr parameters. This change
prepares the APIs to support symbol lookups using a build ID and offset,
which are contained in the sample payload rather than just the address.

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1000,8 +1000,10 @@ static int perf_sample__fprintf_brstack(struct perf_sample *sample,
>  
>  			addr_location__init(&alf);
>  			addr_location__init(&alt);
> -			thread__find_map_fb(thread, sample->cpumode, from, &alf);
> -			thread__find_map_fb(thread, sample->cpumode, to, &alt);
> +			thread__find_map_fb(thread,
> +			    &(struct perf_sample){.cpumode = sample->cpumode, .ip = from}, &alf);
> +			thread__find_map_fb(thread,
> +			    &(struct perf_sample){.cpumode = sample->cpumode, .ip = to}, &alt);

[Severity: Medium]
Will these compound literals cause a significant performance regression in
this loop?

Because struct perf_sample is a large structure (~320 bytes), omitting the
remaining fields in the compound literal forces the compiler to
zero-initialize the rest of the struct for every branch stack entry.
Creating and zeroing these large structs inside a tight loop could
introduce severe CPU overhead when processing traces.

[ ... ]

> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -702,34 +702,39 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>  	thread__zput(al->thread);
>  	al->thread = thread__get(thread);
>  
> -	al->addr = addr;
> -	al->cpumode = cpumode;
> +	if (sample->ip) {
> +		al->addr = sample->ip;
> +	} else {
> +		pr_debug("Sample missing IP data\n");
> +		return NULL;
> +	}
> +	al->cpumode = sample->cpumode;

[Severity: Medium]
Could this fail when resolving legitimate 0 addresses?

If a page fault or branch event occurs exactly at address 0 (for example,
a NULL pointer dereference), tools like trace__pgfault pass a sample->addr
of 0 as the IP. This condition evaluates to false for address 0, causing
the function to return NULL early.

When the function returns early here, it skips the initialization of
al->cpumode and al->level, which can leave those fields uninitialized
and cause trace output corruption (like printing null bytes).

[ ... ]

> @@ -816,7 +822,10 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
>  		return -1;
>  
>  	dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread__tid(thread));
> -	thread__find_map(thread, sample->cpumode, sample->ip, al);
> +	thread__find_map(thread,
> +			 &(struct perf_sample){.cpumode = sample->cpumode,
> +					       .ip = sample->ip,
> +					       .bid = sample->bid}, al);

[Severity: Medium]
Could this compound literal also introduce overhead in the hot path?

Similar to the loop in perf_sample__fprintf_brstack, machine__resolve is
called for every single sample event. Creating a new, mostly
zero-initialized struct perf_sample here forces the compiler to write
zeroes to hundreds of bytes per event parsed, which can drastically slow
down trace processing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4