Re: [PATCH v6 06/25] perf evsel: Refactor evsel tracepoint sample accessors perf_sample

[Ian Rogers]

On Sun, Apr 5, 2026 at 11:06 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Fri, Apr 03, 2026 at 08:43:06PM -0700, Ian Rogers wrote:
> > The evsel argument to evsel__intval, evsel__rawptr, and similar
> > functions, is unnecessary as it can be read from the sample. Remove
> > the evsel and rename the function to match that the data is coming
> > from the sample.
> >
> > Add bounds checks to a number read values from review feedback. Make
> > perf_sample__strval avoid returning NULL pointers, return an empty
> > string instead. Fix the function type to reflect this, catching a bug
> > in kwork where the string wasn't being duplicated.
>
> I'm not sure if it's a bug... the string was never freed and duplicating
> it would create a lot of memory leaks.
>
> Also it seems you still check the return value of perf_sample__strval()
> if it's NULL.

Yeah, this is one of the areas where working with Sashiko is
difficult. The kwork code has memory handling issues, and each patch
series has revealed new warnings. I believe the issues are fixed in
v6, but as you point out, I haven't addressed the pre-existing memory
leaks. I don't think we need to overcomplicate fixing the evsel in
perf_sample issue this series is addressing.

Specifically the current code will assign to struct perf_kwork name
both a value within a sample:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-kwork.c?h=perf-tools-next#n1021
```
work->name = evsel__strval(evsel, sample, "name");
```

Or a strdup-ed value:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-kwork.c?h=perf-tools-next#n1152
```
work->name = evsel__softirq_name(evsel, num);
```
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-kwork.c?h=perf-tools-next#n1125
```
static char *evsel__softirq_name(struct evsel *evsel, u64 num)
...
name = strdup(sym->str);
...
return name;
```

The name's lifetime must match that of a sample, which seems
ridiculous given the ring buffer... will be overwritten. So in this
change, based on the Sashiko feedback, I've strdup-ed the names to
avoid the name pointing to memory within a sample that will change
when the typically stack allocated value disappears. Sashiko
originally complained about a lack of bounds checking, but then
complained about strdups of NULL, etc. At some point I have to stop
addressing every issue within the codebase that Sashiko raises, and
kwork seems to generate more of those than elsewhere.

I think it is pretty obvious the kwork memory handling at least needs
a free function for struct perf_kwork, this can then also free the
name, but refactoring and fixing kwork wasn't the point of this patch
series. We've just merged the tests I sent out to provide minimal
kwork coverage.

Thanks,
Ian