Quick comments in advance of an updated patch.
On Wed, Oct 22, 2025, at 10:36 AM, Aaron Merey wrote:
>> + /* TODO: Is locking necessary? */
>
> Currently there is no locking in libebl itself and synchronization is
> handled by the calling library.
Ok, then I won't worry about this for now.
>> + ebl->cached_perf_regs_mask = perf_regs_mask;
>> + ebl->cached_regs_mapping = (int *)calloc (count, sizeof(int));
>
> I can't find a corresponding free for this calloc.
I can add one in eblclosebackend.c. I think that's the right place: the
ebl->destr stuff seems to be provision for per-architecture features.
>> + if ((bit & expected_mask) && (bit && perf_regs_mask))
>
> Is 'bit && perf_regs_masks' correct or should this use & instead?
Yes, good catch. Going to re-check the rest of that code -- since the bug
entailed in this is not triggered in the usual case of perf_regs_mask ==
expected_mask.
>> + {
>> + perf_to_regs[k] = j;
>> + j++;
>> + }
>> + else
>> + {
>> + perf_to_regs[k] = -1;
>> + }
>> + }
>> + assert (j <= (int)ebl->cached_n_regs_mapping);
>
> Returning false and setting dwfl_errno in the caller is better than
> terminating the process. We do have asserts present in other elfutils
> library code but we have been (slowly) replacing these.
Ok, will change that to a 'return false' as suggested.
>> int
>> -dwflst_perf_sample_getframes (Dwfl *dwfl, Elf *elf,
>> - pid_t pid, pid_t tid,
>> - const void *stack, size_t stack_size,
>> - const Dwarf_Word *regs, uint n_regs,
>> - uint64_t perf_regs_mask, uint abi,
>> - int (*callback) (Dwfl_Frame *state, void *arg),
>> - void *arg)
>> +dwflst_sample_getframes (Dwfl *dwfl, Elf *elf,
>> + pid_t pid, pid_t tid,
>> + const void *stack, size_t stack_size,
>> + const Dwarf_Word *regs, uint n_regs,
>> + const int *regs_mapping, size_t n_regs_mapping,
>> + int (*callback) (Dwfl_Frame *state, void *arg),
>> + void *arg)
>
> This is a nice improvement and helps keep the API generic. Do we
> still need dwflst_perf_sample_getframes in the public API? Can the
> dwflst_sample_getframes implementation figure out sample types or
> regs_mappings by itself?
>
> If not can we replace dwfl_perf_sample_getframes with a small helper
> function? Something like dwflst_get_perf_mappings that returns
> regs_mappings suitable for perf samples. If so, this work doesn't
> need to be done in this patch. This API is still "experimental" so we
> have flexibility to change this after the upcoming release.
Yeah, what you're describing is what the implementation of
dwflst_perf_sample_getframes() now contains: a call to
ebl_sample_perf_regs_mapping() followed by a call to dwflst_sample_getframes().
I thought that keeping dwflst_perf_sample_getframes() as a wrapper function was
neater, but we could instead publish a wrapper for
ebl_sample_perf_regs_mapping() taking Dwfl instead of Ebl and let the client
handle the boilerplate. Don't feel the argument in favour of that is
overwhelming, but I'm open to whatever helps settle the API as stable.
These comments + a couple of TODOs in dwflst_sample_frame.c, and I will have a
v2 of the patch. ETA ~soon.
--
All the best,
Serhei
http://serhei.io
