[PATCH dwarves 0/3] dwarves: improve BTF encoder comparison method

[PATCH dwarves 0/3] dwarves: improve BTF encoder comparison method

[Alan Maguire]

Currently when looking for function prototype mismatches with a view
to excluding inconsistent functions, we fall back to a comparison
between parameter names when the name and number of parameters match.
This is brittle, as it is sometimes the case that a function has
multiple type-identical definitions which use different parameters.

Here the existing dwarves_fprintf functionality is re-used to instead
create a string representation of the function prototype - minus the
parameter names - to support a less brittle comparison method.

To support this, patch 1 generalizes function prototype print to
take a conf_fprintf parameter; this allows us to customize the
parameters we use in prototype string generation.

Patch 2 supports generating prototypes without modifiers such
as const as they can lead to false positive prototype mismatches;
see the patch for details.

Finally patch 3 replaces the logic used to compare parameter
names with the prototype string comparison instead.

Using verbose pahole output we can see some of the rejected
comparisons.  73 comparisons are rejected via prototype
comparison, 63 of which are non "."-suffixed functions.  For
example:

function mismatch for 'name_show'('name_show'): 'ssize_t ()(struct kobject *, struct kobj_attribute *, char *)' != 'ssize_t ()(struct device *, struct device_attribute *, char *)'

With these changes, the syscalls defined in sys_ni.c
that Jiri mentioned were missing [1] are present in BTF:

[43071] FUNC '__ia32_compat_sys_io_setup' type_id=42335 linkage=static
[43295] FUNC '__ia32_sys_io_setup' type_id=42335 linkage=static
[47536] FUNC '__x64_sys_io_setup' type_id=42335 linkage=static

[43290] FUNC '__ia32_sys_io_destroy' type_id=42335 linkage=static
[47531] FUNC '__x64_sys_io_destroy' type_id=42335 linkage=static

[43072] FUNC '__ia32_compat_sys_io_submit' type_id=42335 linkage=static
[43296] FUNC '__ia32_sys_io_submit' type_id=42335 linkage=static
[47537] FUNC '__x64_sys_io_submit' type_id=42335 linkage=static

[1] https://lore.kernel.org/bpf/ZAsBYpsBV0wvkhh0@krava/

Alan Maguire (3):
  dwarves_fprintf: generalize function prototype print to support
    passing conf
  dwarves_fprintf: support skipping modifier
  btf_encoder: compare functions via prototypes not parameter names

 btf_encoder.c     | 67 +++++++++++++++++++++++++------------------------------
 dwarves.h         |  6 +++++
 dwarves_fprintf.c | 48 ++++++++++++++++++++++++++-------------
 3 files changed, 70 insertions(+), 51 deletions(-)

-- 
1.8.3.1

[PATCH dwarves 1/3] dwarves_fprintf: generalize function prototype print to support passing conf

[Alan Maguire]

function__prototype() writes the function prototype to the passed-in
buffer, but uses the default conf_fprint structure which prints
parameter names.  Generalize into function__prototype_conf() so that
callers can pass in their own conf_fprintf; this will be useful
for generating prototype strings for type matching.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 dwarves.h         |  5 +++++
 dwarves_fprintf.c | 22 +++++++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/dwarves.h b/dwarves.h
index e92b2fd..d04a36d 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -133,6 +133,7 @@ struct conf_fprintf {
 	uint8_t	   hex_fmt:1;
 	uint8_t	   strip_inline:1;
 	uint8_t	   skip_emitting_atomic_typedefs:1;
+	uint8_t	   skip_emitting_errors:1;
 };
 
 struct cus;
@@ -944,6 +945,10 @@ size_t function__fprintf_stats(const struct tag *tag_func,
 			       FILE *fp);
 const char *function__prototype(const struct function *func,
 				const struct cu *cu, char *bf, size_t len);
+const char *function__prototype_conf(const struct function *func,
+				     const struct cu *cu,
+				     const struct conf_fprintf *conf,
+				     char *bf, size_t len);
 
 static __pure inline uint64_t function__addr(const struct function *func)
 {
diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index 30355b4..5c6bf9c 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -1102,21 +1102,33 @@ static size_t union__fprintf(struct type *type, const struct cu *cu,
 				 conf->suffix ? " " : "", conf->suffix ?: "");
 }
 
-const char *function__prototype(const struct function *func,
-				const struct cu *cu, char *bf, size_t len)
+const char *function__prototype_conf(const struct function *func,
+				     const struct cu *cu,
+				     const struct conf_fprintf *conf,
+				     char *bf, size_t len)
 {
 	FILE *bfp = fmemopen(bf, len, "w");
 
 	if (bfp != NULL) {
-		ftype__fprintf(&func->proto, cu, NULL, 0, 0, 0, true,
-			       &conf_fprintf__defaults, bfp);
+		ftype__fprintf(&func->proto, cu, NULL, 0, 0, 0, true, conf,
+			       bfp);
 		fclose(bfp);
-	} else
+	} else {
+		if (conf->skip_emitting_errors)
+			return NULL;
 		snprintf(bf, len, "<ERROR(%s): fmemopen failed!>", __func__);
+	}
 
 	return bf;
 }
 
+const char *function__prototype(const struct function *func,
+				const struct cu *cu, char *bf, size_t len)
+{
+	return function__prototype_conf(func, cu, &conf_fprintf__defaults,
+					bf, len);
+}
+
 size_t ftype__fprintf_parms(const struct ftype *ftype,
 			    const struct cu *cu, int indent,
 			    const struct conf_fprintf *conf, FILE *fp)
-- 
1.8.3.1

[PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Alan Maguire]

When doing BTF comparisons between functions defined in multiple
CUs, it was noticed a few critical functions failed prototype
comparisons due to multiple "const" modifiers; for example:

function mismatch for 'memchr_inv'('memchr_inv'): 'void * ()(const const void  * , int, size_t)' != 'void * ()(const void  *, int, size_t)'

function mismatch for 'strnlen'('strnlen'): '__kernel_size_t ()(const const char  * , __kernel_size_t)' != '__kernel_size_t ()(const char  *, size_t)'

(note the "const const" in the first parameter.)

As such it would be useful to omit modifiers for comparison
purposes.  Also noted was the fact that for the "no_parm_names"
case, an extra space was being emitted in some cases, also
throwing off string comparisons of prototypes.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 dwarves.h         |  1 +
 dwarves_fprintf.c | 26 ++++++++++++++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/dwarves.h b/dwarves.h
index d04a36d..7a319d1 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -134,6 +134,7 @@ struct conf_fprintf {
 	uint8_t	   strip_inline:1;
 	uint8_t	   skip_emitting_atomic_typedefs:1;
 	uint8_t	   skip_emitting_errors:1;
+	uint8_t    skip_emitting_modifier:1;
 };
 
 struct cus;
diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index 5c6bf9c..b20a473 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
 				struct tag *next_type = cu__type(cu, type->type);
 
 				if (next_type && tag__is_pointer(next_type)) {
-					const_pointer = "const ";
+					if (!conf->skip_emitting_modifier)
+						const_pointer = "const ";
 					type = next_type;
 				}
 			}
@@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
 				   *type_str = __tag__name(type, cu, tmpbf,
 							   sizeof(tmpbf),
 							   pconf);
-			switch (tag->tag) {
-			case DW_TAG_volatile_type: prefix = "volatile "; break;
-			case DW_TAG_const_type:    prefix = "const ";	 break;
-			case DW_TAG_restrict_type: suffix = " restrict"; break;
-			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
+			if (!conf->skip_emitting_modifier) {
+				switch (tag->tag) {
+				case DW_TAG_volatile_type: prefix = "volatile "; break;
+				case DW_TAG_const_type: prefix = "const"; break;
+				case DW_TAG_restrict_type: suffix = " restrict"; break;
+				case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
+				}
 			}
-			snprintf(bf, len, "%s%s%s ", prefix, type_str, suffix);
+			snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
+				 conf->no_parm_names ? "" : " ");
 		}
 		break;
 	case DW_TAG_array_type:
