All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH dwarves] pahole: --unions shouldn't bypass struct-only options
@ 2026-08-31 18:56 Aditya Dutt
  2026-08-31 21:35 ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 3+ messages in thread
From: Aditya Dutt @ 2026-08-31 18:56 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Aditya Dutt, dwarves, Alan Maguire

class__filter() returns early for --unions before the guard for struct
only options. This results in calling functions like
print_packable_info(), which reads c->priv which doesn't exist for
unions.

  $ pahole --unions --packable m
  Segmentation fault (core dumped)

The early return also prevents other struct only option filtering.
                             before             after
  --unions --packable        segfault           (nothing)
  --unions -H 1              union u_named {}   (nothing)
  --unions --bit_holes 1     union u_named {}   (nothing)
  --unions --hole_size_ge 1  u_named            (nothing)
  --unions --padding_ge 1    union u_named {}   (nothing)
  --with_flexible_array      union u_named {}   (nothing)

These don't make sense for a union: all of its members start at offset 0,
so there are no holes to count or pack, C doesn't allow a flexible array
member in one. nr_holes, padding and has_flexible_array only exist in
'struct class', so there is nowhere to keep an answer either.

--with_flexible_array and --with_embedded_flexible_array were missing from the
struct only list, so add them too.

Fixes: 3661f17d0b2cd56b ("pahole: Introduce --unions to consider just unions")
Signed-off-by: Aditya Dutt <duttaditya18@gmail.com>
---

There are a few more CLI bugs and will be sending patches for them
soon: 'pahole -C <type> -T' segfaults etc.

Are there other things related to pahole I can contribute to? I would
also like to help with the Rust support if there is something useful I
can pick up.

 pahole.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/pahole.c b/pahole.c
index a1b3d4a..c7daa31 100644
--- a/pahole.c
+++ b/pahole.c
@@ -462,6 +462,11 @@ static void class_formatter(struct class *class, struct cu *cu, uint32_t id)
 static void print_packable_info(struct class *c, struct cu *cu, uint32_t id)
 {
 	const struct tag *t = class__tag(c);
+
+	/* unions are not packable */
+	if (!tag__is_struct(t))
+		return;
+
 	const size_t orig_size = class__size(c);
 	const size_t new_size = class__size(c->priv);
 	const size_t savings = orig_size - new_size;
@@ -716,12 +721,6 @@ static struct class *class__filter(struct class *class, struct cu *cu,
 	     strncmp(decl_exclude_prefix, tag__decl_file(tag, cu),
 		     decl_exclude_prefix_len) == 0))
 		return NULL;
-	/*
-	 * if --unions was used and we got here, its a union and we satisfy the other
-	 * filters/options, so don't filter it.
-	 */
-	if (just_unions)
-		return class;
 	/*
 	 * The following only make sense for structs, i.e. 'struct class',
 	 * and as we can get here with a union, that is represented by a 'struct type',
@@ -730,7 +729,8 @@ static struct class *class__filter(struct class *class, struct cu *cu,
 	 */
 	if (!tag__is_struct(tag))
 		return (just_structs || show_packable || nr_holes || nr_bit_holes || hole_size_ge ||
-			end_padding_ge || end_padding) ? NULL : class;
+			end_padding_ge || end_padding || show_with_flexible_array ||
+			show_with_embedded_flexible_array) ? NULL : class;
 
 	if (tag->top_level)
 		class__find_holes(class);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH dwarves] pahole: --unions shouldn't bypass struct-only options
  2026-08-31 18:56 [PATCH dwarves] pahole: --unions shouldn't bypass struct-only options Aditya Dutt
@ 2026-08-31 21:35 ` Arnaldo Carvalho de Melo
  2026-09-01 13:21   ` Aditya Dutt
  0 siblings, 1 reply; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-08-31 21:35 UTC (permalink / raw)
  To: Aditya Dutt; +Cc: dwarves, Alan Maguire

On Tue, Sep 01, 2026 at 12:26:41AM +0530, Aditya Dutt wrote:
> class__filter() returns early for --unions before the guard for struct
> only options. This results in calling functions like
> print_packable_info(), which reads c->priv which doesn't exist for
> unions.
> 
>   $ pahole --unions --packable m
>   Segmentation fault (core dumped)

Yeah, reproduced with the default use of /sys/kernel/btf/vmlinux:

acme@number:~/git/pahole$ pahole --unions --packable
Segmentation fault         (core dumped) pahole --unions --packable
acme@number:~/git/pahole$

Please split this into multiple patches, so that we can cherry pick
independent patches when we have some issue with one of the patches in
the series.
 
