[PATCH 1/2] fprintf: Fix nested struct printing

[PATCH 1/2] fprintf: Fix nested struct printing

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

This code:

    struct X {
       struct {
       } __attribute__((foo)) x __attribute__((bar));
    }

Was wrongly printed as:

    struct X {
       struct {
       } x __attribute__((foo)) __attribute__((bar));
    }

This unfortunately matters a lot, since "bar" is suppose to apply to
"x", but "foo" to typeof(x). In the wrong form, both apply to "x",
leading to e.g. incorrect layout for __aligned__ attribute.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 dwarves_fprintf.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index c35868a..1c1d949 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -787,7 +787,7 @@ print_default:
 					   (type->tag == DW_TAG_class_type &&
 					    !tconf.classes_as_structs) ? "class" : "struct",
 					   tconf.type_spacing - 7,
-					   type__name(ctype), name);
+					   type__name(ctype), name ?: "");
 		} else {
 			struct class *cclass = tag__class(type);
 
@@ -802,7 +802,7 @@ print_default:
 		ctype = tag__type(type);
 
 		if (type__name(ctype) != NULL && !expand_types) {
-			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name);
+			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name ?: "");
 		} else {
 			tconf.type_spacing -= 8;
 			printed += union__fprintf(ctype, cu, &tconf, fp);
@@ -812,7 +812,7 @@ print_default:
 		ctype = tag__type(type);
 
 		if (type__name(ctype) != NULL)
-			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name);
+			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name ?: "");
 		else
 			printed += enumeration__fprintf(type, &tconf, fp);
 		break;