@@ -818,9 +822,11 @@ print_default:
 	case DW_TAG_const_type:
 		modifier = "const";
 print_modifier: {
-		size_t modifier_printed = fprintf(fp, "%s ", modifier);
-		tconf.type_spacing -= modifier_printed;
-		printed		   += modifier_printed;
+		if (!conf->skip_emitting_modifier) {
+			size_t modifier_printed = fprintf(fp, "%s ", modifier);
+			tconf.type_spacing -= modifier_printed;
+			printed		   += modifier_printed;
+		}
 
 		struct tag *ttype = cu__type(cu, type->type);
 		if (ttype) {
-- 
1.8.3.1

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Arnaldo Carvalho de Melo]

Em Fri, Mar 10, 2023 at 02:50:49PM +0000, Alan Maguire escreveu:
> When doing BTF comparisons between functions defined in multiple
> CUs, it was noticed a few critical functions failed prototype
> comparisons due to multiple "const" modifiers; for example:
> 
> function mismatch for 'memchr_inv'('memchr_inv'): 'void * ()(const const void  * , int, size_t)' != 'void * ()(const void  *, int, size_t)'
> 
> function mismatch for 'strnlen'('strnlen'): '__kernel_size_t ()(const const char  * , __kernel_size_t)' != '__kernel_size_t ()(const char  *, size_t)'
> 
> (note the "const const" in the first parameter.)
> 
> As such it would be useful to omit modifiers for comparison
> purposes.  Also noted was the fact that for the "no_parm_names"
> case, an extra space was being emitted in some cases, also
> throwing off string comparisons of prototypes.

Running 'btfdiff vmlinux' after this change ends up in a segfault:

⬢[acme@toolbox pahole]$ btfdiff vmlinux
/var/home/acme/bin/btfdiff: line 34:  8183 Segmentation fault      (core dumped) ${pahole_bin} -F dwarf --flat_arrays --sort --jobs --suppress_aligned_attribute --suppress_force_paddings --suppress_packed --lang_exclude rust --show_private_classes $dwarf_input > $dwarf_output
/var/home/acme/bin/btfdiff: line 39:  8237 Segmentation fault      (core dumped) ${pahole_bin} -F btf --sort --suppress_aligned_attribute --suppress_packed $btf_input > $btf_output
⬢[acme@toolbox pahole]$

Investigating.

- Arnaldo
 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  dwarves.h         |  1 +
>  dwarves_fprintf.c | 26 ++++++++++++++++----------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/dwarves.h b/dwarves.h
> index d04a36d..7a319d1 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -134,6 +134,7 @@ struct conf_fprintf {
>  	uint8_t	   strip_inline:1;
>  	uint8_t	   skip_emitting_atomic_typedefs:1;
>  	uint8_t	   skip_emitting_errors:1;
> +	uint8_t    skip_emitting_modifier:1;
>  };
>  
>  struct cus;
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index 5c6bf9c..b20a473 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
>  				struct tag *next_type = cu__type(cu, type->type);
>  
>  				if (next_type && tag__is_pointer(next_type)) {
> -					const_pointer = "const ";
> +					if (!conf->skip_emitting_modifier)
> +						const_pointer = "const ";
>  					type = next_type;
>  				}
>  			}
> @@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>  				   *type_str = __tag__name(type, cu, tmpbf,
>  							   sizeof(tmpbf),
>  							   pconf);
> -			switch (tag->tag) {
> -			case DW_TAG_volatile_type: prefix = "volatile "; break;
> -			case DW_TAG_const_type:    prefix = "const ";	 break;
> -			case DW_TAG_restrict_type: suffix = " restrict"; break;
> -			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> +			if (!conf->skip_emitting_modifier) {
> +				switch (tag->tag) {
> +				case DW_TAG_volatile_type: prefix = "volatile "; break;
> +				case DW_TAG_const_type: prefix = "const"; break;
> +				case DW_TAG_restrict_type: suffix = " restrict"; break;
> +				case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> +				}
>  			}
> -			snprintf(bf, len, "%s%s%s ", prefix, type_str, suffix);
> +			snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
> +				 conf->no_parm_names ? "" : " ");
>  		}
>  		break;
>  	case DW_TAG_array_type:
> @@ -818,9 +822,11 @@ print_default:
>  	case DW_TAG_const_type:
>  		modifier = "const";
>  print_modifier: {
> -		size_t modifier_printed = fprintf(fp, "%s ", modifier);
> -		tconf.type_spacing -= modifier_printed;
> -		printed		   += modifier_printed;
> +		if (!conf->skip_emitting_modifier) {
> +			size_t modifier_printed = fprintf(fp, "%s ", modifier);
> +			tconf.type_spacing -= modifier_printed;
> +			printed		   += modifier_printed;
> +		}
>  
>  		struct tag *ttype = cu__type(cu, type->type);
>  		if (ttype) {
> -- 
> 1.8.3.1
> 

-- 

- Arnaldo

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Arnaldo Carvalho de Melo]

Em Mon, Mar 13, 2023 at 09:20:33AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Mar 10, 2023 at 02:50:49PM +0000, Alan Maguire escreveu:
> > When doing BTF comparisons between functions defined in multiple
> > CUs, it was noticed a few critical functions failed prototype
> > comparisons due to multiple "const" modifiers; for example:
> > 
> > function mismatch for 'memchr_inv'('memchr_inv'): 'void * ()(const const void  * , int, size_t)' != 'void * ()(const void  *, int, size_t)'
> > 
> > function mismatch for 'strnlen'('strnlen'): '__kernel_size_t ()(const const char  * , __kernel_size_t)' != '__kernel_size_t ()(const char  *, size_t)'
> > 
> > (note the "const const" in the first parameter.)
> > 
> > As such it would be useful to omit modifiers for comparison
> > purposes.  Also noted was the fact that for the "no_parm_names"
> > case, an extra space was being emitted in some cases, also
> > throwing off string comparisons of prototypes.
> 
> Running 'btfdiff vmlinux' after this change ends up in a segfault:
> 
> ⬢[acme@toolbox pahole]$ btfdiff vmlinux
> /var/home/acme/bin/btfdiff: line 34:  8183 Segmentation fault      (core dumped) ${pahole_bin} -F dwarf --flat_arrays --sort --jobs --suppress_aligned_attribute --suppress_force_paddings --suppress_packed --lang_exclude rust --show_private_classes $dwarf_input > $dwarf_output
> /var/home/acme/bin/btfdiff: line 39:  8237 Segmentation fault      (core dumped) ${pahole_bin} -F btf --sort --suppress_aligned_attribute --suppress_packed $btf_input > $btf_output
> ⬢[acme@toolbox pahole]$
> 
> Investigating.

(gdb) run -F dwarf --flat_arrays --sort --jobs --suppress_aligned_attribute --suppress_force_paddings --suppress_packed --lang_exclude rust --show_private_classes vmlinux
Starting program: /var/home/acme/bin/pahole -F dwarf --flat_arrays --sort --jobs --suppress_aligned_attribute --suppress_force_paddings --suppress_packed --lang_exclude rust --show_private_classes vmlinux
Thread 1 "pahole" received signal SIGSEGV, Segmentation fault.
0x00007ffff7f26cff in __tag__name (tag=0x7fff88016a20, cu=0x7fff88001e30, bf=0x7fffffffce90 "void ()(void)", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:584
584				if (!conf->skip_emitting_modifier) {
(gdb) bt
#0  0x00007ffff7f26cff in __tag__name (tag=0x7fff88016a20, cu=0x7fff88001e30, bf=0x7fffffffce90 "void ()(void)", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:584
#1  0x00007ffff7f26873 in tag__ptr_name (tag=0x7fff88016990, cu=0x7fff88001e30, bf=0x7fffffffd9d0 "long unsigned int", len=1024, ptr_suffix=0x7ffff7f88fb0 "*", conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:515
#2  0x00007ffff7f26acd in __tag__name (tag=0x7fff88016990, cu=0x7fff88001e30, bf=0x7fffffffd9d0 "long unsigned int", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:551
#3  0x00007ffff7f270d5 in tag__name (tag=0x7fff88016990, cu=0x7fff88001e30, bf=0x7fffffffd9d0 "long unsigned int", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:639
#4  0x0000000000404042 in type__compare_members_types (a=0x7fff9401bc30, cu_a=0x7fff94001e30, b=0x7fff8801bba0, cu_b=0x7fff88001e30) at /var/home/acme/git/pahole/pahole.c:258
#5  0x0000000000404cd0 in resort_add (resorted=0x7fffffffded8, str=0x7fff8801d120) at /var/home/acme/git/pahole/pahole.c:649
#6  0x0000000000404d7e in resort_classes (resorted=0x7fffffffded8, head=0x411420 <structures.list>) at /var/home/acme/git/pahole/pahole.c:668
#7  0x0000000000404dda in print_ordered_classes () at /var/home/acme/git/pahole/pahole.c:678
#8  0x000000000040a93c in main (argc=13, argv=0x7fffffffe068) at /var/home/acme/git/pahole/pahole.c:3528
(gdb)

I'm adding this:

diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index b20a473125c3aa41..c2fdcdad078a5335 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -506,7 +506,7 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
 				struct tag *next_type = cu__type(cu, type->type);
 
 				if (next_type && tag__is_pointer(next_type)) {
-					if (!conf->skip_emitting_modifier)
+					if (!(conf && conf->skip_emitting_modifier))
 						const_pointer = "const ";
 					type = next_type;
 				}
@@ -581,7 +581,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
 				   *type_str = __tag__name(type, cu, tmpbf,
 							   sizeof(tmpbf),
 							   pconf);
-			if (!conf->skip_emitting_modifier) {
+			if (!pconf->skip_emitting_modifier) {
 				switch (tag->tag) {
 				case DW_TAG_volatile_type: prefix = "volatile "; break;
 				case DW_TAG_const_type: prefix = "const"; break;
@@ -590,7 +590,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
 				}
 			}
 			snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
-				 conf->no_parm_names ? "" : " ");
+				 pconf->no_parm_names ? "" : " ");
 		}
 		break;
 	case DW_TAG_array_type:


With it:

⬢[acme@toolbox pahole]$ btfdiff vmlinux
⬢[acme@toolbox pahole]$
 
> - Arnaldo
>  
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >  dwarves.h         |  1 +
> >  dwarves_fprintf.c | 26 ++++++++++++++++----------
> >  2 files changed, 17 insertions(+), 10 deletions(-)
> > 
> > diff --git a/dwarves.h b/dwarves.h
> > index d04a36d..7a319d1 100644
> > --- a/dwarves.h
> > +++ b/dwarves.h
> > @@ -134,6 +134,7 @@ struct conf_fprintf {
> >  	uint8_t	   strip_inline:1;
> >  	uint8_t	   skip_emitting_atomic_typedefs:1;
> >  	uint8_t	   skip_emitting_errors:1;
> > +	uint8_t    skip_emitting_modifier:1;
> >  };
> >  
> >  struct cus;
> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > index 5c6bf9c..b20a473 100644
> > --- a/dwarves_fprintf.c
> > +++ b/dwarves_fprintf.c
> > @@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
> >  				struct tag *next_type = cu__type(cu, type->type);
> >  
> >  				if (next_type && tag__is_pointer(next_type)) {
> > -					const_pointer = "const ";
> > +					if (!conf->skip_emitting_modifier)
> > +						const_pointer = "const ";
> >  					type = next_type;
> >  				}
> >  			}
> > @@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> >  				   *type_str = __tag__name(type, cu, tmpbf,
> >  							   sizeof(tmpbf),
> >  							   pconf);
> > -			switch (tag->tag) {
> > -			case DW_TAG_volatile_type: prefix = "volatile "; break;
> > -			case DW_TAG_const_type:    prefix = "const ";	 break;
> > -			case DW_TAG_restrict_type: suffix = " restrict"; break;
> > -			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> > +			if (!conf->skip_emitting_modifier) {
> > +				switch (tag->tag) {
> > +				case DW_TAG_volatile_type: prefix = "volatile "; break;
> > +				case DW_TAG_const_type: prefix = "const"; break;
> > +				case DW_TAG_restrict_type: suffix = " restrict"; break;
> > +				case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> > +				}
> >  			}
> > -			snprintf(bf, len, "%s%s%s ", prefix, type_str, suffix);
> > +			snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
> > +				 conf->no_parm_names ? "" : " ");
> >  		}
> >  		break;
> >  	case DW_TAG_array_type:
> > @@ -818,9 +822,11 @@ print_default:
> >  	case DW_TAG_const_type:
> >  		modifier = "const";
> >  print_modifier: {
> > -		size_t modifier_printed = fprintf(fp, "%s ", modifier);
> > -		tconf.type_spacing -= modifier_printed;
> > -		printed		   += modifier_printed;
> > +		if (!conf->skip_emitting_modifier) {
> > +			size_t modifier_printed = fprintf(fp, "%s ", modifier);
> > +			tconf.type_spacing -= modifier_printed;
> > +			printed		   += modifier_printed;
> > +		}
> >  
> >  		struct tag *ttype = cu__type(cu, type->type);
> >  		if (ttype) {
> > -- 
> > 1.8.3.1
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Alan Maguire]

On 13/03/2023 12:29, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 13, 2023 at 09:20:33AM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Fri, Mar 10, 2023 at 02:50:49PM +0000, Alan Maguire escreveu:
>>> When doing BTF comparisons between functions defined in multiple
>>> CUs, it was noticed a few critical functions failed prototype
>>> comparisons due to multiple "const" modifiers; for example:
>>>
>>> function mismatch for 'memchr_inv'('memchr_inv'): 'void * ()(const const void  * , int, size_t)' != 'void * ()(const void  *, int, size_t)'
>>>
>>> function mismatch for 'strnlen'('strnlen'): '__kernel_size_t ()(const const char  * , __kernel_size_t)' != '__kernel_size_t ()(const char  *, size_t)'
>>>
>>> (note the "const const" in the first parameter.)
>>>
>>> As such it would be useful to omit modifiers for comparison
>>> purposes.  Also noted was the fact that for the "no_parm_names"
>>> case, an extra space was being emitted in some cases, also
>>> throwing off string comparisons of prototypes.
>>
>> Running 'btfdiff vmlinux' after this change ends up in a segfault:
>>
>> ⬢[acme@toolbox pahole]$ btfdiff vmlinux
>> /var/home/acme/bin/btfdiff: line 34:  8183 Segmentation fault      (core dumped) ${pahole_bin} -F dwarf --flat_arrays --sort --jobs --suppress_aligned_attribute --suppress_force_paddings --suppress_packed --lang_exclude rust --show_private_classes $dwarf_input > $dwarf_output
>> /var/home/acme/bin/btfdiff: line 39:  8237 Segmentation fault      (core dumped) ${pahole_bin} -F btf --sort --suppress_aligned_attribute --suppress_packed $btf_input > $btf_output
>> ⬢[acme@toolbox pahole]$
>>
>> Investigating.
> 
> (gdb) run -F dwarf --flat_arrays --sort --jobs --suppress_aligned_attribute --suppress_force_paddings --suppress_packed --lang_exclude rust --show_private_classes vmlinux
> Starting program: /var/home/acme/bin/pahole -F dwarf --flat_arrays --sort --jobs --suppress_aligned_attribute --suppress_force_paddings --suppress_packed --lang_exclude rust --show_private_classes vmlinux
> Thread 1 "pahole" received signal SIGSEGV, Segmentation fault.
> 0x00007ffff7f26cff in __tag__name (tag=0x7fff88016a20, cu=0x7fff88001e30, bf=0x7fffffffce90 "void ()(void)", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:584
> 584				if (!conf->skip_emitting_modifier) {
> (gdb) bt
> #0  0x00007ffff7f26cff in __tag__name (tag=0x7fff88016a20, cu=0x7fff88001e30, bf=0x7fffffffce90 "void ()(void)", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:584
> #1  0x00007ffff7f26873 in tag__ptr_name (tag=0x7fff88016990, cu=0x7fff88001e30, bf=0x7fffffffd9d0 "long unsigned int", len=1024, ptr_suffix=0x7ffff7f88fb0 "*", conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:515
> #2  0x00007ffff7f26acd in __tag__name (tag=0x7fff88016990, cu=0x7fff88001e30, bf=0x7fffffffd9d0 "long unsigned int", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:551
> #3  0x00007ffff7f270d5 in tag__name (tag=0x7fff88016990, cu=0x7fff88001e30, bf=0x7fffffffd9d0 "long unsigned int", len=1024, conf=0x0) at /var/home/acme/git/pahole/dwarves_fprintf.c:639
> #4  0x0000000000404042 in type__compare_members_types (a=0x7fff9401bc30, cu_a=0x7fff94001e30, b=0x7fff8801bba0, cu_b=0x7fff88001e30) at /var/home/acme/git/pahole/pahole.c:258
> #5  0x0000000000404cd0 in resort_add (resorted=0x7fffffffded8, str=0x7fff8801d120) at /var/home/acme/git/pahole/pahole.c:649
> #6  0x0000000000404d7e in resort_classes (resorted=0x7fffffffded8, head=0x411420 <structures.list>) at /var/home/acme/git/pahole/pahole.c:668
> #7  0x0000000000404dda in print_ordered_classes () at /var/home/acme/git/pahole/pahole.c:678
> #8  0x000000000040a93c in main (argc=13, argv=0x7fffffffe068) at /var/home/acme/git/pahole/pahole.c:3528
> (gdb)
> 
> I'm adding this:
> 
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index b20a473125c3aa41..c2fdcdad078a5335 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -506,7 +506,7 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
>  				struct tag *next_type = cu__type(cu, type->type);
>  
>  				if (next_type && tag__is_pointer(next_type)) {
> -					if (!conf->skip_emitting_modifier)
> +					if (!(conf && conf->skip_emitting_modifier))
>  						const_pointer = "const ";
>  					type = next_type;
>  				}
> @@ -581,7 +581,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>  				   *type_str = __tag__name(type, cu, tmpbf,
>  							   sizeof(tmpbf),
>  							   pconf);
> -			if (!conf->skip_emitting_modifier) {
> +			if (!pconf->skip_emitting_modifier) {
>  				switch (tag->tag) {
>  				case DW_TAG_volatile_type: prefix = "volatile "; break;
>  				case DW_TAG_const_type: prefix = "const"; break;
> @@ -590,7 +590,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>  				}
>  			}
>  			snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
> -				 conf->no_parm_names ? "" : " ");
> +				 pconf->no_parm_names ? "" : " ");
>  		}
>  		break;
>  	case DW_TAG_array_type:
> 
> 
> With it:
> 
> ⬢[acme@toolbox pahole]$ btfdiff vmlinux
> ⬢[acme@toolbox pahole]$
> 

thanks for finding and fixing this!

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Eduard Zingerman]

