Re: [PATCH] pahole: Apply CU-level filters early in loading

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Wed, Jul 31, 2024 at 09:57:25AM +0100, Alan Maguire wrote:
> On 30/07/2024 23:43, Matthew Maurer wrote:
> > Without this, even with `--lang_exclude=rust` set, running on `vmlinux`
> > with `CONFIG_RUST` enabled will lead to errors like:
> > die__process_function: tag not supported 0x2f (template_type_parameter)!
> > because the filtering doesn't happen until finalization, but unsupported
> > tags are reported during loading.
> > 
> > As an added bonus, this should speed up processing of large objects with
> > filtered CUs, as their details will no longer be walked.
> >
> 
> One question on this; if we are always doing early filtering like this,
> should the explicit cu__filter() call be removed from pahole_stealer()?

When I saw the introduction of an extra callback to be used inside the
dwarf_loader I thought that it would be used only for this specific
language filtering feature, i.e. a defensive approach at implementing
this to avoid unintended side effects of doing all filtering at that
point, maybe some other feature somehow depends on the cu__filter()
being called where it was so far.

But then it is being used for all filtering, so it seems just a way to
reduce the patch size...

So I'd keep the cu->early_cu_filter() but would use it only for the
language filtering feature, wdyt?

- Arnaldo
 
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > ---
> >  dwarf_loader.c | 10 ++++++++++
> >  dwarves.h      |  1 +
> >  pahole.c       |  1 +
> >  3 files changed, 12 insertions(+)
> > 
> > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > index b832c93..c48dfef 100644
> > --- a/dwarf_loader.c
> > +++ b/dwarf_loader.c
> > @@ -2854,6 +2854,16 @@ static int die__process(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
> >  
> >  	cu->language = attr_numeric(die, DW_AT_language);
> >  
> > +	if (conf->early_cu_filter)
> > +		cu = (conf->early_cu_filter)(cu);

Also we can have it more compactly as:

+	if (conf->early_cu_filter)
+		cu = conf->early_cu_filter(cu);

No?

> > +
> > +	/*
> > +	 * If we filtered this CU out, we still want to keep iterating, but
> > +	 * there's no need to walk the rest of the CU info.
> > +	 */
> > +	if (cu == NULL)
> > +		return DWARF_CB_OK;
> > +
> >  	if (dwarf_child(die, &child) == 0) {
> >  		int err = die__process_unit(&child, cu, conf);
> >  		if (err)
> > diff --git a/dwarves.h b/dwarves.h
> > index f5ae79f..92d102b 100644
> > --- a/dwarves.h
> > +++ b/dwarves.h
> > @@ -72,6 +72,7 @@ struct conf_load {
> >  	enum load_steal_kind	(*steal)(struct cu *cu,
> >  					 struct conf_load *conf,
> >  					 void *thr_data);
> > +	struct cu *		(*early_cu_filter)(struct cu *cu);
> >  	int			(*thread_exit)(struct conf_load *conf, void *thr_data);
> >  	void			*cookie;
> >  	char			*format_path;
> > diff --git a/pahole.c b/pahole.c
> > index 954498d..937b0a1 100644
> > --- a/pahole.c
> > +++ b/pahole.c
> > @@ -3765,6 +3765,7 @@ int main(int argc, char *argv[])
> >  	memset(tab, ' ', sizeof(tab) - 1);
> >  
> >  	conf_load.steal = pahole_stealer;
> > +	conf_load.early_cu_filter = cu__filter;
> >  	conf_load.thread_exit = pahole_thread_exit;
> >  
> >  	if (conf_load.reproducible_build) {