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

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

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