On Fri, 2023-03-10 at 14:50 +0000, Alan Maguire wrote:
> When doing BTF comparisons between functions defined in multiple
> CUs, it was noticed a few critical functions failed prototype
> comparisons due to multiple "const" modifiers; for example:
> 
> function mismatch for 'memchr_inv'('memchr_inv'): 'void * ()(const const void  * , int, size_t)' != 'void * ()(const void  *, int, size_t)'
> 
> function mismatch for 'strnlen'('strnlen'): '__kernel_size_t ()(const const char  * , __kernel_size_t)' != '__kernel_size_t ()(const char  *, size_t)'
> 
> (note the "const const" in the first parameter.)

Hi Alan,

Could you please share which command/flags do you use to generate the
'memchr_inv' with 'const const'?
I tried the ones used in 'btfdiff':
- pahole -F dwarf  --flat_arrays --sort --jobs --suppress_aligned_attribute \
  --suppress_force_paddings --suppress_packed --lang_exclude rust \
  --show_private_classes ./vmlinux
- pahole -F btf --sort --suppress_aligned_attribute --suppress_packed ./vmlinux

But don't see any function prototypes generated with 'const const'.

On the other hand, I see it in a few structure definitions, e.g. here
is original C code (include/linux/sysrq.h:32):

    struct sysrq_key_op {
    	void (* const handler)(int);
    	const char * const help_msg;
    	const char * const action_msg;
    	const int enable_mask;
    };