> The early return also prevents other struct only option filtering.
>                              before             after
>   --unions --packable        segfault           (nothing)
>   --unions -H 1              union u_named {}   (nothing)
>   --unions --bit_holes 1     union u_named {}   (nothing)
>   --unions --hole_size_ge 1  u_named            (nothing)
>   --unions --padding_ge 1    union u_named {}   (nothing)
>   --with_flexible_array      union u_named {}   (nothing)
> 
> These don't make sense for a union: all of its members start at offset 0,
> so there are no holes to count or pack, C doesn't allow a flexible array
> member in one. nr_holes, padding and has_flexible_array only exist in
> 'struct class', so there is nowhere to keep an answer either.

I can imagine looking for an union with bit holes:

union x86_pmu_config {
	struct {
		u64                event:8;            /*     0: 0  8 */
		u64                umask:8;            /*     0: 8  8 */
		u64                usr:1;              /*     0:16  8 */
		u64                os:1;               /*     0:17  8 */
		u64                edge:1;             /*     0:18  8 */
		u64                pc:1;               /*     0:19  8 */
		u64                interrupt:1;        /*     0:20  8 */
		u64                __reserved1:1;      /*     0:21  8 */
		u64                en:1;               /*     0:22  8 */
		u64                inv:1;              /*     0:23  8 */
		u64                cmask:8;            /*     0:24  8 */
		u64                event2:4;           /*     0:32  8 */
		u64                __reserved2:4;      /*     0:36  8 */
		u64                go:1;               /*     0:40  8 */
		u64                ho:1;               /*     0:41  8 */
	} bits;                                        /*     0     8 */
	u64                        value;              /*     0     8 */
};

The sum of that bitfield is 42 bits, so we have a "padding"/hole of 22
bits in that union, can't see quickly a use fase for this right now, but
maybe someone can have this corner case need?
 
> --with_flexible_array and --with_embedded_flexible_array were missing from the
> struct only list, so add them too.

Well spotted, please put this in a separate patch, with the
corresponding fix.

You added a fixes tag, and it is the right one, but for the segfault and
maybe some of the other cases that don't make sense (didn't look at all
of them in detail).

> Fixes: 3661f17d0b2cd56b ("pahole: Introduce --unions to consider just unions")

I have a local branch with cli fixes:

a2aea4b8ccd0fa58 (topic/cli-fixes) tests: Add compile emission and word size tests
b97f6e88676cb21e tests: Add struct packing and reorganization test
62595f6b42d3b776 tests: Add pahole --sort, word_size unions and --prettify bitfields test
1b0c903b0244cdf8 tests: Add recursive container search and union word_size tests
96380358ca13e286 tests: Add pahole --count, --skip, --structs, --contains and --first_obj_only test
ebe2ef62649bc3d9 pahole: Make --show_reorg_steps imply --reorganize
cd0420cb8108a221 pahole: Make --hex apply to --sizes and --packable output
7c7aaea07a513909 pahole: Make --skip work with all display modes
e60e8e0a48445f45 pahole: Make --count work with all display modes
0c97000781bd13c6 pahole: Make --word_size work with all display modes
9964f22ecd6429ec pahole: Fix --word_size resize to use base_type__is_word_size_dependent()
87cfa639dd40c155 dwarves: Add base_type__is_word_size_dependent() helper
acme@number:~/git/pahole$

Need to publish it on a temp branch

But yeah, we're interested in fixes, feel free to send them to the
mailing list and we'll try to provide feedback and go on merging the
ones we agree with,

Thanks!

- Arnaldo

> Signed-off-by: Aditya Dutt <duttaditya18@gmail.com>
> ---
> 
> There are a few more CLI bugs and will be sending patches for them
> soon: 'pahole -C <type> -T' segfaults etc.
> 
> Are there other things related to pahole I can contribute to? I would
> also like to help with the Rust support if there is something useful I
> can pick up.
> 
>  pahole.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/pahole.c b/pahole.c
> index a1b3d4a..c7daa31 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -462,6 +462,11 @@ static void class_formatter(struct class *class, struct cu *cu, uint32_t id)
>  static void print_packable_info(struct class *c, struct cu *cu, uint32_t id)
>  {
>  	const struct tag *t = class__tag(c);
> +
> +	/* unions are not packable */
> +	if (!tag__is_struct(t))
> +		return;
> +
>  	const size_t orig_size = class__size(c);
>  	const size_t new_size = class__size(c->priv);
>  	const size_t savings = orig_size - new_size;
> @@ -716,12 +721,6 @@ static struct class *class__filter(struct class *class, struct cu *cu,
>  	     strncmp(decl_exclude_prefix, tag__decl_file(tag, cu),
>  		     decl_exclude_prefix_len) == 0))
>  		return NULL;
> -	/*
> -	 * if --unions was used and we got here, its a union and we satisfy the other
> -	 * filters/options, so don't filter it.
> -	 */
> -	if (just_unions)
> -		return class;
>  	/*
>  	 * The following only make sense for structs, i.e. 'struct class',
>  	 * and as we can get here with a union, that is represented by a 'struct type',
> @@ -730,7 +729,8 @@ static struct class *class__filter(struct class *class, struct cu *cu,
>  	 */
>  	if (!tag__is_struct(tag))
>  		return (just_structs || show_packable || nr_holes || nr_bit_holes || hole_size_ge ||
> -			end_padding_ge || end_padding) ? NULL : class;
> +			end_padding_ge || end_padding || show_with_flexible_array ||
> +			show_with_embedded_flexible_array) ? NULL : class;
>  
>  	if (tag->top_level)
>  		class__find_holes(class);
> -- 
> 2.34.1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH dwarves] pahole: --unions shouldn't bypass struct-only options
  2026-08-31 21:35 ` Arnaldo Carvalho de Melo
@ 2026-09-01 13:21   ` Aditya Dutt
  0 siblings, 0 replies; 3+ messages in thread
