| Message ID | 1678459850-16140-3-git-send-email-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | dwarves: improve BTF encoder comparison method | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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
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!
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) {
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). [...]
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) { >
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. [...]
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). > > [...] >
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
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
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) {
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(-)