| Message ID | 20201112150506.705430-4-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | btf_encoder: Fix functions BTF data generation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Nov 12, 2020 at 7:05 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Recent btf encoder's changes brakes BTF data for some gcc versions. > > The problem is that some functions can appear in dwarf data in some > instances without arguments, while they are defined with some. > > Current code will record 'no arguments' for such functions and they > disregard the rest of the DWARF data claiming otherwise. > > This patch changes the BTF function generation, so that in the main > cu__encode_btf processing we do not generate any BTF function code, > but only collect functions 'to generate' and update their arguments. > > When we process the whole data, we go through the functions and > generate its BTD data. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > btf_encoder.c | 110 +++++++++++++++++++++++++++++++++----------------- > pahole.c | 2 +- > 2 files changed, 73 insertions(+), 39 deletions(-) > > diff --git a/btf_encoder.c b/btf_encoder.c > index efc4f48dbc5a..46cb7e6f5abe 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -35,7 +35,10 @@ struct funcs_layout { > struct elf_function { > const char *name; > unsigned long addr; > - bool generated; > + struct cu *cu; > + struct function *fn; > + int args_cnt; > + uint32_t type_id_off; > }; > > static struct elf_function *functions; > @@ -64,6 +67,7 @@ static void delete_functions(void) > static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > { > struct elf_function *new; > + char *name; > > if (elf_sym__type(sym) != STT_FUNC) > return 0; > @@ -83,9 +87,20 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > functions = new; > } > > - functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab); > + /* > + * At the time we process functions, > + * elf object might be already released. > + */ > + name = strdup(elf_sym__name(sym, btfe->symtab)); > + if (!name) > + return -1; > + > + functions[functions_cnt].name = name; > functions[functions_cnt].addr = elf_sym__value(sym); > - functions[functions_cnt].generated = false; > + functions[functions_cnt].fn = NULL; > + functions[functions_cnt].cu = NULL; > + functions[functions_cnt].args_cnt = 0; > + functions[functions_cnt].type_id_off = 0; > functions_cnt++; > return 0; > } > @@ -164,20 +179,6 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl) > return 0; > } > > -static bool should_generate_function(const struct btf_elf *btfe, const char *name) > -{ > - struct elf_function *p; > - struct elf_function key = { .name = name }; > - > - p = bsearch(&key, functions, functions_cnt, > - sizeof(functions[0]), functions_cmp); > - if (!p || p->generated) > - return false; > - > - p->generated = true; > - return true; > -} > - > static bool btf_name_char_ok(char c, bool first) > { > if (c == '_' || c == '.') > @@ -368,6 +369,21 @@ static int generate_func(struct btf_elf *btfe, struct cu *cu, > return err; > } > > +static int process_functions(struct btf_elf *btfe) > +{ > + unsigned long i; > + > + for (i = 0; i < functions_cnt; i++) { > + struct elf_function *func = &functions[i]; > + > + if (!func->fn) > + continue; > + if (generate_func(btfe, func->cu, func->fn, func->type_id_off)) > + return -1; > + } > + return 0; > +} > + > int btf_encoder__encode() > { > int err; > @@ -375,7 +391,9 @@ int btf_encoder__encode() > if (gobuffer__size(&btfe->percpu_secinfo) != 0) > btf_elf__add_datasec_type(btfe, PERCPU_SECTION, &btfe->percpu_secinfo); > > - err = btf_elf__encode(btfe, 0); > + err = process_functions(btfe); > + if (!err) > + err = btf_elf__encode(btfe, 0); > delete_functions(); > btf_elf__delete(btfe); > btfe = NULL; > @@ -539,15 +557,17 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) > return 0; > } > > -static bool has_arg_names(struct cu *cu, struct ftype *ftype) > +static bool has_arg_names(struct cu *cu, struct ftype *ftype, int *args_cnt) > { > struct parameter *param; > const char *name; > > + *args_cnt = 0; > ftype__for_each_parameter(ftype, param) { > name = dwarves__active_loader->strings__ptr(cu, param->name); > if (name == NULL) > return false; > + ++*args_cnt; > } > return true; > } > @@ -624,32 +644,46 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, > has_index_type = true; > } > > - cu__for_each_function(cu, core_id, fn) { > - /* > - * The functions_cnt != 0 means we parsed all necessary > - * kernel symbols and we are using ftrace location filter > - * for functions. If it's not available keep the current > - * dwarf declaration check. > - */ > - if (functions_cnt) { > + /* > + * The functions_cnt != 0 means we parsed all necessary > + * kernel symbols and we are using ftrace location filter > + * for functions. If it's not available keep the current > + * dwarf declaration check. > + */ > + if (functions_cnt) { > + cu__for_each_function(cu, core_id, fn) { > + struct elf_function *p; > + struct elf_function key = { .name = function__name(fn, cu) }; > + int args_cnt = 0; > + > /* > - * We check following conditions: > - * - argument names are defined > - * - there's symbol and address defined for the function > - * - function address belongs to ftrace locations > - * - function is generated only once > + * Collect functions that match ftrace filter > + * and pick the one with proper argument names. > + * The BTF generation happens at the end in > + * btf_encoder__encode function. > */ > - if (!has_arg_names(cu, &fn->proto)) > + p = bsearch(&key, functions, functions_cnt, > + sizeof(functions[0]), functions_cmp); > + if (!p) > continue; > - if (!should_generate_function(btfe, function__name(fn, cu))) > + > + if (!has_arg_names(cu, &fn->proto, &args_cnt)) So I can't unfortunately reproduce that GCC bug with DWARF info. What was exactly the symptom? Maybe you can also share readelf -wi dump for your problematic vmlinux? The reason I'm asking is because I wonder if we should still ignore functions if fn->declaration is set. E.g., for the issue we investigated yesterday, the function with no arguments has declaration set to 1, so just ignoring it would solve the problem. I'm wondering if it's enough to do just that instead of doing this whole delayed function collection/processing. Also, I'd imagine the only expected cases where we can override the function (args_cnt > p->args_cnt) would be if p->args_cnt == 0, no? All other cases are either newly discovered "bogusness" of DWARF (and would be good to know about this) or it's a name collision for functions. Basically, before we go all the way to rework this again, let's see if just skipping declarations would be enough? > continue; > - } else { > + > + if (!p->fn || args_cnt > p->args_cnt) { > + p->fn = fn; > + p->cu = cu; > + p->args_cnt = args_cnt; > + p->type_id_off = type_id_off; > + } > + } > + } else { > + cu__for_each_function(cu, core_id, fn) { > if (fn->declaration || !fn->external) > continue; > + if (generate_func(btfe, cu, fn, type_id_off)) > + goto out; > } I'm trending towards disliking this completely different fallback mechanism. It saved bpf-next accidentally, but otherwise obscured the issue and generally makes testing pahole with artificial binary BTFs (from test programs) harder. How about we unify approaches, but just use mcount symbols opportunistically, as an additional filter, if it's available? With that, testing that we still handle functions with duplicate names properly would be trivial (which I suspect we don't and we'll just keep the one with more args now, right?) And it makes static functions available for non-vmlinux binaries automatically (might be good or bad, but still...). > - > - if (generate_func(btfe, cu, fn, type_id_off)) > - goto out; > } > > if (skip_encoding_vars) > diff --git a/pahole.c b/pahole.c > index fca27148e0bb..d6165d4164dd 100644 > --- a/pahole.c > +++ b/pahole.c > @@ -2392,7 +2392,7 @@ static enum load_steal_kind pahole_stealer(struct cu *cu, > fprintf(stderr, "Encountered error while encoding BTF.\n"); > exit(1); > } > - return LSK__DELETE; > + return LSK__KEEPIT; > } > > if (ctf_encode) { > -- > 2.26.2 >
On Thu, Nov 12, 2020 at 11:54:41AM -0800, Andrii Nakryiko wrote: SNIP > > @@ -624,32 +644,46 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, > > has_index_type = true; > > } > > > > - cu__for_each_function(cu, core_id, fn) { > > - /* > > - * The functions_cnt != 0 means we parsed all necessary > > - * kernel symbols and we are using ftrace location filter > > - * for functions. If it's not available keep the current > > - * dwarf declaration check. > > - */ > > - if (functions_cnt) { > > + /* > > + * The functions_cnt != 0 means we parsed all necessary > > + * kernel symbols and we are using ftrace location filter > > + * for functions. If it's not available keep the current > > + * dwarf declaration check. > > + */ > > + if (functions_cnt) { > > + cu__for_each_function(cu, core_id, fn) { > > + struct elf_function *p; > > + struct elf_function key = { .name = function__name(fn, cu) }; > > + int args_cnt = 0; > > + > > /* > > - * We check following conditions: > > - * - argument names are defined > > - * - there's symbol and address defined for the function > > - * - function address belongs to ftrace locations > > - * - function is generated only once > > + * Collect functions that match ftrace filter > > + * and pick the one with proper argument names. > > + * The BTF generation happens at the end in > > + * btf_encoder__encode function. > > */ > > - if (!has_arg_names(cu, &fn->proto)) > > + p = bsearch(&key, functions, functions_cnt, > > + sizeof(functions[0]), functions_cmp); > > + if (!p) > > continue; > > - if (!should_generate_function(btfe, function__name(fn, cu))) > > + > > + if (!has_arg_names(cu, &fn->proto, &args_cnt)) > > So I can't unfortunately reproduce that GCC bug with DWARF info. What > was exactly the symptom? Maybe you can also share readelf -wi dump for > your problematic vmlinux? hum, can't see -wi working for readelf, however I placed my vmlinux in here: http://people.redhat.com/~jolsa/vmlinux.gz the symptom is that resolve_btfids will fail kernel build: BTFIDS vmlinux FAILED unresolved symbol vfs_getattr because BTF data contains multiple FUNC records for same function and the problem is that declaration tag itself is missing: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060 so pahole won't skip them the first workaround was to workaround that and go for function records that have code address attached, but that's buggy as well: https://bugzilla.redhat.com/show_bug.cgi?id=1890107 then after some discussions we ended up using ftrace addresses as filter for what we want to display.. plus we got static functions as well however for this way we failed to get proper arguments ;-) > > The reason I'm asking is because I wonder if we should still ignore > functions if fn->declaration is set. E.g., for the issue we > investigated yesterday, the function with no arguments has declaration > set to 1, so just ignoring it would solve the problem. I'm wondering > if it's enough to do just that instead of doing this whole delayed > function collection/processing. > > Also, I'd imagine the only expected cases where we can override the > function (args_cnt > p->args_cnt) would be if p->args_cnt == 0, no? I don't know, because originally I'd expect that we would not see function record with zero args when it actualy has some > All other cases are either newly discovered "bogusness" of DWARF (and > would be good to know about this) or it's a name collision for > functions. Basically, before we go all the way to rework this again, > let's see if just skipping declarations would be enough? so there's actualy new developement today in: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060#c14 gcc might actualy get fixed, so I think we could go back to using declaration tag and use ftrace as additional filter.. at least this exercise gave us static functions ;-) however we still have fedora with disabled disabled CONFIG_DEBUG_INFO_BTF and will need to wait for that fix to enable that back > > > continue; > > - } else { > > + > > + if (!p->fn || args_cnt > p->args_cnt) { > > + p->fn = fn; > > + p->cu = cu; > > + p->args_cnt = args_cnt; > > + p->type_id_off = type_id_off; > > + } > > + } > > + } else { > > + cu__for_each_function(cu, core_id, fn) { > > if (fn->declaration || !fn->external) > > continue; > > + if (generate_func(btfe, cu, fn, type_id_off)) > > + goto out; > > } > > I'm trending towards disliking this completely different fallback > mechanism. It saved bpf-next accidentally, but otherwise obscured the > issue and generally makes testing pahole with artificial binary BTFs > (from test programs) harder. How about we unify approaches, but just > use mcount symbols opportunistically, as an additional filter, if it's > available? ok > > With that, testing that we still handle functions with duplicate names > properly would be trivial (which I suspect we don't and we'll just > keep the one with more args now, right?) And it makes static functions > available for non-vmlinux binaries automatically (might be good or > bad, but still...). if we keep just the ftrace filter and rely on declaration tag, the args checking will go away right? jirka
On Thu, Nov 12, 2020 at 1:14 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Nov 12, 2020 at 11:54:41AM -0800, Andrii Nakryiko wrote: > > SNIP > > > > @@ -624,32 +644,46 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, > > > has_index_type = true; > > > } > > > > > > - cu__for_each_function(cu, core_id, fn) { > > > - /* > > > - * The functions_cnt != 0 means we parsed all necessary > > > - * kernel symbols and we are using ftrace location filter > > > - * for functions. If it's not available keep the current > > > - * dwarf declaration check. > > > - */ > > > - if (functions_cnt) { > > > + /* > > > + * The functions_cnt != 0 means we parsed all necessary > > > + * kernel symbols and we are using ftrace location filter > > > + * for functions. If it's not available keep the current > > > + * dwarf declaration check. > > > + */ > > > + if (functions_cnt) { > > > + cu__for_each_function(cu, core_id, fn) { > > > + struct elf_function *p; > > > + struct elf_function key = { .name = function__name(fn, cu) }; > > > + int args_cnt = 0; > > > + > > > /* > > > - * We check following conditions: > > > - * - argument names are defined > > > - * - there's symbol and address defined for the function > > > - * - function address belongs to ftrace locations > > > - * - function is generated only once > > > + * Collect functions that match ftrace filter > > > + * and pick the one with proper argument names. > > > + * The BTF generation happens at the end in > > > + * btf_encoder__encode function. > > > */ > > > - if (!has_arg_names(cu, &fn->proto)) > > > + p = bsearch(&key, functions, functions_cnt, > > > + sizeof(functions[0]), functions_cmp); > > > + if (!p) > > > continue; > > > - if (!should_generate_function(btfe, function__name(fn, cu))) > > > + > > > + if (!has_arg_names(cu, &fn->proto, &args_cnt)) > > > > So I can't unfortunately reproduce that GCC bug with DWARF info. What > > was exactly the symptom? Maybe you can also share readelf -wi dump for > > your problematic vmlinux? > > hum, can't see -wi working for readelf, however I placed my vmlinux > in here: > http://people.redhat.com/~jolsa/vmlinux.gz > > the symptom is that resolve_btfids will fail kernel build: > > BTFIDS vmlinux > FAILED unresolved symbol vfs_getattr > > because BTF data contains multiple FUNC records for same function > > and the problem is that declaration tag itself is missing: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060 > so pahole won't skip them > > the first workaround was to workaround that and go for function > records that have code address attached, but that's buggy as well: > https://bugzilla.redhat.com/show_bug.cgi?id=1890107 > > then after some discussions we ended up using ftrace addresses > as filter for what we want to display.. plus we got static functions > as well > > however for this way we failed to get proper arguments ;-) Right, I followed along overall, but forgot the details of the initial problem. Thanks for the refresher. See below for my current thoughts on dealing with all this. > > > > > The reason I'm asking is because I wonder if we should still ignore > > functions if fn->declaration is set. E.g., for the issue we > > investigated yesterday, the function with no arguments has declaration > > set to 1, so just ignoring it would solve the problem. I'm wondering > > if it's enough to do just that instead of doing this whole delayed > > function collection/processing. > > > > Also, I'd imagine the only expected cases where we can override the > > function (args_cnt > p->args_cnt) would be if p->args_cnt == 0, no? > > I don't know, because originally I'd expect that we would not see > function record with zero args when it actualy has some > > > All other cases are either newly discovered "bogusness" of DWARF (and > > would be good to know about this) or it's a name collision for > > functions. Basically, before we go all the way to rework this again, > > let's see if just skipping declarations would be enough? > > so there's actualy new developement today in: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060#c14 > > gcc might actualy get fixed, so I think we could go back to > using declaration tag and use ftrace as additional filter.. > at least this exercise gave us static functions ;-) > > however we still have fedora with disabled disabled CONFIG_DEBUG_INFO_BTF > and will need to wait for that fix to enable that back Right, we better have a more robust approach not relying on not-yet-released GCC. > > > > > > continue; > > > - } else { > > > + > > > + if (!p->fn || args_cnt > p->args_cnt) { > > > + p->fn = fn; > > > + p->cu = cu; > > > + p->args_cnt = args_cnt; > > > + p->type_id_off = type_id_off; > > > + } > > > + } > > > + } else { > > > + cu__for_each_function(cu, core_id, fn) { > > > if (fn->declaration || !fn->external) > > > continue; > > > + if (generate_func(btfe, cu, fn, type_id_off)) > > > + goto out; > > > } > > > > I'm trending towards disliking this completely different fallback > > mechanism. It saved bpf-next accidentally, but otherwise obscured the > > issue and generally makes testing pahole with artificial binary BTFs > > (from test programs) harder. How about we unify approaches, but just > > use mcount symbols opportunistically, as an additional filter, if it's > > available? > > ok > > > > > With that, testing that we still handle functions with duplicate names > > properly would be trivial (which I suspect we don't and we'll just > > keep the one with more args now, right?) And it makes static functions > > available for non-vmlinux binaries automatically (might be good or > > bad, but still...). > > if we keep just the ftrace filter and rely on declaration tag, > the args checking will go away right? So I looked at your vmlinux image. I think we should just keep everything mostly as it it right now (without changes in this patch), but add just two simple checks: 1. Skip if fn->declaration (ignore correctly marked func declarations) 2. Skip if DW_AT_inline: 1 (ignore inlined functions). I'd keep the named arguments check as is, I think it's helpful. 1) will skip stuff that's explicitly marked as declaration. 2) inline check will partially mitigate dropping of fn->external check (and we can't really attach to inlined functions). So will the named args check as well. Then we should do mcount checks, **if** mcount symbols are present (which should always be the case for any reasonable vmlinux image that's supposed to be used with BPF, I think). So together, it should cover all known issues, I think. And then we'll just have to watch out for any new ones. GCC bugfix is good, but it's too late: the harm is done and there are compilers out there that we should deal with. > > jirka >
On Thu, Nov 12, 2020 at 4:08 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > So I looked at your vmlinux image. I think we should just keep > everything mostly as it it right now (without changes in this patch), > but add just two simple checks: > > 1. Skip if fn->declaration (ignore correctly marked func declarations) > 2. Skip if DW_AT_inline: 1 (ignore inlined functions). > > I'd keep the named arguments check as is, I think it's helpful. 1) > will skip stuff that's explicitly marked as declaration. 2) inline > check will partially mitigate dropping of fn->external check (and we > can't really attach to inlined functions). I thought DW_AT_inline is an indication that the function was marked "inline" in C code. That doesn't mean that the function was actually inlined. So I don't think pahole should check that bit.
On Thu, Nov 12, 2020 at 4:19 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Nov 12, 2020 at 4:08 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > So I looked at your vmlinux image. I think we should just keep > > everything mostly as it it right now (without changes in this patch), > > but add just two simple checks: > > > > 1. Skip if fn->declaration (ignore correctly marked func declarations) > > 2. Skip if DW_AT_inline: 1 (ignore inlined functions). > > > > I'd keep the named arguments check as is, I think it's helpful. 1) > > will skip stuff that's explicitly marked as declaration. 2) inline > > check will partially mitigate dropping of fn->external check (and we > > can't really attach to inlined functions). > > I thought DW_AT_inline is an indication that the function was marked "inline" > in C code. That doesn't mean that the function was actually inlined. > So I don't think pahole should check that bit. According to DWARF spec, there are 4 possible values: DW_INL_not_inlined = 0 Not declared inline nor inlined by the compiler DW_INL_inlined = 1 Not declared inline but inlined by the compiler DW_INL_declared_not_inlined = 2 Declared inline but not inlined by the compiler DW_INL_declared_inlined = 3 Declared inline and inlined by the compiler So DW_INL_inlined is supposed to be added to functions that are not marked inline, but were nevertheless inlined. I saw this for one of vfs_getattr entries in DWARF, which clearly is not marked inline. But also that DWARF entry had proper args with names, so it would work fine as well. I don't know, with DWARF it's always some guessing game. Let's leave DW_AT_inline alone for now. Important part is skipping declarations (when they are marked as such), though I'm not claiming it will solve the problem completely... :)
On 11/12/20 4:30 PM, Andrii Nakryiko wrote: > On Thu, Nov 12, 2020 at 4:19 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Thu, Nov 12, 2020 at 4:08 PM Andrii Nakryiko >> <andrii.nakryiko@gmail.com> wrote: >>> >>> So I looked at your vmlinux image. I think we should just keep >>> everything mostly as it it right now (without changes in this patch), >>> but add just two simple checks: >>> >>> 1. Skip if fn->declaration (ignore correctly marked func declarations) >>> 2. Skip if DW_AT_inline: 1 (ignore inlined functions). >>> >>> I'd keep the named arguments check as is, I think it's helpful. 1) >>> will skip stuff that's explicitly marked as declaration. 2) inline >>> check will partially mitigate dropping of fn->external check (and we >>> can't really attach to inlined functions). >> >> I thought DW_AT_inline is an indication that the function was marked "inline" >> in C code. That doesn't mean that the function was actually inlined. >> So I don't think pahole should check that bit. > > According to DWARF spec, there are 4 possible values: > > DW_INL_not_inlined = 0 Not declared inline nor inlined by > the compiler > DW_INL_inlined = 1 Not declared inline but inlined by > the compiler > DW_INL_declared_not_inlined = 2 Declared inline but not inlined by > the compiler > DW_INL_declared_inlined = 3 Declared inline and inlined by the compiler > > So DW_INL_inlined is supposed to be added to functions that are not > marked inline, but were nevertheless inlined. I saw this for one of > vfs_getattr entries in DWARF, which clearly is not marked inline. I looked at llvm source code, llvm only tries to assign DW_INL_inlined and also only at certain conditions. Not sure about gcc. Probably similar. So this field is not reliable, esp. without it does not mean it is not inlined. > > But also that DWARF entry had proper args with names, so it would work > fine as well. I don't know, with DWARF it's always some guessing game. > Let's leave DW_AT_inline alone for now. > > Important part is skipping declarations (when they are marked as > such), though I'm not claiming it will solve the problem completely... > :) >
On Thu, Nov 12, 2020 at 5:01 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 11/12/20 4:30 PM, Andrii Nakryiko wrote: > > On Thu, Nov 12, 2020 at 4:19 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > >> > >> On Thu, Nov 12, 2020 at 4:08 PM Andrii Nakryiko > >> <andrii.nakryiko@gmail.com> wrote: > >>> > >>> So I looked at your vmlinux image. I think we should just keep > >>> everything mostly as it it right now (without changes in this patch), > >>> but add just two simple checks: > >>> > >>> 1. Skip if fn->declaration (ignore correctly marked func declarations) > >>> 2. Skip if DW_AT_inline: 1 (ignore inlined functions). > >>> > >>> I'd keep the named arguments check as is, I think it's helpful. 1) > >>> will skip stuff that's explicitly marked as declaration. 2) inline > >>> check will partially mitigate dropping of fn->external check (and we > >>> can't really attach to inlined functions). > >> > >> I thought DW_AT_inline is an indication that the function was marked "inline" > >> in C code. That doesn't mean that the function was actually inlined. > >> So I don't think pahole should check that bit. > > > > According to DWARF spec, there are 4 possible values: > > > > DW_INL_not_inlined = 0 Not declared inline nor inlined by > > the compiler > > DW_INL_inlined = 1 Not declared inline but inlined by > > the compiler > > DW_INL_declared_not_inlined = 2 Declared inline but not inlined by > > the compiler > > DW_INL_declared_inlined = 3 Declared inline and inlined by the compiler > > > > So DW_INL_inlined is supposed to be added to functions that are not > > marked inline, but were nevertheless inlined. I saw this for one of > > vfs_getattr entries in DWARF, which clearly is not marked inline. > > I looked at llvm source code, llvm only tries to assign DW_INL_inlined > and also only at certain conditions. Not sure about gcc. Probably > similar. So this field is not reliable, esp. without it does not mean it > is not inlined. Can't say I'm surprised... > > > > > But also that DWARF entry had proper args with names, so it would work > > fine as well. I don't know, with DWARF it's always some guessing game. > > Let's leave DW_AT_inline alone for now. > > > > Important part is skipping declarations (when they are marked as > > such), though I'm not claiming it will solve the problem completely... > > :) > >
On Thu, Nov 12, 2020 at 04:08:02PM -0800, Andrii Nakryiko wrote: > On Thu, Nov 12, 2020 at 1:14 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Thu, Nov 12, 2020 at 11:54:41AM -0800, Andrii Nakryiko wrote: > > > > SNIP > > > > > > @@ -624,32 +644,46 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, > > > > has_index_type = true; > > > > } > > > > > > > > - cu__for_each_function(cu, core_id, fn) { > > > > - /* > > > > - * The functions_cnt != 0 means we parsed all necessary > > > > - * kernel symbols and we are using ftrace location filter > > > > - * for functions. If it's not available keep the current > > > > - * dwarf declaration check. > > > > - */ > > > > - if (functions_cnt) { > > > > + /* > > > > + * The functions_cnt != 0 means we parsed all necessary > > > > + * kernel symbols and we are using ftrace location filter > > > > + * for functions. If it's not available keep the current > > > > + * dwarf declaration check. > > > > + */ > > > > + if (functions_cnt) { > > > > + cu__for_each_function(cu, core_id, fn) { > > > > + struct elf_function *p; > > > > + struct elf_function key = { .name = function__name(fn, cu) }; > > > > + int args_cnt = 0; > > > > + > > > > /* > > > > - * We check following conditions: > > > > - * - argument names are defined > > > > - * - there's symbol and address defined for the function > > > > - * - function address belongs to ftrace locations > > > > - * - function is generated only once > > > > + * Collect functions that match ftrace filter > > > > + * and pick the one with proper argument names. > > > > + * The BTF generation happens at the end in > > > > + * btf_encoder__encode function. > > > > */ > > > > - if (!has_arg_names(cu, &fn->proto)) > > > > + p = bsearch(&key, functions, functions_cnt, > > > > + sizeof(functions[0]), functions_cmp); > > > > + if (!p) > > > > continue; > > > > - if (!should_generate_function(btfe, function__name(fn, cu))) > > > > + > > > > + if (!has_arg_names(cu, &fn->proto, &args_cnt)) > > > > > > So I can't unfortunately reproduce that GCC bug with DWARF info. What > > > was exactly the symptom? Maybe you can also share readelf -wi dump for > > > your problematic vmlinux? > > > > hum, can't see -wi working for readelf, however I placed my vmlinux > > in here: > > http://people.redhat.com/~jolsa/vmlinux.gz > > > > the symptom is that resolve_btfids will fail kernel build: > > > > BTFIDS vmlinux > > FAILED unresolved symbol vfs_getattr > > > > because BTF data contains multiple FUNC records for same function > > > > and the problem is that declaration tag itself is missing: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060 > > so pahole won't skip them > > > > the first workaround was to workaround that and go for function > > records that have code address attached, but that's buggy as well: > > https://bugzilla.redhat.com/show_bug.cgi?id=1890107 > > > > then after some discussions we ended up using ftrace addresses > > as filter for what we want to display.. plus we got static functions > > as well > > > > however for this way we failed to get proper arguments ;-) > > Right, I followed along overall, but forgot the details of the initial > problem. Thanks for the refresher. See below for my current thoughts > on dealing with all this. > > > > > > > > > The reason I'm asking is because I wonder if we should still ignore > > > functions if fn->declaration is set. E.g., for the issue we > > > investigated yesterday, the function with no arguments has declaration > > > set to 1, so just ignoring it would solve the problem. I'm wondering > > > if it's enough to do just that instead of doing this whole delayed > > > function collection/processing. > > > > > > Also, I'd imagine the only expected cases where we can override the > > > function (args_cnt > p->args_cnt) would be if p->args_cnt == 0, no? > > > > I don't know, because originally I'd expect that we would not see > > function record with zero args when it actualy has some > > > > > All other cases are either newly discovered "bogusness" of DWARF (and > > > would be good to know about this) or it's a name collision for > > > functions. Basically, before we go all the way to rework this again, > > > let's see if just skipping declarations would be enough? > > > > so there's actualy new developement today in: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060#c14 > > > > gcc might actualy get fixed, so I think we could go back to > > using declaration tag and use ftrace as additional filter.. > > at least this exercise gave us static functions ;-) > > > > however we still have fedora with disabled disabled CONFIG_DEBUG_INFO_BTF > > and will need to wait for that fix to enable that back > > Right, we better have a more robust approach not relying on > not-yet-released GCC. > > > > > > > > > > continue; > > > > - } else { > > > > + > > > > + if (!p->fn || args_cnt > p->args_cnt) { > > > > + p->fn = fn; > > > > + p->cu = cu; > > > > + p->args_cnt = args_cnt; > > > > + p->type_id_off = type_id_off; > > > > + } > > > > + } > > > > + } else { > > > > + cu__for_each_function(cu, core_id, fn) { > > > > if (fn->declaration || !fn->external) > > > > continue; > > > > + if (generate_func(btfe, cu, fn, type_id_off)) > > > > + goto out; > > > > } > > > > > > I'm trending towards disliking this completely different fallback > > > mechanism. It saved bpf-next accidentally, but otherwise obscured the > > > issue and generally makes testing pahole with artificial binary BTFs > > > (from test programs) harder. How about we unify approaches, but just > > > use mcount symbols opportunistically, as an additional filter, if it's > > > available? > > > > ok > > > > > > > > With that, testing that we still handle functions with duplicate names > > > properly would be trivial (which I suspect we don't and we'll just > > > keep the one with more args now, right?) And it makes static functions > > > available for non-vmlinux binaries automatically (might be good or > > > bad, but still...). > > > > if we keep just the ftrace filter and rely on declaration tag, > > the args checking will go away right? > > So I looked at your vmlinux image. I think we should just keep > everything mostly as it it right now (without changes in this patch), > but add just two simple checks: > > 1. Skip if fn->declaration (ignore correctly marked func declarations) > 2. Skip if DW_AT_inline: 1 (ignore inlined functions). > > I'd keep the named arguments check as is, I think it's helpful. 1) > will skip stuff that's explicitly marked as declaration. 2) inline ok, that should fix the fail on gccs that still generate declaration tag, so you should be fine and our version of gcc should continue work as well > check will partially mitigate dropping of fn->external check (and we > can't really attach to inlined functions). So will the named args > check as well. Then we should do mcount checks, **if** mcount symbols > are present (which should always be the case for any reasonable > vmlinux image that's supposed to be used with BPF, I think). > > So together, it should cover all known issues, I think. And then we'll > just have to watch out for any new ones. GCC bugfix is good, but it's > too late: the harm is done and there are compilers out there that we > should deal with. hopefuly with this change we can enable BTF back before the gcc fix thanks, jirka
Em Fri, Nov 13, 2020 at 11:59:23AM +0100, Jiri Olsa escreveu: > On Thu, Nov 12, 2020 at 04:08:02PM -0800, Andrii Nakryiko wrote: > > On Thu, Nov 12, 2020 at 1:14 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > On Thu, Nov 12, 2020 at 11:54:41AM -0800, Andrii Nakryiko wrote: > > > > So I can't unfortunately reproduce that GCC bug with DWARF info. What > > > > was exactly the symptom? Maybe you can also share readelf -wi dump for > > > > your problematic vmlinux? > > > hum, can't see -wi working for readelf, however I placed my vmlinux > > > in here: > > > http://people.redhat.com/~jolsa/vmlinux.gz > > > the symptom is that resolve_btfids will fail kernel build: > > > BTFIDS vmlinux > > > FAILED unresolved symbol vfs_getattr > > > because BTF data contains multiple FUNC records for same function > > > and the problem is that declaration tag itself is missing: > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060 > > > so pahole won't skip them > > > the first workaround was to workaround that and go for function > > > records that have code address attached, but that's buggy as well: > > > https://bugzilla.redhat.com/show_bug.cgi?id=1890107 > > > then after some discussions we ended up using ftrace addresses > > > as filter for what we want to display.. plus we got static functions > > > as well > > > however for this way we failed to get proper arguments ;-) > > Right, I followed along overall, but forgot the details of the initial > > problem. Thanks for the refresher. See below for my current thoughts > > on dealing with all this. I'll add this to the set of regression tests I use with pahole: [acme@five vfs_gettattr.multiple.btf.entries.jolsa]$ bpftool btf dump file vmlinux | grep -w FUNC | cut -d\' -f2 | sort | uniq -c | sort -nr | head 3 __x64_sys_userfaultfd 3 __x64_sys_timerfd_settime 3 __x64_sys_timerfd_gettime 3 __x64_sys_timerfd_create 3 __x64_sys_syslog 3 __x64_sys_sysfs 3 __x64_sys_swapon 3 __x64_sys_swapoff 3 __x64_sys_socketpair 3 __x64_sys_socket [acme@five vfs_gettattr.multiple.btf.entries.jolsa]$ pfunct -F btf vmlinux | sort | uniq -c | sort -nr | head 3 __x64_sys_userfaultfd 3 __x64_sys_timerfd_settime 3 __x64_sys_timerfd_gettime 3 __x64_sys_timerfd_create 3 __x64_sys_syslog 3 __x64_sys_sysfs 3 __x64_sys_swapon 3 __x64_sys_swapoff 3 __x64_sys_socketpair 3 __x64_sys_socket [acme@five vfs_gettattr.multiple.btf.entries.jolsa]$ I.e. the output of those tools need to match and all functions need to appear only once. I'll also use pfunct with -F dwarf to get the same results, probably will add these to the 'btfdiff' tool that is in the pahole git repo. - Arnaldo
diff --git a/btf_encoder.c b/btf_encoder.c index efc4f48dbc5a..46cb7e6f5abe 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -35,7 +35,10 @@ struct funcs_layout { struct elf_function { const char *name; unsigned long addr; - bool generated; + struct cu *cu; + struct function *fn; + int args_cnt; + uint32_t type_id_off; }; static struct elf_function *functions; @@ -64,6 +67,7 @@ static void delete_functions(void) static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) { struct elf_function *new; + char *name; if (elf_sym__type(sym) != STT_FUNC) return 0; @@ -83,9 +87,20 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) functions = new; } - functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab); + /* + * At the time we process functions, + * elf object might be already released. + */ + name = strdup(elf_sym__name(sym, btfe->symtab)); + if (!name) + return -1; + + functions[functions_cnt].name = name; functions[functions_cnt].addr = elf_sym__value(sym); - functions[functions_cnt].generated = false; + functions[functions_cnt].fn = NULL; + functions[functions_cnt].cu = NULL; + functions[functions_cnt].args_cnt = 0; + functions[functions_cnt].type_id_off = 0; functions_cnt++; return 0; } @@ -164,20 +179,6 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl) return 0; } -static bool should_generate_function(const struct btf_elf *btfe, const char *name) -{ - struct elf_function *p; - struct elf_function key = { .name = name }; - - p = bsearch(&key, functions, functions_cnt, - sizeof(functions[0]), functions_cmp); - if (!p || p->generated) - return false; - - p->generated = true; - return true; -} - static bool btf_name_char_ok(char c, bool first) { if (c == '_' || c == '.') @@ -368,6 +369,21 @@ static int generate_func(struct btf_elf *btfe, struct cu *cu, return err; } +static int process_functions(struct btf_elf *btfe) +{ + unsigned long i; + + for (i = 0; i < functions_cnt; i++) { + struct elf_function *func = &functions[i]; + + if (!func->fn) + continue; + if (generate_func(btfe, func->cu, func->fn, func->type_id_off)) + return -1; + } + return 0; +} + int btf_encoder__encode() { int err; @@ -375,7 +391,9 @@ int btf_encoder__encode() if (gobuffer__size(&btfe->percpu_secinfo) != 0) btf_elf__add_datasec_type(btfe, PERCPU_SECTION, &btfe->percpu_secinfo); - err = btf_elf__encode(btfe, 0); + err = process_functions(btfe); + if (!err) + err = btf_elf__encode(btfe, 0); delete_functions(); btf_elf__delete(btfe); btfe = NULL; @@ -539,15 +557,17 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) return 0; } -static bool has_arg_names(struct cu *cu, struct ftype *ftype) +static bool has_arg_names(struct cu *cu, struct ftype *ftype, int *args_cnt) { struct parameter *param; const char *name; + *args_cnt = 0; ftype__for_each_parameter(ftype, param) { name = dwarves__active_loader->strings__ptr(cu, param->name); if (name == NULL) return false; + ++*args_cnt; } return true; } @@ -624,32 +644,46 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, has_index_type = true; } - cu__for_each_function(cu, core_id, fn) { - /* - * The functions_cnt != 0 means we parsed all necessary - * kernel symbols and we are using ftrace location filter - * for functions. If it's not available keep the current - * dwarf declaration check. - */ - if (functions_cnt) { + /* + * The functions_cnt != 0 means we parsed all necessary + * kernel symbols and we are using ftrace location filter + * for functions. If it's not available keep the current + * dwarf declaration check. + */ + if (functions_cnt) { + cu__for_each_function(cu, core_id, fn) { + struct elf_function *p; + struct elf_function key = { .name = function__name(fn, cu) }; + int args_cnt = 0; + /* - * We check following conditions: - * - argument names are defined - * - there's symbol and address defined for the function - * - function address belongs to ftrace locations - * - function is generated only once + * Collect functions that match ftrace filter + * and pick the one with proper argument names. + * The BTF generation happens at the end in + * btf_encoder__encode function. */ - if (!has_arg_names(cu, &fn->proto)) + p = bsearch(&key, functions, functions_cnt, + sizeof(functions[0]), functions_cmp); + if (!p) continue; - if (!should_generate_function(btfe, function__name(fn, cu))) + + if (!has_arg_names(cu, &fn->proto, &args_cnt)) continue; - } else { + + if (!p->fn || args_cnt > p->args_cnt) { + p->fn = fn; + p->cu = cu; + p->args_cnt = args_cnt; + p->type_id_off = type_id_off; + } + } + } else { + cu__for_each_function(cu, core_id, fn) { if (fn->declaration || !fn->external) continue; + if (generate_func(btfe, cu, fn, type_id_off)) + goto out; } - - if (generate_func(btfe, cu, fn, type_id_off)) - goto out; } if (skip_encoding_vars) diff --git a/pahole.c b/pahole.c index fca27148e0bb..d6165d4164dd 100644 --- a/pahole.c +++ b/pahole.c @@ -2392,7 +2392,7 @@ static enum load_steal_kind pahole_stealer(struct cu *cu, fprintf(stderr, "Encountered error while encoding BTF.\n"); exit(1); } - return LSK__DELETE; + return LSK__KEEPIT; } if (ctf_encode) {
Recent btf encoder's changes brakes BTF data for some gcc versions. The problem is that some functions can appear in dwarf data in some instances without arguments, while they are defined with some. Current code will record 'no arguments' for such functions and they disregard the rest of the DWARF data claiming otherwise. This patch changes the BTF function generation, so that in the main cu__encode_btf processing we do not generate any BTF function code, but only collect functions 'to generate' and update their arguments. When we process the whole data, we go through the functions and generate its BTD data. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- btf_encoder.c | 110 +++++++++++++++++++++++++++++++++----------------- pahole.c | 2 +- 2 files changed, 73 insertions(+), 39 deletions(-)