| Message ID | 1674567931-26458-1-git-send-email-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions | expand |
On 24/01/2023 15:14, Jiri Olsa wrote: > On Tue, Jan 24, 2023 at 01:45:26PM +0000, Alan Maguire wrote: >> At optimization level -O2 or higher in gcc, static functions may be >> optimized such that they have suffixes like .isra.0, .constprop.0 etc. >> These represent >> >> - constant propagation (.constprop.0); >> - interprocedural scalar replacement of aggregates, removal of >> unused parameters and replacement of parameters passed by >> reference by parameters passed by value (.isra.0) >> >> See [1] for details. >> >> Currently BTF encoding does not handle such optimized functions >> that get renamed with a "." suffix such as ".isra.0", ".constprop.0". >> This is safer because such suffixes can often indicate parameters have >> been optimized out. This series addresses this by matching a >> function to a suffixed version ("foo" matching "foo.isra.0") while >> ensuring that the function signature does not contain optimized-out >> parameters. Note that if the function is found ("foo") it will >> be preferred, only falling back to "foo.isra.0" if lookup of the >> function fails. Addition to BTF is skipped if the function has >> optimized-out parameters, since the expected function signature >> will not match. BTF encoding does not include the "."-suffix to >> be consistent with DWARF. In addition, the kernel currently does >> not allow a "." suffix in a BTF function name. >> >> A problem with this approach however is that BTF carries out the >> encoding process in parallel across multiple CUs, and sometimes >> a function has optimized-out parameters in one CU but not others; >> we see this for NF_HOOK.constprop.0 for example. So in order to >> determine if the function has optimized-out parameters in any >> CU, its addition is not carried out until we have processed all >> CUs and are about to merge BTF. At this point we know if any >> such optimizations have occurred. Patches 1-4 handle the >> optimized-out parameter identification and matching "."-suffixed >> functions with the original function to facilitate BTF >> encoding. >> >> Patch 5 addresses a related problem - it is entirely possible >> for a static function of the same name to exist in different >> CUs with different function signatures. Because BTF does not >> currently encode any information that would help disambiguate >> which BTF function specification matches which static function >> (in the case of multiple different function signatures), it is >> best to eliminate such functions from BTF for now. The same >> mechanism that is used to compare static "."-suffixed functions >> is re-used for the static function comparison. A superficial >> comparison of number of parameters/parameter names is done to >> see if such representations are consistent, and if inconsistent >> prototypes are observed, the function is flagged for exclusion >> from BTF. > > should we remove all instances of static functions with same name? > > Yonghong suggested in the other thread, that user will assume that > the function he's attached to is the one he expects, while he will > be attached to any (first) match we get from kallsyms_lookup_name > The problem is that many static functions that have consistent prototypes are encountered multiple times when processing CUs, even though some have only one System.map/kallsyms entry. I tweaked patch 5 to count how many times a function was encountered when compiling the tree of static functions. It turns out of the 25872 functions, 22608 are found once, leaving 3264 that are found multiple times. This would be a lot of functions to leave out I think, especially since many don't actually have multiple kallsyms entries to deal with and the prototype is consistent. BTW there's a bug in patch 5 that can cause a segmentation fault, apologies about this. I don't want to send a v2 yet as folks haven't had a chance to look at it properly but the fix is below. Thanks! Alan diff --git a/btf_encoder.c b/btf_encoder.c index ee0b032..e89c1a8 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -786,7 +786,7 @@ static int function__compare(const void *a, const void *b) return strcmp(function__name(fa), function__name(fb)); } -#define BTF_ENCODER_MAX_PARAMETERS 10 +#define BTF_ENCODER_MAX_PARAMETERS 12 struct btf_encoder_state { struct btf_encoder *encoder; @@ -869,7 +869,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *en } parameter_names__get(&fn->proto, BTF_ENCODER_MAX_PARAMET parameter_names); - for (i = 0; i < ofn->proto.nr_parms; i++) { + for (i = 0; i < ofn->proto.nr_parms && i < BTF_ENCODER_M if (!state->parameter_names[i]) { if (!parameter_names[i]) continue; > thanks, > jirka > >> >> When these methods are combined - the additive encoding of >> "."-suffixed functions and the subtractive elimination of >> functions with inconsistent parameters - we see a small overall >> increase in the number of functions in vmlinux BTF, from >> 49538 to 50083. It turns out that the inconsistent prototype >> checking will actually eliminate some of the suffix matches >> also, for cases where the DWARF representation of a function >> differs across CUs, but not via the abstract origin late DWARF >> references showing optimized-out parameters that we check >> for in patch 1. >> >> [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html >> >> Alan Maguire (5): >> dwarves: help dwarf loader spot functions with optimized-out >> parameters >> btf_encoder: refactor function addition into dedicated >> btf_encoder__add_func >> btf_encoder: child encoders should have a reference to parent encoder >> btf_encoder: represent "."-suffixed optimized functions (".isra.0") in >> BTF >> btf_encoder: skip BTF encoding of static functions with inconsistent >> prototypes >> >> btf_encoder.c | 357 +++++++++++++++++++++++++++++++++++++++++++++++++-------- >> btf_encoder.h | 2 +- >> dwarf_loader.c | 76 +++++++++++- >> dwarves.h | 5 +- >> pahole.c | 7 +- >> 5 files changed, 390 insertions(+), 57 deletions(-) >> >> -- >> 1.8.3.1 >>
On Tue, Jan 24, 2023 at 04:11:56PM +0000, Alan Maguire wrote: > On 24/01/2023 15:14, Jiri Olsa wrote: > > On Tue, Jan 24, 2023 at 01:45:26PM +0000, Alan Maguire wrote: > >> At optimization level -O2 or higher in gcc, static functions may be > >> optimized such that they have suffixes like .isra.0, .constprop.0 etc. > >> These represent > >> > >> - constant propagation (.constprop.0); > >> - interprocedural scalar replacement of aggregates, removal of > >> unused parameters and replacement of parameters passed by > >> reference by parameters passed by value (.isra.0) > >> > >> See [1] for details. > >> > >> Currently BTF encoding does not handle such optimized functions > >> that get renamed with a "." suffix such as ".isra.0", ".constprop.0". > >> This is safer because such suffixes can often indicate parameters have > >> been optimized out. This series addresses this by matching a > >> function to a suffixed version ("foo" matching "foo.isra.0") while > >> ensuring that the function signature does not contain optimized-out > >> parameters. Note that if the function is found ("foo") it will > >> be preferred, only falling back to "foo.isra.0" if lookup of the > >> function fails. Addition to BTF is skipped if the function has > >> optimized-out parameters, since the expected function signature > >> will not match. BTF encoding does not include the "."-suffix to > >> be consistent with DWARF. In addition, the kernel currently does > >> not allow a "." suffix in a BTF function name. > >> > >> A problem with this approach however is that BTF carries out the > >> encoding process in parallel across multiple CUs, and sometimes > >> a function has optimized-out parameters in one CU but not others; > >> we see this for NF_HOOK.constprop.0 for example. So in order to > >> determine if the function has optimized-out parameters in any > >> CU, its addition is not carried out until we have processed all > >> CUs and are about to merge BTF. At this point we know if any > >> such optimizations have occurred. Patches 1-4 handle the > >> optimized-out parameter identification and matching "."-suffixed > >> functions with the original function to facilitate BTF > >> encoding. > >> > >> Patch 5 addresses a related problem - it is entirely possible > >> for a static function of the same name to exist in different > >> CUs with different function signatures. Because BTF does not > >> currently encode any information that would help disambiguate > >> which BTF function specification matches which static function > >> (in the case of multiple different function signatures), it is > >> best to eliminate such functions from BTF for now. The same > >> mechanism that is used to compare static "."-suffixed functions > >> is re-used for the static function comparison. A superficial > >> comparison of number of parameters/parameter names is done to > >> see if such representations are consistent, and if inconsistent > >> prototypes are observed, the function is flagged for exclusion > >> from BTF. > > > > should we remove all instances of static functions with same name? > > > > Yonghong suggested in the other thread, that user will assume that > > the function he's attached to is the one he expects, while he will > > be attached to any (first) match we get from kallsyms_lookup_name > > > > The problem is that many static functions that have consistent > prototypes are encountered multiple times when processing CUs, > even though some have only one System.map/kallsyms entry. I tweaked patch 5 > to count how many times a function was encountered when compiling the > tree of static functions. It turns out of the 25872 functions, 22608 > are found once, leaving 3264 that are found multiple times. This would > be a lot of functions to leave out I think, especially since many > don't actually have multiple kallsyms entries to deal with and > the prototype is consistent. ok, I checked and it seems like this is the case for inlined functions, not sure what we can do here all the instances having same prototype might be enough for now jirka > > BTW there's a bug in patch 5 that can cause a segmentation fault, > apologies about this. I don't want to send a v2 yet as folks > haven't had a chance to look at it properly but the fix is below. > Thanks! > > Alan > > diff --git a/btf_encoder.c b/btf_encoder.c > index ee0b032..e89c1a8 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -786,7 +786,7 @@ static int function__compare(const void *a, const void *b) > return strcmp(function__name(fa), function__name(fb)); > } > > -#define BTF_ENCODER_MAX_PARAMETERS 10 > +#define BTF_ENCODER_MAX_PARAMETERS 12 > > struct btf_encoder_state { > struct btf_encoder *encoder; > @@ -869,7 +869,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *en > } > parameter_names__get(&fn->proto, BTF_ENCODER_MAX_PARAMET > parameter_names); > - for (i = 0; i < ofn->proto.nr_parms; i++) { > + for (i = 0; i < ofn->proto.nr_parms && i < BTF_ENCODER_M > if (!state->parameter_names[i]) { > if (!parameter_names[i]) > continue; > > > thanks, > > jirka > > > >> > >> When these methods are combined - the additive encoding of > >> "."-suffixed functions and the subtractive elimination of > >> functions with inconsistent parameters - we see a small overall > >> increase in the number of functions in vmlinux BTF, from > >> 49538 to 50083. It turns out that the inconsistent prototype > >> checking will actually eliminate some of the suffix matches > >> also, for cases where the DWARF representation of a function > >> differs across CUs, but not via the abstract origin late DWARF > >> references showing optimized-out parameters that we check > >> for in patch 1. > >> > >> [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > >> > >> Alan Maguire (5): > >> dwarves: help dwarf loader spot functions with optimized-out > >> parameters > >> btf_encoder: refactor function addition into dedicated > >> btf_encoder__add_func > >> btf_encoder: child encoders should have a reference to parent encoder > >> btf_encoder: represent "."-suffixed optimized functions (".isra.0") in > >> BTF > >> btf_encoder: skip BTF encoding of static functions with inconsistent > >> prototypes > >> > >> btf_encoder.c | 357 +++++++++++++++++++++++++++++++++++++++++++++++++-------- > >> btf_encoder.h | 2 +- > >> dwarf_loader.c | 76 +++++++++++- > >> dwarves.h | 5 +- > >> pahole.c | 7 +- > >> 5 files changed, 390 insertions(+), 57 deletions(-) > >> > >> -- > >> 1.8.3.1 > >>
At optimization level -O2 or higher in gcc, static functions may be optimized such that they have suffixes like .isra.0, .constprop.0 etc. These represent - constant propagation (.constprop.0); - interprocedural scalar replacement of aggregates, removal of unused parameters and replacement of parameters passed by reference by parameters passed by value (.isra.0) See [1] for details. Currently BTF encoding does not handle such optimized functions that get renamed with a "." suffix such as ".isra.0", ".constprop.0". This is safer because such suffixes can often indicate parameters have been optimized out. This series addresses this by matching a function to a suffixed version ("foo" matching "foo.isra.0") while ensuring that the function signature does not contain optimized-out parameters. Note that if the function is found ("foo") it will be preferred, only falling back to "foo.isra.0" if lookup of the function fails. Addition to BTF is skipped if the function has optimized-out parameters, since the expected function signature will not match. BTF encoding does not include the "."-suffix to be consistent with DWARF. In addition, the kernel currently does not allow a "." suffix in a BTF function name. A problem with this approach however is that BTF carries out the encoding process in parallel across multiple CUs, and sometimes a function has optimized-out parameters in one CU but not others; we see this for NF_HOOK.constprop.0 for example. So in order to determine if the function has optimized-out parameters in any CU, its addition is not carried out until we have processed all CUs and are about to merge BTF. At this point we know if any such optimizations have occurred. Patches 1-4 handle the optimized-out parameter identification and matching "."-suffixed functions with the original function to facilitate BTF encoding. Patch 5 addresses a related problem - it is entirely possible for a static function of the same name to exist in different CUs with different function signatures. Because BTF does not currently encode any information that would help disambiguate which BTF function specification matches which static function (in the case of multiple different function signatures), it is best to eliminate such functions from BTF for now. The same mechanism that is used to compare static "."-suffixed functions is re-used for the static function comparison. A superficial comparison of number of parameters/parameter names is done to see if such representations are consistent, and if inconsistent prototypes are observed, the function is flagged for exclusion from BTF. When these methods are combined - the additive encoding of "."-suffixed functions and the subtractive elimination of functions with inconsistent parameters - we see a small overall increase in the number of functions in vmlinux BTF, from 49538 to 50083. It turns out that the inconsistent prototype checking will actually eliminate some of the suffix matches also, for cases where the DWARF representation of a function differs across CUs, but not via the abstract origin late DWARF references showing optimized-out parameters that we check for in patch 1. [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html Alan Maguire (5): dwarves: help dwarf loader spot functions with optimized-out parameters btf_encoder: refactor function addition into dedicated btf_encoder__add_func btf_encoder: child encoders should have a reference to parent encoder btf_encoder: represent "."-suffixed optimized functions (".isra.0") in BTF btf_encoder: skip BTF encoding of static functions with inconsistent prototypes btf_encoder.c | 357 +++++++++++++++++++++++++++++++++++++++++++++++++-------- btf_encoder.h | 2 +- dwarf_loader.c | 76 +++++++++++- dwarves.h | 5 +- pahole.c | 7 +- 5 files changed, 390 insertions(+), 57 deletions(-)