Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf

[Yonghong Song <yhs@fb.com>]

On 11/3/20 11:23 AM, Andrii Nakryiko wrote:
> On Tue, Nov 3, 2020 at 11:06 AM Jiri Olsa <jolsa@redhat.com> wrote:
>>
>> On Tue, Nov 03, 2020 at 10:58:58AM -0800, Andrii Nakryiko wrote:
>>> On Mon, Nov 2, 2020 at 2:57 PM Jiri Olsa <jolsa@redhat.com> wrote:
>>>>
>>>> On Mon, Nov 02, 2020 at 10:59:08PM +0100, Jiri Olsa wrote:
>>>>> On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa wrote:
>>>>>> We need to generate just single BTF instance for the
>>>>>> function, while DWARF data contains multiple instances
>>>>>> of DW_TAG_subprogram tag.
>>>>>>
>>>>>> Unfortunately we can no longer rely on DW_AT_declaration
>>>>>> tag (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060)
>>>>>>
>>>>>> Instead we apply following checks:
>>>>>>    - argument names are defined for the function
>>>>>>    - there's symbol and address defined for the function
>>>>>>    - function is generated only once
>>>>>>
>>>>>> Also because we want to follow kernel's ftrace traceable
>>>>>> functions, this patchset is adding extra check that the
>>>>>> function is one of the ftrace's functions.
>>>>>>
>>>>>> All ftrace functions addresses are stored in vmlinux
>>>>>> binary within symbols:
>>>>>>    __start_mcount_loc
>>>>>>    __stop_mcount_loc
>>>>>
>>>>> hum, for some reason this does not pass through bpf internal
>>>>> functions like bpf_iter_bpf_map.. I learned it hard way ;-)
>>>
>>> what's the exact name of the function that was missing?
>>> bpf_iter_bpf_map doesn't exist. And if it's __init function, why does
>>> it matter, it's not going to be even available at runtime, right?
>>>
>>
>> bpf_map iter definition:
>>
>> DEFINE_BPF_ITER_FUNC(bpf_map, struct bpf_iter_meta *meta, struct bpf_map *map)
>>
>> goes to:
>>
>> #define DEFINE_BPF_ITER_FUNC(target, args...)                   \
>>          extern int bpf_iter_ ## target(args);                   \
>>          int __init bpf_iter_ ## target(args) { return 0; }
>>
>> that creates __init bpf_iter_bpf_map function that will make
>> it into BTF where it's expected when opening iterator, but the
>> code will be freed because it's __init function
> 
> hm... should we just drop __init there?
> 
> Yonghong, is __init strictly necessary, or was just an optimization to
> save a tiny bit of space?

It is an optimization to save some space. We only need function
signature, not function body, for bpf_iter.

The macro definition is in include/linux/bpf.h.

#define DEFINE_BPF_ITER_FUNC(target, args...)                   \
         extern int bpf_iter_ ## target(args);                   \
         int __init bpf_iter_ ## target(args) { return 0; }

Maybe you could have a section, e.g., called
   .init.bpf.preserve_type
which you can scan through to preserve the types.

Alternatively you can drop the above __init, the saving is
indeed tiny. But this adds overhead to ksymbol lookup and
may not be desirable.

> 
>>
>> there are few iteratos functions like that, and I was going to
>> check if there's more
>>
>>>
>>>>> will check
>>>>
>>>> so it gets filtered out because it's __init function
>>>> I'll check if the fix below catches all internal functions,
>>>> but I guess we should do something more robust
>>>>
>>>> jirka
>>>>
>>>>
>>>> ---
>>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>>> index 0a378aa92142..3cd94280c35b 100644
>>>> --- a/btf_encoder.c
>>>> +++ b/btf_encoder.c
>>>> @@ -143,7 +143,8 @@ static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms)
>>>>                  /* Do not enable .init section functions. */
>>>>                  if (init_filter &&
>>>>                      func->addr >= ms->init_begin &&
>>>> -                   func->addr <  ms->init_end)
>>>> +                   func->addr <  ms->init_end &&
>>>> +                   strncmp("bpf_", func->name, 4))
>>>
>>> this looks like a very wrong way to do this? Can you please elaborate
>>> on what's missing and why it shouldn't be missing?
>>
>> yes, it's just a hack, we should do something more
>> robust as I mentioned above
>>
>> it just allowed me to use iterators finaly ;-)
> 
> sure, I get it, I was just trying to understand why there is such a
> problem in the first place. Turns out we need FUNCs not just for
> fentry/fexit and similar, but also for bpf_iter, which is an entirely
> different use case (similar to raw_tp, but raw_tp is using typedef ->
> func_proto approach).
> 
> So I don't know, we might as well just not do mcount checks?.. As an
> alternative, but it's not great as well.
> 
>>
>> jirka
>>