| Message ID | 1678459850-16140-1-git-send-email-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | dwarves: improve BTF encoder comparison method | expand |
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 >
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 >
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
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(-)