Re: [PATCH v1 bpf-next 15/18] resolve_btfids: Tag fmod_ret candidates as targets

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/11/06 09:50PM, bot+bpf-ci@kernel.org wrote:
> > resolve_btfids: Tag fmod_ret candidates as targets
> 
> TL;DR: Nits only in resolve_btfids fmodret.c: duplicated alias-walk code in
> mark_traceable_function(), its unused bool return, and possibly obsolete
> section names (.meminit, .cpuinit, .devinit) in section_cannot_be_attached().
> 
> > diff --git a/tools/bpf/resolve_btfids/fmodret.c b/tools/bpf/resolve_btfids/fmodret.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/bpf/resolve_btfids/fmodret.c
> 
> [ ... ]
> 
> > +static bool mark_traceable_function(struct elf_function_list *list,
> > +				    Elf64_Section shndx, GElf_Addr address,
> > +				    enum ftrace_site_kind kind)
> > +{
> > +	size_t left = 0;
> > +	size_t right = list->nr;
> > +	size_t first;
> > +	size_t i;
> > +	bool found = false;
> 
> [ ... ]
> 
> > +	if (first != SIZE_MAX) {
> > +		while (first && list->functions[first - 1].shndx == shndx &&
> > +		       list->functions[first - 1].value ==
> > +						list->functions[first].value)
> > +			first--;
> > +		for (i = first; i < list->nr &&
> > +		     list->functions[i].shndx == shndx &&
> > +		     list->functions[i].value ==
> > +					list->functions[first].value; i++) {
> > +			list->functions[i].traceable = true;
> > +			found = true;
> > +		}
> > +		return found;
> > +	}
> > +	if (kind == FTRACE_SITE_PATCHABLE || !left)
> > +		return false;
> > +
> > +	/* The nearest preceding function owns the call site. */
> > +	first = left - 1;
> > +	if (list->functions[first].shndx != shndx)
> > +		return false;
> > +	while (first && list->functions[first - 1].shndx == shndx &&
> > +	       list->functions[first - 1].value == list->functions[first].value)
> > +		first--;
> > +	for (i = first; i < list->nr &&
> > +	     list->functions[i].shndx == shndx &&
> > +	     list->functions[i].value == list->functions[first].value; i++) {
> > +		if (!function_contains(&list->functions[i], address))
> > +			continue;
> > +		list->functions[i].traceable = true;
> > +		found = true;
> > +	}
> > +	return found;
> > +}
> 
> This isn't a bug, but mark_traceable_function() has two near-identical blocks
> that walk back to the first alias at a (shndx, value) pair and then loop
> forward over the aliases.  The second differs only by the
> function_contains() test.
> 
> Could the walk back use function_at(list, shndx, list->functions[first].value),
> with one forward loop that takes the containment test as a condition?  Then
> the two copies would not need to be kept in sync.
> 
> Also, this isn't a bug, but is the bool return of mark_traceable_function()
> meant for something, such as a -v count of unmatched ftrace sites?  None of
> its callers in mark_relocated_ftrace_sites() and read_ftrace_site_range()
> read the value, so could it return void and drop 'found'?

Both makes sense, will fix.

> [ ... ]
> 
> > +static bool section_cannot_be_attached(const char *name)
> > +{
> > +	return section_prefix(name, ".init.text") ||
> > +	       section_prefix(name, ".exit.text") ||
> > +	       section_prefix(name, ".noinstr.text") ||
> > +	       section_prefix(name, ".meminit.text") ||
> > +	       section_prefix(name, ".memexit.text") ||
> > +	       section_prefix(name, ".cpuinit.text") ||
> > +	       section_prefix(name, ".cpuexit.text") ||
> > +	       section_prefix(name, ".devinit.text") ||
> > +	       section_prefix(name, ".devexit.text");
> > +}
> 
> This isn't a bug, but are .meminit.text, .memexit.text, .cpuinit.text,
> .cpuexit.text, .devinit.text and .devexit.text still emitted anywhere?
> 
> In include/linux/init.h, __meminit is now either empty or __init, and the
> __cpuinit/__devinit annotations were removed.  Neither vmlinux.lds.h nor
> scripts/module.lds.S places these sections, and modpost lists only
> .init.text and .exit.text as init/exit text.
> 
> If they are gone, could section_cannot_be_attached() be reduced to
> .init.text, .exit.text and .noinstr.text?
> 

Makes sense, will fix. Thanks robot.

> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42294987431