And here is how it is reconstructed from DWARF (same happens when
reconstructed from BTF):

    struct sysrq_key_op {
            const void                 (*handler)(int);      /*     0     8 */
            const const char  *        help_msg;             /*     8     8 */
            const const char  *        action_msg;           /*    16     8 */
            const int                  enable_mask;          /*    24     4 */
    
            /* size: 32, cachelines: 1, members: 4 */
            /* padding: 4 */
            /* last cacheline: 32 bytes */
    };

So it seems to be a general issue with modifiers printing.

Thanks,
Eduard
> 
> As such it would be useful to omit modifiers for comparison
> purposes.  Also noted was the fact that for the "no_parm_names"
> case, an extra space was being emitted in some cases, also
> throwing off string comparisons of prototypes.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  dwarves.h         |  1 +
>  dwarves_fprintf.c | 26 ++++++++++++++++----------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/dwarves.h b/dwarves.h
> index d04a36d..7a319d1 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -134,6 +134,7 @@ struct conf_fprintf {
>  	uint8_t	   strip_inline:1;
>  	uint8_t	   skip_emitting_atomic_typedefs:1;
>  	uint8_t	   skip_emitting_errors:1;
> +	uint8_t    skip_emitting_modifier:1;
>  };
>  
>  struct cus;
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index 5c6bf9c..b20a473 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
>  				struct tag *next_type = cu__type(cu, type->type);
>  
>  				if (next_type && tag__is_pointer(next_type)) {
> -					const_pointer = "const ";
> +					if (!conf->skip_emitting_modifier)
> +						const_pointer = "const ";
>  					type = next_type;
>  				}
>  			}
> @@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>  				   *type_str = __tag__name(type, cu, tmpbf,
>  							   sizeof(tmpbf),
>  							   pconf);
> -			switch (tag->tag) {
> -			case DW_TAG_volatile_type: prefix = "volatile "; break;
> -			case DW_TAG_const_type:    prefix = "const ";	 break;
> -			case DW_TAG_restrict_type: suffix = " restrict"; break;
> -			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> +			if (!conf->skip_emitting_modifier) {
> +				switch (tag->tag) {
> +				case DW_TAG_volatile_type: prefix = "volatile "; break;
> +				case DW_TAG_const_type: prefix = "const"; break;
> +				case DW_TAG_restrict_type: suffix = " restrict"; break;
> +				case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> +				}
>  			}
> -			snprintf(bf, len, "%s%s%s ", prefix, type_str, suffix);
> +			snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
> +				 conf->no_parm_names ? "" : " ");
>  		}
>  		break;
>  	case DW_TAG_array_type:
> @@ -818,9 +822,11 @@ print_default:
>  	case DW_TAG_const_type:
>  		modifier = "const";
>  print_modifier: {
> -		size_t modifier_printed = fprintf(fp, "%s ", modifier);
> -		tconf.type_spacing -= modifier_printed;
> -		printed		   += modifier_printed;
> +		if (!conf->skip_emitting_modifier) {
> +			size_t modifier_printed = fprintf(fp, "%s ", modifier);
> +			tconf.type_spacing -= modifier_printed;
> +			printed		   += modifier_printed;
> +		}
>  
>  		struct tag *ttype = cu__type(cu, type->type);
>  		if (ttype) {

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Alan Maguire]

On 13/03/2023 13:50, Eduard Zingerman wrote:
> On Fri, 2023-03-10 at 14:50 +0000, Alan Maguire wrote:
>> When doing BTF comparisons between functions defined in multiple
>> CUs, it was noticed a few critical functions failed prototype
>> comparisons due to multiple "const" modifiers; for example:
>>
>> function mismatch for 'memchr_inv'('memchr_inv'): 'void * ()(const const void  * , int, size_t)' != 'void * ()(const void  *, int, size_t)'
>>
>> function mismatch for 'strnlen'('strnlen'): '__kernel_size_t ()(const const char  * , __kernel_size_t)' != '__kernel_size_t ()(const char  *, size_t)'
>>
>> (note the "const const" in the first parameter.)
> 
> Hi Alan,
> 
> Could you please share which command/flags do you use to generate the
> 'memchr_inv' with 'const const'?


sure; try adding "--skip_encoding_btf_inconsistent_proto --btf_gen_optimized".
I was testing with gcc 11.2.1.
 
> I tried the ones used in 'btfdiff':
> - pahole -F dwarf  --flat_arrays --sort --jobs --suppress_aligned_attribute \
>   --suppress_force_paddings --suppress_packed --lang_exclude rust \
>   --show_private_classes ./vmlinux
> - pahole -F btf --sort --suppress_aligned_attribute --suppress_packed ./vmlinux
> 
> But don't see any function prototypes generated with 'const const'.
> 
> On the other hand, I see it in a few structure definitions, e.g. here
> is original C code (include/linux/sysrq.h:32):
> 
>     struct sysrq_key_op {
>     	void (* const handler)(int);
>     	const char * const help_msg;
>     	const char * const action_msg;
>     	const int enable_mask;
>     };
> 
> And here is how it is reconstructed from DWARF (same happens when
> reconstructed from BTF):
> 
>     struct sysrq_key_op {
>             const void                 (*handler)(int);      /*     0     8 */
>             const const char  *        help_msg;             /*     8     8 */
>             const const char  *        action_msg;           /*    16     8 */
>             const int                  enable_mask;          /*    24     4 */
>     
>             /* size: 32, cachelines: 1, members: 4 */
>             /* padding: 4 */
>             /* last cacheline: 32 bytes */
>     };
> 
> So it seems to be a general issue with modifiers printing.
> 

So it seems like the modifier ordering isn't preserved, even though
the final BTF representation looks right? Thanks!

Alan

> Thanks,
> Eduard
>>
>> As such it would be useful to omit modifiers for comparison
>> purposes.  Also noted was the fact that for the "no_parm_names"
>> case, an extra space was being emitted in some cases, also
>> throwing off string comparisons of prototypes.
>>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  dwarves.h         |  1 +
>>  dwarves_fprintf.c | 26 ++++++++++++++++----------
>>  2 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/dwarves.h b/dwarves.h
>> index d04a36d..7a319d1 100644
>> --- a/dwarves.h
>> +++ b/dwarves.h
>> @@ -134,6 +134,7 @@ struct conf_fprintf {
>>  	uint8_t	   strip_inline:1;
>>  	uint8_t	   skip_emitting_atomic_typedefs:1;
>>  	uint8_t	   skip_emitting_errors:1;
>> +	uint8_t    skip_emitting_modifier:1;
>>  };
>>  
>>  struct cus;
>> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
>> index 5c6bf9c..b20a473 100644
>> --- a/dwarves_fprintf.c
>> +++ b/dwarves_fprintf.c
>> @@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
>>  				struct tag *next_type = cu__type(cu, type->type);
>>  
>>  				if (next_type && tag__is_pointer(next_type)) {
>> -					const_pointer = "const ";
>> +					if (!conf->skip_emitting_modifier)
>> +						const_pointer = "const ";
>>  					type = next_type;
>>  				}
>>  			}
>> @@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>>  				   *type_str = __tag__name(type, cu, tmpbf,
>>  							   sizeof(tmpbf),
>>  							   pconf);
>> -			switch (tag->tag) {
>> -			case DW_TAG_volatile_type: prefix = "volatile "; break;
>> -			case DW_TAG_const_type:    prefix = "const ";	 break;
>> -			case DW_TAG_restrict_type: suffix = " restrict"; break;
>> -			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
>> +			if (!conf->skip_emitting_modifier) {
>> +				switch (tag->tag) {
>> +				case DW_TAG_volatile_type: prefix = "volatile "; break;
>> +				case DW_TAG_const_type: prefix = "const"; break;
>> +				case DW_TAG_restrict_type: suffix = " restrict"; break;
>> +				case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
>> +				}
>>  			}
>> -			snprintf(bf, len, "%s%s%s ", prefix, type_str, suffix);
>> +			snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
>> +				 conf->no_parm_names ? "" : " ");
>>  		}
>>  		break;
>>  	case DW_TAG_array_type:
>> @@ -818,9 +822,11 @@ print_default:
>>  	case DW_TAG_const_type:
>>  		modifier = "const";
>>  print_modifier: {
>> -		size_t modifier_printed = fprintf(fp, "%s ", modifier);
>> -		tconf.type_spacing -= modifier_printed;
>> -		printed		   += modifier_printed;
>> +		if (!conf->skip_emitting_modifier) {
>> +			size_t modifier_printed = fprintf(fp, "%s ", modifier);
>> +			tconf.type_spacing -= modifier_printed;
>> +			printed		   += modifier_printed;
>> +		}
>>  
>>  		struct tag *ttype = cu__type(cu, type->type);
>>  		if (ttype) {
> 

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Eduard Zingerman]

On Mon, 2023-03-13 at 16:37 +0000, Alan Maguire wrote:
[...]
> sure; try adding "--skip_encoding_btf_inconsistent_proto --btf_gen_optimized".
> I was testing with gcc 11.2.1.

pahole -F dwarf \
       --flat_arrays \
       --sort --jobs \
       --suppress_aligned_attribute \
       --suppress_force_paddings \
       --suppress_packed \
       --lang_exclude rust \
       --show_private_classes \
       --skip_encoding_btf_inconsistent_proto \
       --btf_gen_optimized \
       ./vmlinux

Like this, right?
gcc 11.3, pahole master, still don't see this in function prototypes,
maybe I have a simpler kernel config...

[...]

> > On the other hand, I see it in a few structure definitions, e.g. here
> > is original C code (include/linux/sysrq.h:32):
> > 
> >     struct sysrq_key_op {
> >     	void (* const handler)(int);
> >     	const char * const help_msg;
> >     	const char * const action_msg;
> >     	const int enable_mask;
> >     };
> > 
> > And here is how it is reconstructed from DWARF (same happens when
> > reconstructed from BTF):
> > 
> >     struct sysrq_key_op {
> >             const void                 (*handler)(int);      /*     0     8 */
> >             const const char  *        help_msg;             /*     8     8 */
> >             const const char  *        action_msg;           /*    16     8 */
> >             const int                  enable_mask;          /*    24     4 */
> >     
> >             /* size: 32, cachelines: 1, members: 4 */
> >             /* padding: 4 */
> >             /* last cacheline: 32 bytes */
> >     };
> > 
> > So it seems to be a general issue with modifiers printing.
> > 
> 
> So it seems like the modifier ordering isn't preserved, even though
> the final BTF representation looks right? Thanks!

Yes, BTF looks right, bpftool prints the structure correctly.

[...]

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Arnaldo Carvalho de Melo]