From: Aditya Dutt @ 2026-09-01 13:21 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Aditya Dutt, dwarves, Alan Maguire

On Mon, Aug 31, 2026 at 06:35:16PM -0300, Arnaldo Carvalho de Melo wrote:
> Please split this into multiple patches, so that we can cherry pick
> independent patches when we have some issue with one of the patches in
> the series.

I'll split this change to 3 patches.
1. Fixing only the segfault by adding the guard at the top of
print_packable_info. Contains the Fixes tag.
2. --with_flexible_array and --with_embedded_flexible_array guard if !struct.
3. In the case of unions, don't ignore struct-only filters.

Is this good, so that whatever is needed can be picked up?

> I can imagine looking for an union with bit holes:
>
> union x86_pmu_config {
> 	struct {
> 		u64                event:8;            /*     0: 0  8 */
> 		u64                umask:8;            /*     0: 8  8 */
> 		u64                usr:1;              /*     0:16  8 */
> 		u64                os:1;               /*     0:17  8 */
> 		u64                edge:1;             /*     0:18  8 */
> 		u64                pc:1;               /*     0:19  8 */
> 		u64                interrupt:1;        /*     0:20  8 */
> 		u64                __reserved1:1;      /*     0:21  8 */
> 		u64                en:1;               /*     0:22  8 */
> 		u64                inv:1;              /*     0:23  8 */
> 		u64                cmask:8;            /*     0:24  8 */
> 		u64                event2:4;           /*     0:32  8 */
> 		u64                __reserved2:4;      /*     0:36  8 */
> 		u64                go:1;               /*     0:40  8 */
> 		u64                ho:1;               /*     0:41  8 */
> 	} bits;                                        /*     0     8 */
> 	u64                        value;              /*     0     8 */
> };
>
> The sum of that bitfield is 42 bits, so we have a "padding"/hole of 22
> bits in that union, can't see quickly a use fase for this right now, but
> maybe someone can have this corner case need?

You can already get that using 'pahole -a -A' (both flags are needed).
For example:

	$ cat /tmp/a.c
	typedef unsigned long long u64;
	union u_hole {
		struct {
			u64 a:8;
			u64 b:8;
			u64 c:1;
		} bits;
		u64 value;
	};
	union u_hole u;
	$ gcc -c -g /tmp/a.c -o /tmp/a.o
	$ pahole -a -A /tmp/a.o
	struct {
		u64                        a:8;                  /*     0: 0  8 */
		u64                        b:8;                  /*     0: 8  8 */
		u64                        c:1;                  /*     0:16  8 */

		/* size: 8, cachelines: 1, members: 3 */
		/* bit_padding: 47 bits */
		/* last cacheline: 8 bytes */
	};
	union u_hole {
		struct {
			u64                a:8;                /*     0: 0  8 */
			u64                b:8;                /*     0: 8  8 */
			u64                c:1;                /*     0:16  8 */
		} bits;                                        /*     0     8 */
		u64                        value;              /*     0     8 */
	};

The padding is for the anonymous struct and not the union itself.
Does it make sense for '--unions' to show the results for the anonymous
structs inside a union?

Maybe a '--recursive' modifier is better? Which looks inside
structs/unions and unions when applying the filters, so that the outer
struct/union returned if a filter applies to any inner struct/union.

> > --with_flexible_array and --with_embedded_flexible_array were missing from the
> > struct only list, so add them too.
>
> Well spotted, please put this in a separate patch, with the
> corresponding fix.

Same thing here, a union cannot have a flexible array directly but a
union may have a struct inside containing a flexible array. Maybe a
'--recursive' modifier makes sense? And optionally, you can specify the
depth to which we search '--recursive=2' etc.

-- 

Thanks,
Aditya Dutt

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-09-01 13:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-31 18:56 [PATCH dwarves] pahole: --unions shouldn't bypass struct-only options Aditya Dutt
2026-08-31 21:35 ` Arnaldo Carvalho de Melo
2026-09-01 13:21   ` Aditya Dutt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.