@@ -863,7 +863,21 @@ static size_t class_member__fprintf(struct class_member *member, bool union_memb
 	if (member->is_static)
 		printed += fprintf(fp, "static ");
 
-	printed += type__fprintf(type, cu, name, &sconf, fp);
+	/* For struct-like constructs, the name of the member cannot be
+	 * conflated with the name of its type, otherwise __attribute__ are
+	 * printed in the wrong order.
+	 */
+	if (tag__is_union(type) || tag__is_struct(type) ||
+	    tag__is_enumeration(type)) {
+		printed += type__fprintf(type, cu, NULL, &sconf, fp);
+		if (name) {
+			if (!type__name(tag__type(type)))
+				printed += fprintf(fp, " ");
+			printed += fprintf(fp, "%s", name);
+		}
+	} else {
+		printed += type__fprintf(type, cu, name, &sconf, fp);
+	}
 
 	if (member->is_static) {
 		if (member->const_value != 0)
-- 
2.25.1

[PATCH 2/2] btf_loader.c: Infer alignment info

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

BTF does not carry alignment information, but it carries the offset in
structs. This allows inferring the original alignment, yielding a C
header dump that is not identical to the original C code, but is
guaranteed to lead to the same memory layout.

This allows using the output of pahole in another program to poke at
memory, with the assurance that we will not read garbage.

Note: Since the alignment is inferred from the offset, it sometimes
happens that the offset was already correctly aligned, which means the
inferred alignment will be smaller than in the original source. This
does not impact the ability to read existing structs, but it could
impact creating such struct if other client code expects higher
alignment than the one exposed in the generated header.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 btf_loader.c | 66 ++++++++++++++++++++++++++++++++++++++++------------
 dwarves.c    |  2 +-
 dwarves.h    |  2 ++
 3 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index 3e5a945..54353d7 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -471,10 +471,37 @@ static int btf__load_sections(struct btf *btf, struct cu *cu)
 	return btf__load_types(btf, cu);
 }
 
+static uint32_t class__infer_alignment(uint32_t byte_offset,
+				       uint32_t natural_alignment,
+				       uint32_t smallest_offset)
+{
+	uint32_t alignment = 0;
+	uint32_t offset_delta = byte_offset - smallest_offset;
+
+	if (offset_delta) {
+		if (byte_offset % 2 == 0) {
+			/* Find the power of 2 immediately higher than
+			 * offset_delta
+			 */
+			alignment = 1 << (8 * sizeof(offset_delta) -
+					      __builtin_clz(offset_delta));
+		} else {
+			alignment = 0;
+		}
+	}
+
+	/* Natural alignment, nothing to do */
+	if (alignment <= natural_alignment || alignment == 1)
+		alignment = 0;
+
+	return alignment;
+}
+
 static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
 {
 	struct class_member *pos;
 	struct type *tag_type = tag__type(tag);
+	uint32_t smallest_offset = 0;
 
 	type__for_each_data_member(tag_type, pos) {
 		struct tag *type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
@@ -486,31 +513,39 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
 		pos->byte_size = tag__size(type, cu);
 		pos->bit_size = pos->byte_size * 8;
 
-		/* bitfield fixup is needed for enums and base types only */
-		if (type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type)
-			continue;
-
 		/* if BTF data is incorrect and has size == 0, skip field,
 		 * instead of crashing */
 		if (pos->byte_size == 0) {
 			continue;
 		}
 
-		if (pos->bitfield_size) {
-			/* bitfields seem to be always aligned, no matter the packing */
-			pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
-			pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
-			/* re-adjust bitfield offset if it is negative */
-			if (pos->bitfield_offset < 0) {
-				pos->bitfield_offset += pos->bit_size;
-				pos->byte_offset -= pos->byte_size;
-				pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
+		/* bitfield fixup is needed for enums and base types only */
+		if (type->tag == DW_TAG_base_type || type->tag == DW_TAG_enumeration_type) {
+			if (pos->bitfield_size) {
+				/* bitfields seem to be always aligned, no matter the packing */
+				pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
+				pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
+				/* re-adjust bitfield offset if it is negative */
+				if (pos->bitfield_offset < 0) {
+					pos->bitfield_offset += pos->bit_size;
+					pos->byte_offset -= pos->byte_size;
+					pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
+				}
+			} else {
+				pos->byte_offset = pos->bit_offset / 8;
 			}
-		} else {
-			pos->byte_offset = pos->bit_offset / 8;
 		}
+
+		pos->alignment = class__infer_alignment(pos->byte_offset,
+							tag__natural_alignment(type, cu),
+							smallest_offset);
+		smallest_offset = pos->byte_offset + pos->byte_size;
 	}
 
+	tag_type->alignment = class__infer_alignment(tag_type->size,
+						     tag__natural_alignment(tag, cu),
+						     smallest_offset);
+
 	return 0;
 }
 
@@ -519,6 +554,7 @@ static int cu__fixup_btf_bitfields(struct cu *cu)
 	int err = 0;
 	struct tag *pos;
 
+
 	list_for_each_entry(pos, &cu->tags, node)
 		if (tag__is_struct(pos) || tag__is_union(pos)) {
 			err = class__fixup_btf_bitfields(pos, cu);
diff --git a/dwarves.c b/dwarves.c
index b6f2489..bb8af5b 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -1515,7 +1515,7 @@ void class__find_holes(struct class *class)
 
 static size_t type__natural_alignment(struct type *type, const struct cu *cu);
 
-static size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
+size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
 {
 	size_t natural_alignment = 1;
 
diff --git a/dwarves.h b/dwarves.h
index 30d33fa..c2fea0a 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -1002,6 +1002,8 @@ struct type {
 
 void __type__init(struct type *type);
 
+size_t tag__natural_alignment(struct tag *tag, const struct cu *cu);
+
 static inline struct class *type__class(const struct type *type)
 {
 	return (struct class *)type;
-- 
2.25.1

Re: [PATCH 1/2] fprintf: Fix nested struct printing

[Arnaldo Carvalho de Melo]

Em Thu, Oct 14, 2021 at 12:48:49PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> This code:
> 
>     struct X {
>        struct {
>        } __attribute__((foo)) x __attribute__((bar));
>     }
> 
> Was wrongly printed as:
> 
>     struct X {
>        struct {
>        } x __attribute__((foo)) __attribute__((bar));
>     }
> 
> This unfortunately matters a lot, since "bar" is suppose to apply to
> "x", but "foo" to typeof(x). In the wrong form, both apply to "x",
> leading to e.g. incorrect layout for __aligned__ attribute.

I couldn't quickly find a change in behaviour with a random vmlinux I
have for testing:

⬢[acme@toolbox pahole]$ build/pahole.old -F dwarf vmlinux > before
⬢[acme@toolbox pahole]$ build/pahole -F dwarf vmlinux > after
⬢[acme@toolbox pahole]$ diff -u before after
⬢[acme@toolbox pahole]$ grep "__attribute__.*__attribute__" before  | wc -l
40
⬢[acme@toolbox pahole]$

btdiff is happy tho:

⬢[acme@toolbox pahole]$ btfdiff vmlinux
⬢[acme@toolbox pahole]$ cat btfdiff
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0-only
# Copyright © 2019 Red Hat Inc, Arnaldo Carvalho de Melo <acme@redhat.com>
# Use pahole to produce output from BTF and from DWARF, then do a diff
# Use --flat_arrays with DWARF as BTF, like CTF, flattens arrays.
# Use --show_private_classes as BTF shows all structs, while pahole knows
# if some struct is defined only inside another struct/class or in a function,
# this information is not available when loading from BTF.

if [ $# -eq 0 ] ; then
	echo "Usage: btfdiff <filename_with_DWARF_and_maybe_BTF_info> [<filename_with_BTF_info>]"
	exit 1
fi

dwarf_input=$1
btf_input=$dwarf_input

if [ $# -eq 2 ] ; then
	btf_input=$2
fi

btf_output=$(mktemp /tmp/btfdiff.btf.XXXXXX)
dwarf_output=$(mktemp /tmp/btfdiff.dwarf.XXXXXX)
pahole_bin=${PAHOLE-"pahole"}

${pahole_bin} -F dwarf \
	      --flat_arrays \
	      --sort \
	      --jobs \
	      --suppress_aligned_attribute \
	      --suppress_force_paddings \
	      --suppress_packed \
	      --show_private_classes $dwarf_input > $dwarf_output
${pahole_bin} -F btf \
	      --sort \
	      --suppress_packed \
	      $btf_input > $btf_output

diff -up $dwarf_output $btf_output

rm -f $btf_output $dwarf_output
exit 0
⬢[acme@toolbox pahole]$

As is fullcircle:

⬢[acme@toolbox pahole]$ fullcircle tcp.o
⬢[acme@toolbox pahole]$ cat fullcircle
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0-only
# Copyright © 2019 Red Hat Inc, Arnaldo Carvalho de Melo <acme@redhat.com>
# Use pfunct to produce compilable output from a object, then do a codiff -s
# To see if the type information generated from source code generated
# from type information in a file compiled from the original source code matches.

if [ $# -eq 0 ] ; then
	echo "Usage: fullcircle <filename_with_type_info>"
	exit 1
fi

file=$1

nr_cus=$(readelf -wi ${file} | grep DW_TAG_compile_unit | wc -l)
if [ $nr_cus -gt 1 ]; then
	exit 0
fi

c_output=$(mktemp /tmp/fullcircle.XXXXXX.c)
o_output=$(mktemp /tmp/fullcircle.XXXXXX.o)
pfunct_bin=${PFUNCT-"pfunct"}
codiff_bin=${CODIFF-"codiff"}

# See how your DW_AT_producer looks like and find the
# right regexp to get after the GCC version string, this one
# seems good enough for Red Hat/Fedora/CentOS that look like:
#
#   DW_AT_producer    : (indirect string, offset: 0x3583): GNU C89 8.2.1 20181215 (Red Hat 8.2.1-6) -mno-sse -mno-mmx
#
# So we need from -mno-sse onwards

CFLAGS=$(readelf -wi $file | grep -w DW_AT_producer | sed -r      's/.*\)( -[[:alnum:]]+.*)+/\1/g')

# Check if we managed to do the sed or if this is something like GNU AS
[ "${CFLAGS/DW_AT_producer/}" != "${CFLAGS}" ] && exit

${pfunct_bin} --compile $file > $c_output
gcc $CFLAGS -c -g $c_output -o $o_output
${codiff_bin} -q -s $file $o_output

rm -f $c_output $o_output
exit 0
⬢[acme@toolbox pahole]$

- Arnaldo
 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  dwarves_fprintf.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index c35868a..1c1d949 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -787,7 +787,7 @@ print_default:
>  					   (type->tag == DW_TAG_class_type &&
>  					    !tconf.classes_as_structs) ? "class" : "struct",
>  					   tconf.type_spacing - 7,
> -					   type__name(ctype), name);
> +					   type__name(ctype), name ?: "");
>  		} else {
>  			struct class *cclass = tag__class(type);
>  
> @@ -802,7 +802,7 @@ print_default:
>  		ctype = tag__type(type);
>  
>  		if (type__name(ctype) != NULL && !expand_types) {
> -			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name);
> +			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name ?: "");
>  		} else {
>  			tconf.type_spacing -= 8;
>  			printed += union__fprintf(ctype, cu, &tconf, fp);
> @@ -812,7 +812,7 @@ print_default:
>  		ctype = tag__type(type);
>  
>  		if (type__name(ctype) != NULL)
> -			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name);
> +			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name ?: "");
>  		else
>  			printed += enumeration__fprintf(type, &tconf, fp);
>  		break;
> @@ -863,7 +863,21 @@ static size_t class_member__fprintf(struct class_member *member, bool union_memb
>  	if (member->is_static)
>  		printed += fprintf(fp, "static ");
>  
> -	printed += type__fprintf(type, cu, name, &sconf, fp);
> +	/* For struct-like constructs, the name of the member cannot be
> +	 * conflated with the name of its type, otherwise __attribute__ are
> +	 * printed in the wrong order.
> +	 */
> +	if (tag__is_union(type) || tag__is_struct(type) ||
> +	    tag__is_enumeration(type)) {
> +		printed += type__fprintf(type, cu, NULL, &sconf, fp);
> +		if (name) {
> +			if (!type__name(tag__type(type)))
> +				printed += fprintf(fp, " ");
> +			printed += fprintf(fp, "%s", name);
> +		}
> +	} else {
> +		printed += type__fprintf(type, cu, name, &sconf, fp);
> +	}
>  
>  	if (member->is_static) {
>  		if (member->const_value != 0)
> -- 
> 2.25.1

-- 

- Arnaldo

Re: [PATCH 2/2] btf_loader.c: Infer alignment info

[Arnaldo Carvalho de Melo]

Em Thu, Oct 14, 2021 at 12:48:50PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
 
> BTF does not carry alignment information, but it carries the offset in
> structs. This allows inferring the original alignment, yielding a C
> header dump that is not identical to the original C code, but is
> guaranteed to lead to the same memory layout.
 
> This allows using the output of pahole in another program to poke at
> memory, with the assurance that we will not read garbage.

Which is super cool, thanks for working on this.

Please take a look at the 'fullcircle' tool in the pahole repo, I once
used it on all the single CU .o files in the kernel build to make sure
that we generate a source code and then object file that matches the
original object file (and thus its .c file), at least in layouts.

Doing it with BTF too after your change would be fantastic.

Now to review your patch, at first it looks big, but perhaps its
inherent to the change and we can't break it into smaller steps, will
see.

- Arnaldo
 
> Note: Since the alignment is inferred from the offset, it sometimes
> happens that the offset was already correctly aligned, which means the
> inferred alignment will be smaller than in the original source. This
> does not impact the ability to read existing structs, but it could
> impact creating such struct if other client code expects higher
> alignment than the one exposed in the generated header.
> 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  btf_loader.c | 66 ++++++++++++++++++++++++++++++++++++++++------------
>  dwarves.c    |  2 +-
>  dwarves.h    |  2 ++
>  3 files changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index 3e5a945..54353d7 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -471,10 +471,37 @@ static int btf__load_sections(struct btf *btf, struct cu *cu)
>  	return btf__load_types(btf, cu);
>  }
>  
> +static uint32_t class__infer_alignment(uint32_t byte_offset,
> +				       uint32_t natural_alignment,
> +				       uint32_t smallest_offset)
> +{
> +	uint32_t alignment = 0;
> +	uint32_t offset_delta = byte_offset - smallest_offset;
> +
> +	if (offset_delta) {
> +		if (byte_offset % 2 == 0) {
> +			/* Find the power of 2 immediately higher than
> +			 * offset_delta
> +			 */
> +			alignment = 1 << (8 * sizeof(offset_delta) -
> +					      __builtin_clz(offset_delta));
> +		} else {
> +			alignment = 0;
> +		}
> +	}
> +
> +	/* Natural alignment, nothing to do */
> +	if (alignment <= natural_alignment || alignment == 1)
> +		alignment = 0;
> +
> +	return alignment;
> +}
> +
>  static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>  {
>  	struct class_member *pos;
>  	struct type *tag_type = tag__type(tag);
> +	uint32_t smallest_offset = 0;
>  
>  	type__for_each_data_member(tag_type, pos) {
>  		struct tag *type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
> @@ -486,31 +513,39 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>  		pos->byte_size = tag__size(type, cu);
>  		pos->bit_size = pos->byte_size * 8;
>  
> -		/* bitfield fixup is needed for enums and base types only */
> -		if (type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type)
> -			continue;
> -
>  		/* if BTF data is incorrect and has size == 0, skip field,
>  		 * instead of crashing */
>  		if (pos->byte_size == 0) {
>  			continue;
>  		}
>  
> -		if (pos->bitfield_size) {
> -			/* bitfields seem to be always aligned, no matter the packing */
> -			pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
> -			pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
> -			/* re-adjust bitfield offset if it is negative */
> -			if (pos->bitfield_offset < 0) {
> -				pos->bitfield_offset += pos->bit_size;
> -				pos->byte_offset -= pos->byte_size;
> -				pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
> +		/* bitfield fixup is needed for enums and base types only */
> +		if (type->tag == DW_TAG_base_type || type->tag == DW_TAG_enumeration_type) {
> +			if (pos->bitfield_size) {
> +				/* bitfields seem to be always aligned, no matter the packing */
> +				pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
> +				pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
> +				/* re-adjust bitfield offset if it is negative */
> +				if (pos->bitfield_offset < 0) {
> +					pos->bitfield_offset += pos->bit_size;
> +					pos->byte_offset -= pos->byte_size;
> +					pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
> +				}
> +			} else {
> +				pos->byte_offset = pos->bit_offset / 8;
>  			}
> -		} else {
> -			pos->byte_offset = pos->bit_offset / 8;
>  		}
> +
> +		pos->alignment = class__infer_alignment(pos->byte_offset,
> +							tag__natural_alignment(type, cu),
> +							smallest_offset);
> +		smallest_offset = pos->byte_offset + pos->byte_size;
>  	}
>  
> +	tag_type->alignment = class__infer_alignment(tag_type->size,
> +						     tag__natural_alignment(tag, cu),
> +						     smallest_offset);
> +
>  	return 0;
>  }
>  
> @@ -519,6 +554,7 @@ static int cu__fixup_btf_bitfields(struct cu *cu)
>  	int err = 0;
>  	struct tag *pos;
>  
> +
>  	list_for_each_entry(pos, &cu->tags, node)
>  		if (tag__is_struct(pos) || tag__is_union(pos)) {
>  			err = class__fixup_btf_bitfields(pos, cu);
> diff --git a/dwarves.c b/dwarves.c
> index b6f2489..bb8af5b 100644
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -1515,7 +1515,7 @@ void class__find_holes(struct class *class)
>  
>  static size_t type__natural_alignment(struct type *type, const struct cu *cu);
>  
> -static size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
> +size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
>  {
>  	size_t natural_alignment = 1;
>  
> diff --git a/dwarves.h b/dwarves.h
> index 30d33fa..c2fea0a 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -1002,6 +1002,8 @@ struct type {
>  
>  void __type__init(struct type *type);
>  
> +size_t tag__natural_alignment(struct tag *tag, const struct cu *cu);
> +
>  static inline struct class *type__class(const struct type *type)
>  {
>  	return (struct class *)type;
> -- 
> 2.25.1

-- 

- Arnaldo

Re: [PATCH 1/2] fprintf: Fix nested struct printing

[Douglas Raillard]

On 10/15/21 2:08 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 14, 2021 at 12:48:49PM +0100, Douglas RAILLARD escreveu:
>> From: Douglas Raillard <douglas.raillard@arm.com>
>>
>> This code:
>>
>>      struct X {
>>         struct {
>>         } __attribute__((foo)) x __attribute__((bar));
>>      }
>>
>> Was wrongly printed as:
>>
>>      struct X {
>>         struct {
>>         } x __attribute__((foo)) __attribute__((bar));
>>      }
>>
>> This unfortunately matters a lot, since "bar" is suppose to apply to
>> "x", but "foo" to typeof(x). In the wrong form, both apply to "x",
>> leading to e.g. incorrect layout for __aligned__ attribute.
> 
> I couldn't quickly find a change in behaviour with a random vmlinux I
> have for testing:
> 
> ⬢[acme@toolbox pahole]$ build/pahole.old -F dwarf vmlinux > before
> ⬢[acme@toolbox pahole]$ build/pahole -F dwarf vmlinux > after
> ⬢[acme@toolbox pahole]$ diff -u before after
> ⬢[acme@toolbox pahole]$ grep "__attribute__.*__attribute__" before  | wc -l
> 40

That's strange, I get plenty of diff such as this one from struct cfs_rq:

-       } removed __attribute__((__aligned__(64))) __attribute__((__aligned__(64)));      /*   192   128 */
+       } __attribute__((__aligned__(64))) removed __attribute__((__aligned__(64)));      /*   192   128 */


> ⬢[acme@toolbox pahole]$
> 
> btdiff is happy tho:

Same on my side

> 
> ⬢[acme@toolbox pahole]$ btfdiff vmlinux
> ⬢[acme@toolbox pahole]$ cat btfdiff
> #!/bin/bash
> # SPDX-License-Identifier: GPL-2.0-only
> # Copyright © 2019 Red Hat Inc, Arnaldo Carvalho de Melo <acme@redhat.com>
> # Use pahole to produce output from BTF and from DWARF, then do a diff
> # Use --flat_arrays with DWARF as BTF, like CTF, flattens arrays.
> # Use --show_private_classes as BTF shows all structs, while pahole knows
> # if some struct is defined only inside another struct/class or in a function,
> # this information is not available when loading from BTF.
> 
> if [ $# -eq 0 ] ; then
> 	echo "Usage: btfdiff <filename_with_DWARF_and_maybe_BTF_info> [<filename_with_BTF_info>]"
> 	exit 1
> fi
> 
> dwarf_input=$1
> btf_input=$dwarf_input
> 
> if [ $# -eq 2 ] ; then
> 	btf_input=$2
> fi
> 
> btf_output=$(mktemp /tmp/btfdiff.btf.XXXXXX)
> dwarf_output=$(mktemp /tmp/btfdiff.dwarf.XXXXXX)
> pahole_bin=${PAHOLE-"pahole"}
> 
> ${pahole_bin} -F dwarf \
> 	      --flat_arrays \
> 	      --sort \
> 	      --jobs \
> 	      --suppress_aligned_attribute \
> 	      --suppress_force_paddings \
> 	      --suppress_packed \
> 	      --show_private_classes $dwarf_input > $dwarf_output
> ${pahole_bin} -F btf \
> 	      --sort \
> 	      --suppress_packed \
> 	      $btf_input > $btf_output
> 
> diff -up $dwarf_output $btf_output
> 
> rm -f $btf_output $dwarf_output
> exit 0
> ⬢[acme@toolbox pahole]$
> 
> As is fullcircle:

I tried fullcircle on the whole vmlinux and got a bunch of:
readelf: Error: LEB value too large
but beyond that it seems happy.

> ⬢[acme@toolbox pahole]$ fullcircle tcp.o
> ⬢[acme@toolbox pahole]$ cat fullcircle
> #!/bin/bash
> # SPDX-License-Identifier: GPL-2.0-only
> # Copyright © 2019 Red Hat Inc, Arnaldo Carvalho de Melo <acme@redhat.com>
> # Use pfunct to produce compilable output from a object, then do a codiff -s
> # To see if the type information generated from source code generated
> # from type information in a file compiled from the original source code matches.
> 
> if [ $# -eq 0 ] ; then
> 	echo "Usage: fullcircle <filename_with_type_info>"
> 	exit 1
> fi
> 
> file=$1
> 
> nr_cus=$(readelf -wi ${file} | grep DW_TAG_compile_unit | wc -l)
> if [ $nr_cus -gt 1 ]; then
> 	exit 0
> fi
> 
> c_output=$(mktemp /tmp/fullcircle.XXXXXX.c)
> o_output=$(mktemp /tmp/fullcircle.XXXXXX.o)
> pfunct_bin=${PFUNCT-"pfunct"}
> codiff_bin=${CODIFF-"codiff"}
> 
> # See how your DW_AT_producer looks like and find the
> # right regexp to get after the GCC version string, this one
> # seems good enough for Red Hat/Fedora/CentOS that look like:
> #
> #   DW_AT_producer    : (indirect string, offset: 0x3583): GNU C89 8.2.1 20181215 (Red Hat 8.2.1-6) -mno-sse -mno-mmx
> #
> # So we need from -mno-sse onwards
> 
> CFLAGS=$(readelf -wi $file | grep -w DW_AT_producer | sed -r      's/.*\)( -[[:alnum:]]+.*)+/\1/g')
> 
> # Check if we managed to do the sed or if this is something like GNU AS
> [ "${CFLAGS/DW_AT_producer/}" != "${CFLAGS}" ] && exit
> 
> ${pfunct_bin} --compile $file > $c_output
> gcc $CFLAGS -c -g $c_output -o $o_output
> ${codiff_bin} -q -s $file $o_output
> 
> rm -f $c_output $o_output
> exit 0
> ⬢[acme@toolbox pahole]$
> 
> - Arnaldo
>   
>> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
>> ---
>>   dwarves_fprintf.c | 22 ++++++++++++++++++----
>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
>> index c35868a..1c1d949 100644
>> --- a/dwarves_fprintf.c
>> +++ b/dwarves_fprintf.c
>> @@ -787,7 +787,7 @@ print_default:
>>   					   (type->tag == DW_TAG_class_type &&
>>   					    !tconf.classes_as_structs) ? "class" : "struct",
>>   					   tconf.type_spacing - 7,
>> -					   type__name(ctype), name);
>> +					   type__name(ctype), name ?: "");
>>   		} else {
>>   			struct class *cclass = tag__class(type);
>>   
>> @@ -802,7 +802,7 @@ print_default:
>>   		ctype = tag__type(type);
>>   
>>   		if (type__name(ctype) != NULL && !expand_types) {
>> -			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name);
>> +			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name ?: "");
>>   		} else {
>>   			tconf.type_spacing -= 8;
>>   			printed += union__fprintf(ctype, cu, &tconf, fp);
>> @@ -812,7 +812,7 @@ print_default:
>>   		ctype = tag__type(type);
>>   
>>   		if (type__name(ctype) != NULL)
>> -			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name);
>> +			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name ?: "");
>>   		else
>>   			printed += enumeration__fprintf(type, &tconf, fp);
>>   		break;
>> @@ -863,7 +863,21 @@ static size_t class_member__fprintf(struct class_member *member, bool union_memb
>>   	if (member->is_static)
>>   		printed += fprintf(fp, "static ");
>>   
>> -	printed += type__fprintf(type, cu, name, &sconf, fp);
>> +	/* For struct-like constructs, the name of the member cannot be
>> +	 * conflated with the name of its type, otherwise __attribute__ are
>> +	 * printed in the wrong order.
>> +	 */
>> +	if (tag__is_union(type) || tag__is_struct(type) ||
>> +	    tag__is_enumeration(type)) {
>> +		printed += type__fprintf(type, cu, NULL, &sconf, fp);
>> +		if (name) {
>> +			if (!type__name(tag__type(type)))
>> +				printed += fprintf(fp, " ");
>> +			printed += fprintf(fp, "%s", name);
>> +		}
>> +	} else {
>> +		printed += type__fprintf(type, cu, name, &sconf, fp);
>> +	}
>>   
>>   	if (member->is_static) {
>>   		if (member->const_value != 0)
>> -- 
>> 2.25.1
> 

Re: [PATCH 2/2] btf_loader.c: Infer alignment info

[Douglas Raillard]

On 10/15/21 2:12 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 14, 2021 at 12:48:50PM +0100, Douglas RAILLARD escreveu:
>> From: Douglas Raillard <douglas.raillard@arm.com>
>   
>> BTF does not carry alignment information, but it carries the offset in
>> structs. This allows inferring the original alignment, yielding a C
>> header dump that is not identical to the original C code, but is
>> guaranteed to lead to the same memory layout.
>   
>> This allows using the output of pahole in another program to poke at
>> memory, with the assurance that we will not read garbage.
> 
> Which is super cool, thanks for working on this.
> 
> Please take a look at the 'fullcircle' tool in the pahole repo, I once
> used it on all the single CU .o files in the kernel build to make sure
> that we generate a source code and then object file that matches the
> original object file (and thus its .c file), at least in layouts.
> 
> Doing it with BTF too after your change would be fantastic.

I will, I was basically doing the same thing manually:
* generate header from BTF
* compile a kernel module with the generated header
* dump back a header from the .ko (based on DWARF)
* check the vmlinux header and .ko header have identical offsets for the
   relevant types

> 
> Now to review your patch, at first it looks big, but perhaps its
> inherent to the change and we can't break it into smaller steps, will
> see.

I can break it down further, it's a small refactoring to remove the
"continue" followed by the new code. I'll respin the patch and test
with fullcircle.

Thanks,

Douglas

> 
> - Arnaldo
>   
>> Note: Since the alignment is inferred from the offset, it sometimes
>> happens that the offset was already correctly aligned, which means the
>> inferred alignment will be smaller than in the original source. This
>> does not impact the ability to read existing structs, but it could
>> impact creating such struct if other client code expects higher
>> alignment than the one exposed in the generated header.
>>
>> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
>> ---
>>   btf_loader.c | 66 ++++++++++++++++++++++++++++++++++++++++------------
>>   dwarves.c    |  2 +-
>>   dwarves.h    |  2 ++
>>   3 files changed, 54 insertions(+), 16 deletions(-)
>>
>> diff --git a/btf_loader.c b/btf_loader.c
>> index 3e5a945..54353d7 100644
>> --- a/btf_loader.c
>> +++ b/btf_loader.c
>> @@ -471,10 +471,37 @@ static int btf__load_sections(struct btf *btf, struct cu *cu)
>>   	return btf__load_types(btf, cu);
>>   }
>>   
>> +static uint32_t class__infer_alignment(uint32_t byte_offset,
>> +				       uint32_t natural_alignment,
>> +				       uint32_t smallest_offset)
>> +{
>> +	uint32_t alignment = 0;
>> +	uint32_t offset_delta = byte_offset - smallest_offset;
>> +
>> +	if (offset_delta) {
>> +		if (byte_offset % 2 == 0) {
>> +			/* Find the power of 2 immediately higher than
>> +			 * offset_delta
>> +			 */
>> +			alignment = 1 << (8 * sizeof(offset_delta) -
>> +					      __builtin_clz(offset_delta));
>> +		} else {
>> +			alignment = 0;
>> +		}
>> +	}
>> +
>> +	/* Natural alignment, nothing to do */
>> +	if (alignment <= natural_alignment || alignment == 1)
>> +		alignment = 0;
>> +
>> +	return alignment;
>> +}
>> +
>>   static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>>   {
>>   	struct class_member *pos;
>>   	struct type *tag_type = tag__type(tag);
>> +	uint32_t smallest_offset = 0;
>>   
>>   	type__for_each_data_member(tag_type, pos) {
>>   		struct tag *type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
>> @@ -486,31 +513,39 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>>   		pos->byte_size = tag__size(type, cu);
>>   		pos->bit_size = pos->byte_size * 8;
>>   
>> -		/* bitfield fixup is needed for enums and base types only */
>> -		if (type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type)
>> -			continue;
>> -
>>   		/* if BTF data is incorrect and has size == 0, skip field,
>>   		 * instead of crashing */
>>   		if (pos->byte_size == 0) {
>>   			continue;
>>   		}
>>   
>> -		if (pos->bitfield_size) {
>> -			/* bitfields seem to be always aligned, no matter the packing */
>> -			pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
>> -			pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
>> -			/* re-adjust bitfield offset if it is negative */
>> -			if (pos->bitfield_offset < 0) {
>> -				pos->bitfield_offset += pos->bit_size;
>> -				pos->byte_offset -= pos->byte_size;
>> -				pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
>> +		/* bitfield fixup is needed for enums and base types only */
>> +		if (type->tag == DW_TAG_base_type || type->tag == DW_TAG_enumeration_type) {
>> +			if (pos->bitfield_size) {
>> +				/* bitfields seem to be always aligned, no matter the packing */
>> +				pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
>> +				pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
>> +				/* re-adjust bitfield offset if it is negative */
>> +				if (pos->bitfield_offset < 0) {
>> +					pos->bitfield_offset += pos->bit_size;
>> +					pos->byte_offset -= pos->byte_size;
>> +					pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
>> +				}
>> +			} else {
>> +				pos->byte_offset = pos->bit_offset / 8;
>>   			}
>> -		} else {
>> -			pos->byte_offset = pos->bit_offset / 8;
>>   		}
>> +
>> +		pos->alignment = class__infer_alignment(pos->byte_offset,
>> +							tag__natural_alignment(type, cu),
>> +							smallest_offset);
>> +		smallest_offset = pos->byte_offset + pos->byte_size;
>>   	}
>>   
>> +	tag_type->alignment = class__infer_alignment(tag_type->size,
>> +						     tag__natural_alignment(tag, cu),
>> +						     smallest_offset);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -519,6 +554,7 @@ static int cu__fixup_btf_bitfields(struct cu *cu)
>>   	int err = 0;
>>   	struct tag *pos;
>>   
>> +
>>   	list_for_each_entry(pos, &cu->tags, node)
>>   		if (tag__is_struct(pos) || tag__is_union(pos)) {
>>   			err = class__fixup_btf_bitfields(pos, cu);
>> diff --git a/dwarves.c b/dwarves.c
>> index b6f2489..bb8af5b 100644
>> --- a/dwarves.c
>> +++ b/dwarves.c
>> @@ -1515,7 +1515,7 @@ void class__find_holes(struct class *class)
>>   
>>   static size_t type__natural_alignment(struct type *type, const struct cu *cu);
>>   
>> -static size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
>> +size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
>>   {
>>   	size_t natural_alignment = 1;
>>   
>> diff --git a/dwarves.h b/dwarves.h
>> index 30d33fa..c2fea0a 100644
>> --- a/dwarves.h
>> +++ b/dwarves.h
>> @@ -1002,6 +1002,8 @@ struct type {
>>   
>>   void __type__init(struct type *type);
>>   
>> +size_t tag__natural_alignment(struct tag *tag, const struct cu *cu);
>> +
>>   static inline struct class *type__class(const struct type *type)
>>   {
>>   	return (struct class *)type;
>> -- 
>> 2.25.1
>