Em Mon, Mar 13, 2023 at 07:12:43PM +0200, Eduard Zingerman escreveu:
> On Mon, 2023-03-13 at 16:37 +0000, Alan Maguire wrote:
> [...]
> > sure; try adding "--skip_encoding_btf_inconsistent_proto --btf_gen_optimized".
> > I was testing with gcc 11.2.1.
> 
> pahole -F dwarf \
>        --flat_arrays \
>        --sort --jobs \
>        --suppress_aligned_attribute \
>        --suppress_force_paddings \
>        --suppress_packed \
>        --lang_exclude rust \
>        --show_private_classes \
>        --skip_encoding_btf_inconsistent_proto \
>        --btf_gen_optimized \
>        ./vmlinux
> 
> Like this, right?
> gcc 11.3, pahole master, still don't see this in function prototypes,
> maybe I have a simpler kernel config...
> 
> [...]
> 
> > > On the other hand, I see it in a few structure definitions, e.g. here
> > > is original C code (include/linux/sysrq.h:32):
> > > 
> > >     struct sysrq_key_op {
> > >     	void (* const handler)(int);
> > >     	const char * const help_msg;
> > >     	const char * const action_msg;
> > >     	const int enable_mask;
> > >     };
> > > 
> > > And here is how it is reconstructed from DWARF (same happens when
> > > reconstructed from BTF):
> > > 
> > >     struct sysrq_key_op {
> > >             const void                 (*handler)(int);      /*     0     8 */
> > >             const const char  *        help_msg;             /*     8     8 */
> > >             const const char  *        action_msg;           /*    16     8 */
> > >             const int                  enable_mask;          /*    24     4 */
> > >     
> > >             /* size: 32, cachelines: 1, members: 4 */
> > >             /* padding: 4 */
> > >             /* last cacheline: 32 bytes */
> > >     };
> > > 
> > > So it seems to be a general issue with modifiers printing.
> > > 
> > 
> > So it seems like the modifier ordering isn't preserved, even though
> > the final BTF representation looks right? Thanks!
> 
> Yes, BTF looks right, bpftool prints the structure correctly.

