* [PATCH] pahole: Apply CU-level filters early in loading
@ 2024-07-30 22:43 Matthew Maurer
2024-07-31 8:57 ` Alan Maguire
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Maurer @ 2024-07-30 22:43 UTC (permalink / raw)
To: acme; +Cc: rust-for-linux, dwarves, aliceryhl, Matthew Maurer
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.
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);
+
+ /*
+ * 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) {
--
2.46.0.rc1.232.g9752f9e123-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] pahole: Apply CU-level filters early in loading
2024-07-30 22:43 [PATCH] pahole: Apply CU-level filters early in loading Matthew Maurer
@ 2024-07-31 8:57 ` Alan Maguire
2024-07-31 13:26 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Alan Maguire @ 2024-07-31 8:57 UTC (permalink / raw)
To: Matthew Maurer, acme; +Cc: rust-for-linux, dwarves, aliceryhl
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()?
> 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);
> +
> + /*
> + * 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) {
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] pahole: Apply CU-level filters early in loading
2024-07-31 8:57 ` Alan Maguire
@ 2024-07-31 13:26 ` Arnaldo Carvalho de Melo
2024-07-31 17:43 ` Alan Maguire
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-07-31 13:26 UTC (permalink / raw)
To: Alan Maguire; +Cc: Matthew Maurer, rust-for-linux, dwarves, aliceryhl
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) {
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] pahole: Apply CU-level filters early in loading
2024-07-31 13:26 ` Arnaldo Carvalho de Melo
@ 2024-07-31 17:43 ` Alan Maguire
2024-07-31 18:12 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Alan Maguire @ 2024-07-31 17:43 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Matthew Maurer, rust-for-linux, dwarves, aliceryhl
On 31/07/2024 14:26, Arnaldo Carvalho de Melo wrote:
> 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?
So if I understand correctly,
if (languages.exclude)
conf_load.early_cu_filter = cu__filter;
? Seems reasonable to me. Thanks!
Alan
> - 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); S
>
> 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) {
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] pahole: Apply CU-level filters early in loading
2024-07-31 17:43 ` Alan Maguire
@ 2024-07-31 18:12 ` Arnaldo Carvalho de Melo
2024-08-01 9:20 ` Alan Maguire
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-07-31 18:12 UTC (permalink / raw)
To: Alan Maguire; +Cc: Matthew Maurer, rust-for-linux, dwarves, aliceryhl
On Wed, Jul 31, 2024 at 06:43:51PM +0100, Alan Maguire wrote:
>
>
> On 31/07/2024 14:26, Arnaldo Carvalho de Melo wrote:
> > 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?
>
> So if I understand correctly,
>
> if (languages.exclude)
> conf_load.early_cu_filter = cu__filter;
>
> ? Seems reasonable to me. Thanks!
yeah, you got it. So this new early filtering is done for the feature we
have at hand and if other uses of cu__filter() already in place need, we
can transition to this new mechanism that reuses the cu__filter()
function signature/semantics.
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pahole: Apply CU-level filters early in loading
2024-07-31 18:12 ` Arnaldo Carvalho de Melo
@ 2024-08-01 9:20 ` Alan Maguire
0 siblings, 0 replies; 6+ messages in thread
From: Alan Maguire @ 2024-08-01 9:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Matthew Maurer, rust-for-linux, dwarves, aliceryhl
On 31/07/2024 19:12, Arnaldo Carvalho de Melo wrote:
> On Wed, Jul 31, 2024 at 06:43:51PM +0100, Alan Maguire wrote:
>>
>>
>> On 31/07/2024 14:26, Arnaldo Carvalho de Melo wrote:
>>> 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?
>>
>> So if I understand correctly,
>>
>> if (languages.exclude)
>> conf_load.early_cu_filter = cu__filter;
>>
>> ? Seems reasonable to me. Thanks!
>
> yeah, you got it. So this new early filtering is done for the feature we
> have at hand and if other uses of cu__filter() already in place need, we
> can transition to this new mechanism that reuses the cu__filter()
> function signature/semantics.
>
Sounds good! Matthew, can you send a v2 with the above change? It should
still cover us for BTF generation in the kernel case since we always
--lang_exclude rust as long as pahole is recent enough.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-08-01 9:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-30 22:43 [PATCH] pahole: Apply CU-level filters early in loading Matthew Maurer
2024-07-31 8:57 ` Alan Maguire
2024-07-31 13:26 ` Arnaldo Carvalho de Melo
2024-07-31 17:43 ` Alan Maguire
2024-07-31 18:12 ` Arnaldo Carvalho de Melo
2024-08-01 9:20 ` Alan Maguire
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).