Yes, the problem is in pahole's fprintf.c code

⬢[acme@toolbox pahole]$ cat const-pointer-const.c
#include <stdio.h>

struct foo {
	const char * const s;
};

int main(int argc, const char *argv[])
{
	struct foo bar = { .s = argv[1], };
	return printf("%s: %s\n", argv[0], bar.s);
}
⬢[acme@toolbox pahole]$ gcc -g const-pointer-const.c -o const-pointer-const
⬢[acme@toolbox pahole]$ pahole const-pointer-const
struct foo {
	const constchar  *         s;                    /*     0     8 */

	/* size: 8, cachelines: 1, members: 1 */
	/* last cacheline: 8 bytes */
};
⬢[acme@toolbox pahole]$


Seems a long standing bug, so if you fix the whitespace issue we can
progress and not let this problem prevent the release of 1.25, agreed?

- Arnaldo

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Eduard Zingerman]

On Fri, 2023-03-10 at 14:50 +0000, Alan Maguire wrote:
[...]
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index 5c6bf9c..b20a473 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
>  				struct tag *next_type = cu__type(cu, type->type);
>  
>  				if (next_type && tag__is_pointer(next_type)) {
> -					const_pointer = "const ";
> +					if (!conf->skip_emitting_modifier)
> +						const_pointer = "const ";
>  					type = next_type;
>  				}
>  			}
> @@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>  				   *type_str = __tag__name(type, cu, tmpbf,
>  							   sizeof(tmpbf),
>  							   pconf);
> -			switch (tag->tag) {
> -			case DW_TAG_volatile_type: prefix = "volatile "; break;
> -			case DW_TAG_const_type:    prefix = "const ";	 break;
> -			case DW_TAG_restrict_type: suffix = " restrict"; break;
> -			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> +			if (!conf->skip_emitting_modifier) {
> +				switch (tag->tag) {
> +				case DW_TAG_volatile_type: prefix = "volatile "; break;
> +				case DW_TAG_const_type: prefix = "const"; break;

Here the space is removed from literal "const " and this results in
the following output (`pahole -F btf --sort ./vmlinux`):

    struct ZSTD_inBuffer_s {
            constvoid  *               src;                  /*     0     8 */
            ...
    };

(Sorry for late replies).

[...]

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Alan Maguire]

On 13/03/2023 14:45, Eduard Zingerman wrote:
> On Fri, 2023-03-10 at 14:50 +0000, Alan Maguire wrote:
> [...]
>> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
>> index 5c6bf9c..b20a473 100644
>> --- a/dwarves_fprintf.c
>> +++ b/dwarves_fprintf.c
>> @@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
>>  				struct tag *next_type = cu__type(cu, type->type);
>>  
>>  				if (next_type && tag__is_pointer(next_type)) {
>> -					const_pointer = "const ";
>> +					if (!conf->skip_emitting_modifier)
>> +						const_pointer = "const ";
>>  					type = next_type;
>>  				}
>>  			}
>> @@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>>  				   *type_str = __tag__name(type, cu, tmpbf,
>>  							   sizeof(tmpbf),
>>  							   pconf);
>> -			switch (tag->tag) {
>> -			case DW_TAG_volatile_type: prefix = "volatile "; break;
>> -			case DW_TAG_const_type:    prefix = "const ";	 break;
>> -			case DW_TAG_restrict_type: suffix = " restrict"; break;
>> -			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
>> +			if (!conf->skip_emitting_modifier) {
>> +				switch (tag->tag) {
>> +				case DW_TAG_volatile_type: prefix = "volatile "; break;
>> +				case DW_TAG_const_type: prefix = "const"; break;
> 
> Here the space is removed from literal "const " and this results in
> the following output (`pahole -F btf --sort ./vmlinux`):
> 
>     struct ZSTD_inBuffer_s {
>             constvoid  *               src;                  /*     0     8 */
>             ...
>     };
> 

great catch, thanks Eduard! Arnaldo will I send a followup patch for this?

> (Sorry for late replies).
> 
> [...]
> 

Re: [PATCH dwarves 2/3] dwarves_fprintf: support skipping modifier

[Arnaldo Carvalho de Melo]

Em Mon, Mar 13, 2023 at 05:18:28PM +0000, Alan Maguire escreveu:
> On 13/03/2023 14:45, Eduard Zingerman wrote:
> > On Fri, 2023-03-10 at 14:50 +0000, Alan Maguire wrote:
> > [...]
> >> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> >> index 5c6bf9c..b20a473 100644
> >> --- a/dwarves_fprintf.c
> >> +++ b/dwarves_fprintf.c
> >> @@ -506,7 +506,8 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
> >>  				struct tag *next_type = cu__type(cu, type->type);
> >>  
> >>  				if (next_type && tag__is_pointer(next_type)) {
> >> -					const_pointer = "const ";
> >> +					if (!conf->skip_emitting_modifier)
> >> +						const_pointer = "const ";
> >>  					type = next_type;
> >>  				}
> >>  			}
> >> @@ -580,13 +581,16 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> >>  				   *type_str = __tag__name(type, cu, tmpbf,
> >>  							   sizeof(tmpbf),
> >>  							   pconf);
> >> -			switch (tag->tag) {
> >> -			case DW_TAG_volatile_type: prefix = "volatile "; break;
> >> -			case DW_TAG_const_type:    prefix = "const ";	 break;
> >> -			case DW_TAG_restrict_type: suffix = " restrict"; break;
> >> -			case DW_TAG_atomic_type:   prefix = "_Atomic ";  break;
> >> +			if (!conf->skip_emitting_modifier) {
> >> +				switch (tag->tag) {
> >> +				case DW_TAG_volatile_type: prefix = "volatile "; break;
> >> +				case DW_TAG_const_type: prefix = "const"; break;
> > 
> > Here the space is removed from literal "const " and this results in
> > the following output (`pahole -F btf --sort ./vmlinux`):
> > 
> >     struct ZSTD_inBuffer_s {
> >             constvoid  *               src;                  /*     0     8 */
> >             ...
> >     };
> > 
> 
> great catch, thanks Eduard! Arnaldo will I send a followup patch for this?

Would be interesting to fold it into the one introducing the problem,
since I didn't push this to master yet.

You can send the patch and I can fold it, so that I keep that fixe I
made.

- Arnaldo

[PATCH dwarves 3/3] btf_encoder: compare functions via prototypes not parameter names

[Alan Maguire]

Parameter names are a brittle choice for comparing function prototypes.
As noted by Jiri [1], a function can have a weak definition in one
CU with differing names from another definition, and because we require
name-based matches, we can omit functions unnecessarily.  Using a
conf_fprintf that omits parameter names, generate function prototypes
for string comparison instead.

[1] https://lore.kernel.org/bpf/ZAsBYpsBV0wvkhh0@krava/

Reported-by: Jira Olsa <olsajiri@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c | 67 +++++++++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 07a9dc5..65f6e71 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -33,13 +33,13 @@
 #include <search.h> /* for tsearch(), tfind() and tdestroy() */
 #include <pthread.h>
 
-#define BTF_ENCODER_MAX_PARAMETERS	12
+#define BTF_ENCODER_MAX_PROTO	512
 
 /* state used to do later encoding of saved functions */
 struct btf_encoder_state {
 	uint32_t type_id_off;
-	bool got_parameter_names;
-	const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
+	bool got_proto;
+	char proto[BTF_ENCODER_MAX_PROTO];
 };
 
 struct elf_function {
@@ -798,25 +798,25 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
 	return id;
 }
 
-static void parameter_names__get(struct ftype *ftype, size_t nr_parameters,
-				 const char **parameter_names)
+static bool proto__get(struct function *func, char *proto, size_t len)
 {
-	struct parameter *parameter;
-	int i = 0;
-
-	ftype__for_each_parameter(ftype, parameter) {
-		if (i >= nr_parameters)
-			return;
-		parameter_names[i++] = parameter__name(parameter);
-	}
+	const struct conf_fprintf conf = {
+						.name_spacing = 23,
+						.type_spacing = 26,
+						.emit_stats = 0,
+						.no_parm_names = 1,
+						.skip_emitting_errors = 1,
+						.skip_emitting_modifier = 1,
+					};
+
+	return function__prototype_conf(func, func->priv, &conf, proto, len) != NULL;
 }
 
 static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func, struct function *f2)
 {
-	const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
+	char proto[BTF_ENCODER_MAX_PROTO];
 	struct function *f1 = func->function;
 	const char *name;
-	int i;
 
 	if (!f1)
 		return false;
@@ -833,33 +833,27 @@ static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
 	if (f1->proto.nr_parms == 0)
 		return true;
 
-	if (!func->state.got_parameter_names) {
-		parameter_names__get(&f1->proto, BTF_ENCODER_MAX_PARAMETERS,
-				     func->state.parameter_names);
-		func->state.got_parameter_names = true;
-	}
-	parameter_names__get(&f2->proto, BTF_ENCODER_MAX_PARAMETERS, parameter_names);
-	for (i = 0; i < f1->proto.nr_parms && i < BTF_ENCODER_MAX_PARAMETERS; i++) {
-		if (!func->state.parameter_names[i]) {
-			if (!parameter_names[i])
-				continue;
-		} else if (parameter_names[i]) {
-			if (strcmp(func->state.parameter_names[i], parameter_names[i]) == 0)
-				continue;
-		}
-		if (encoder->verbose) {
-			printf("function mismatch for '%s'(%s): parameter #%d '%s' != '%s'\n",
-			       name, f1->alias ?: name, i,
-			       func->state.parameter_names[i] ?: "<null>",
-			       parameter_names[i] ?: "<null>");
+	if (f1->proto.tag.type == f2->proto.tag.type)
+		return true;
+
+	if (!func->state.got_proto)
+		func->state.got_proto = proto__get(f1, func->state.proto, sizeof(func->state.proto));
+
+	if (proto__get(f2, proto, sizeof(proto))) {
+		if (strcmp(func->state.proto, proto) != 0) {
+			if (encoder->verbose)
+				printf("function mismatch for '%s'('%s'): '%s' != '%s'\n",
+				       name, f1->alias ?: name,
+				       func->state.proto, proto);
+			return false;
 		}
-		return false;
 	}
 	return true;
 }
 
 static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
 {
+	fn->priv = encoder->cu;
 	if (func->function) {
 		struct function *existing = func->function;
 
@@ -1022,7 +1016,8 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
 	}
 	encoder->functions.entries[encoder->functions.cnt].generated = false;
 	encoder->functions.entries[encoder->functions.cnt].function = NULL;
-	encoder->functions.entries[encoder->functions.cnt].state.got_parameter_names = false;
+	encoder->functions.entries[encoder->functions.cnt].state.got_proto = false;
+	encoder->functions.entries[encoder->functions.cnt].state.proto[0] = '\0';
 	encoder->functions.entries[encoder->functions.cnt].state.type_id_off = 0;
 	encoder->functions.cnt++;
 	return 0;
-- 
1.8.3.1

Re: [PATCH dwarves 0/3] dwarves: improve BTF encoder comparison method

[Arnaldo Carvalho de Melo]

Em Fri, Mar 10, 2023 at 02:50:47PM +0000, Alan Maguire escreveu:
> Currently when looking for function prototype mismatches with a view
> to excluding inconsistent functions, we fall back to a comparison
> between parameter names when the name and number of parameters match.
> This is brittle, as it is sometimes the case that a function has
> multiple type-identical definitions which use different parameters.
> 
> Here the existing dwarves_fprintf functionality is re-used to instead
> create a string representation of the function prototype - minus the
> parameter names - to support a less brittle comparison method.
> 
> To support this, patch 1 generalizes function prototype print to
> take a conf_fprintf parameter; this allows us to customize the
> parameters we use in prototype string generation.
> 
> Patch 2 supports generating prototypes without modifiers such
> as const as they can lead to false positive prototype mismatches;
> see the patch for details.
> 
> Finally patch 3 replaces the logic used to compare parameter
> names with the prototype string comparison instead.
> 
> Using verbose pahole output we can see some of the rejected
> comparisons.  73 comparisons are rejected via prototype
> comparison, 63 of which are non "."-suffixed functions.  For
> example:
> 
> function mismatch for 'name_show'('name_show'): 'ssize_t ()(struct kobject *, struct kobj_attribute *, char *)' != 'ssize_t ()(struct device *, struct device_attribute *, char *)'
> 
> With these changes, the syscalls defined in sys_ni.c
> that Jiri mentioned were missing [1] are present in BTF:
> 
> [43071] FUNC '__ia32_compat_sys_io_setup' type_id=42335 linkage=static
> [43295] FUNC '__ia32_sys_io_setup' type_id=42335 linkage=static
> [47536] FUNC '__x64_sys_io_setup' type_id=42335 linkage=static
> 
> [43290] FUNC '__ia32_sys_io_destroy' type_id=42335 linkage=static
> [47531] FUNC '__x64_sys_io_destroy' type_id=42335 linkage=static
> 
> [43072] FUNC '__ia32_compat_sys_io_submit' type_id=42335 linkage=static
> [43296] FUNC '__ia32_sys_io_submit' type_id=42335 linkage=static
> [47537] FUNC '__x64_sys_io_submit' type_id=42335 linkage=static
> 
> [1] https://lore.kernel.org/bpf/ZAsBYpsBV0wvkhh0@krava/

I'll test this now, but b4 isn't liking the way you sent it:

⬢[acme@toolbox pahole]$ b4 am -ctsl --cc-trailers 1678459850-16140-2-git-send-email-alan.maguire@oracle.com
Grabbing thread from lore.kernel.org/all/1678459850-16140-2-git-send-email-alan.maguire%40oracle.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 2 messages in the thread
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH 1/3] dwarves_fprintf: generalize function prototype print to support passing conf
    ✓ Signed: DKIM/oracle.com
    + Link: https://lore.kernel.org/r/1678459850-16140-2-git-send-email-alan.maguire@oracle.com
    + Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
  ERROR: missing [2/3]!
  ERROR: missing [3/3]!
---
Total patches: 1
---
WARNING: Thread incomplete!
Cover: ./20230310_alan_maguire_dwarves_improve_btf_encoder_comparison_method.cover
 Link: https://lore.kernel.org/r/1678459850-16140-1-git-send-email-alan.maguire@oracle.com
 Base: applies clean to current tree
       git checkout -b 20230310_alan_maguire_oracle_com HEAD
       git am ./20230310_alan_maguire_dwarves_improve_btf_encoder_comparison_method.mbx
⬢[acme@toolbox pahole]$

I'll apply one by one
 
> Alan Maguire (3):
>   dwarves_fprintf: generalize function prototype print to support
>     passing conf
>   dwarves_fprintf: support skipping modifier
>   btf_encoder: compare functions via prototypes not parameter names
> 
>  btf_encoder.c     | 67 +++++++++++++++++++++++++------------------------------
>  dwarves.h         |  6 +++++
>  dwarves_fprintf.c | 48 ++++++++++++++++++++++++++-------------
>  3 files changed, 70 insertions(+), 51 deletions(-)
> 
> -- 
> 1.8.3.1
> 

-- 

- Arnaldo

Re: [PATCH dwarves 0/3] dwarves: improve BTF encoder comparison method

[Jiri Olsa]

On Fri, Mar 10, 2023 at 02:50:47PM +0000, Alan Maguire wrote:
> Currently when looking for function prototype mismatches with a view
> to excluding inconsistent functions, we fall back to a comparison
> between parameter names when the name and number of parameters match.
> This is brittle, as it is sometimes the case that a function has
> multiple type-identical definitions which use different parameters.
> 
> Here the existing dwarves_fprintf functionality is re-used to instead
> create a string representation of the function prototype - minus the
> parameter names - to support a less brittle comparison method.
> 
> To support this, patch 1 generalizes function prototype print to
> take a conf_fprintf parameter; this allows us to customize the
> parameters we use in prototype string generation.
> 
> Patch 2 supports generating prototypes without modifiers such
> as const as they can lead to false positive prototype mismatches;
> see the patch for details.
> 
> Finally patch 3 replaces the logic used to compare parameter
> names with the prototype string comparison instead.
> 
> Using verbose pahole output we can see some of the rejected
> comparisons.  73 comparisons are rejected via prototype
> comparison, 63 of which are non "."-suffixed functions.  For
> example:
> 
> function mismatch for 'name_show'('name_show'): 'ssize_t ()(struct kobject *, struct kobj_attribute *, char *)' != 'ssize_t ()(struct device *, struct device_attribute *, char *)'
> 
> With these changes, the syscalls defined in sys_ni.c
> that Jiri mentioned were missing [1] are present in BTF:
> 
> [43071] FUNC '__ia32_compat_sys_io_setup' type_id=42335 linkage=static
> [43295] FUNC '__ia32_sys_io_setup' type_id=42335 linkage=static
> [47536] FUNC '__x64_sys_io_setup' type_id=42335 linkage=static
> 
> [43290] FUNC '__ia32_sys_io_destroy' type_id=42335 linkage=static
> [47531] FUNC '__x64_sys_io_destroy' type_id=42335 linkage=static
> 
> [43072] FUNC '__ia32_compat_sys_io_submit' type_id=42335 linkage=static
> [43296] FUNC '__ia32_sys_io_submit' type_id=42335 linkage=static
> [47537] FUNC '__x64_sys_io_submit' type_id=42335 linkage=static
> 
> [1] https://lore.kernel.org/bpf/ZAsBYpsBV0wvkhh0@krava/
> 
> Alan Maguire (3):
>   dwarves_fprintf: generalize function prototype print to support
>     passing conf
>   dwarves_fprintf: support skipping modifier
>   btf_encoder: compare functions via prototypes not parameter names

lgtm, the syscalls from sys_ni.c are there

for me the total number of syscalls increased from 249 to 432, great ;-)

Acked/Tested-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
>  btf_encoder.c     | 67 +++++++++++++++++++++++++------------------------------
>  dwarves.h         |  6 +++++
>  dwarves_fprintf.c | 48 ++++++++++++++++++++++++++-------------
>  3 files changed, 70 insertions(+), 51 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [PATCH dwarves 0/3] dwarves: improve BTF encoder comparison method

[Arnaldo Carvalho de Melo]

Em Mon, Mar 13, 2023 at 10:40:53AM +0100, Jiri Olsa escreveu:
> On Fri, Mar 10, 2023 at 02:50:47PM +0000, Alan Maguire wrote:
> > Currently when looking for function prototype mismatches with a view
> > to excluding inconsistent functions, we fall back to a comparison
> > between parameter names when the name and number of parameters match.
> > This is brittle, as it is sometimes the case that a function has
> > multiple type-identical definitions which use different parameters.
> > 
> > Here the existing dwarves_fprintf functionality is re-used to instead
> > create a string representation of the function prototype - minus the
> > parameter names - to support a less brittle comparison method.
> > 
> > To support this, patch 1 generalizes function prototype print to
> > take a conf_fprintf parameter; this allows us to customize the
> > parameters we use in prototype string generation.
> > 
> > Patch 2 supports generating prototypes without modifiers such
> > as const as they can lead to false positive prototype mismatches;
> > see the patch for details.
> > 
> > Finally patch 3 replaces the logic used to compare parameter
> > names with the prototype string comparison instead.
> > 
> > Using verbose pahole output we can see some of the rejected
> > comparisons.  73 comparisons are rejected via prototype
> > comparison, 63 of which are non "."-suffixed functions.  For
> > example:
> > 
> > function mismatch for 'name_show'('name_show'): 'ssize_t ()(struct kobject *, struct kobj_attribute *, char *)' != 'ssize_t ()(struct device *, struct device_attribute *, char *)'
> > 
> > With these changes, the syscalls defined in sys_ni.c
> > that Jiri mentioned were missing [1] are present in BTF:
> > 
> > [43071] FUNC '__ia32_compat_sys_io_setup' type_id=42335 linkage=static
> > [43295] FUNC '__ia32_sys_io_setup' type_id=42335 linkage=static
> > [47536] FUNC '__x64_sys_io_setup' type_id=42335 linkage=static
> > 
> > [43290] FUNC '__ia32_sys_io_destroy' type_id=42335 linkage=static
> > [47531] FUNC '__x64_sys_io_destroy' type_id=42335 linkage=static
> > 
> > [43072] FUNC '__ia32_compat_sys_io_submit' type_id=42335 linkage=static
> > [43296] FUNC '__ia32_sys_io_submit' type_id=42335 linkage=static
> > [47537] FUNC '__x64_sys_io_submit' type_id=42335 linkage=static
> > 
> > [1] https://lore.kernel.org/bpf/ZAsBYpsBV0wvkhh0@krava/
> > 
> > Alan Maguire (3):
> >   dwarves_fprintf: generalize function prototype print to support
> >     passing conf
> >   dwarves_fprintf: support skipping modifier
> >   btf_encoder: compare functions via prototypes not parameter names
> 
> lgtm, the syscalls from sys_ni.c are there
> 
> for me the total number of syscalls increased from 249 to 432, great ;-)
> 
> Acked/Tested-by: Jiri Olsa <jolsa@kernel.org>
> 
> thanks,
> jirka

Thanks, applied and pushed to the 'next' branch for some more testing.

I really need to get 1.25 out of the door, due to other issue that crops
up with binutils 2.40 (DW_TAG_unspecified_type), and will be travelling
the last two days of this week, so any further testing is more than
welcome.

- Arnaldo