Re: [PATCH v2 dwarves 1/5] dwarves: help dwarf loader spot functions with optimized-out parameters

[Alan Maguire <alan.maguire@oracle.com>]

On 01/02/2023 17:01, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
>> On Wed, Feb 1, 2023 at 7:19 AM David Vernet <void@manifault.com> wrote:
>>>
>>> On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
>>>> Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
>>>>> On 01/02/2023 03:02, David Vernet wrote:
>>>>>> On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
>>>>>>> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <void@manifault.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
>>>>>>>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
>>>>>>>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
>>>>>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>>>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>>>>> +++ b/dwarves.h
>>>>>>>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>>>>>>>>>>   uint8_t          has_addr_info:1;
>>>>>>>>>>>>>>>>>   uint8_t          uses_global_strings:1;
>>>>>>>>>>>>>>>>>   uint8_t          little_endian:1;
>>>>>>>>>>>>>>>>> + uint8_t          nr_register_params;
>>>>>>>>>>>>>>>>>   uint16_t         language;
>>>>>>>>>>>>>>>>>   unsigned long    nr_inline_expansions;
>>>>>>>>>>>>>>>>>   size_t           size_inline_expansions;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>>>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>>>>>>>>>>> defined if using a very old elf.h; below works around this
>>>>>>>>>>>>>>> (dwarves otherwise builds fine on this system).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ok, will add it and will test with containers for older distros too.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>>>>>>>>>>> repo at:
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>>>>>>>>>>
>>>>>>>>>>>>> It failed yesterday and today due to problems with the installation of
>>>>>>>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>>>>>>>>>>> notifications floating by.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>>>>>>>>>>> iterator that Jiri noticed.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
>>>>>>>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>>>>>>>>>>> from the BTF representation, and as a result:
>>>>>>>>>>>>
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>>>>>>>>>>
>>>>>>>>>>>> Not sure why I didn't notice this previously.
>>>>>>>>>>>>
>>>>>>>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>>>>>>>>>>
>>>>>>>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>> {
>>>>>>>>>>>>         return -EOPNOTSUPP;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> looks like this:
>>>>>>>>>>>>
>>>>>>>>>>>>    <8af83a2>   DW_AT_external    : 1
>>>>>>>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>>>>>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
>>>>>>>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
>>>>>>>>>>>>     <8af83a9>   DW_AT_decl_column : 5
>>>>>>>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
>>>>>>>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
>>>>>>>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>>>>>>>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>>>>>>>>>>     <8af83b3>   DW_AT_name        : ctx
>>>>>>>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
>>>>>>>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
>>>>>>>>>>>>     <8af83ba>   DW_AT_decl_column : 51
>>>>>>>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
>>>>>>>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>>>>>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>>>>>>>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
>>>>>>>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
>>>>>>>>>>>>     <8af83c7>   DW_AT_decl_column : 61
>>>>>>>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
>>>>>>>>>>>>
>>>>>>>>>>>> ...and because there are no further abstract origin references
>>>>>>>>>>>> with location information either, we classify it as lacking
>>>>>>>>>>>> locations for (some of) the parameters, and as a result
>>>>>>>>>>>> we skip BTF encoding. We can work around that by doing this:
>>>>>>>>>>>>
>>>>>>>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>
>>>>>>>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
>>>>>>>>>>>
>>>>>>>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
>>>>>>>>>>> applied to arguments.
>>>>>>>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
>>>>>>>>>>> the way we do in selftests.
>>>>>>>>>>
>>>>>>>>>> There is also
>>>>>>>>>> # define __visible __attribute__((__externally_visible__))
>>>>>>>>>> that probably fits the best here.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> testing thus for seems to show that for x86_64, David's series
>>>>>>>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
>>>>>>>>> to cover recently-arrived kfuncs like cpumask) is sufficient
>>>>>>>>> to avoid resolve_btfids warnings.
>>>>>>>>
>>>>>>>> Nice. Alexei -- lmk how you want to proceed. I think using the
>>>>>>>> __bpf_kfunc macro in the short term (with __used and noinline) is
>>>>>>>> probably the least controversial way to unblock this, but am open to
>>>>>>>> other suggestions.
>>>>>>>
>>>>>>> Sounds good to me, but sounds like __used and noinline are not
>>>>>>> enough to address the issues on aarch64?
>>>>>>
>>>>>> Indeed, we'll have to make sure that's also addressed. Alan -- did you
>>>>>> try Alexei's suggestion to use __weak? Does that fix the issue for
>>>>>> aarch64? I'm still confused as to why it's only complaining for a small
>>>>>> subset of kfuncs, which include those that have external linkage.
>>>>>>
>>>>>
>>>>> I finally got to the bottom of the aarch64 issues; there was a 1-line bug
>>>>> in the changes I made to the DWARF handling code which leads to BTF generation;
>>>>> it was excluding a bunch of functions incorrectly, marking them as optimized out.
>>>>> The fix is:
>>>>>
>>>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>>>> index dba2d37..8364e17 100644
>>>>> --- a/dwarf_loader.c
>>>>> +++ b/dwarf_loader.c
>>>>> @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>>>                         Dwarf_Op *expr = loc.expr;
>>>>>
>>>>>                         switch (expr->atom) {
>>>>> -                       case DW_OP_reg1 ... DW_OP_reg31:
>>>>> +                       case DW_OP_reg0 ... DW_OP_reg31:
>>>>>                         case DW_OP_breg0 ... DW_OP_breg31:
>>>>>                                 break;
>>>>>                         default:
>>>>>
>>>>> ..and because reg0 is the first parameter for aarch64, we were
>>>>> incorrectly landing in the "default:" of the switch statement
>>>>> and marking a bunch of functions as optimized out
>>>>> because we thought the first argument was. Sorry about this,
>>>>> and thanks for all the suggestions!
>>>
>>> Great, so inline and __used with __bpf_kfunc sounds like the way forward
>>> in the short term. Arnaldo / Alexei -- how do you want to resolve the
>>> dependency here? Going through bpf-next is probably a good idea so that
>>> we get proper CI coverage, and any kfuncs added to bpf-next after this
>>> can use the macro. Does that work for you?
>>
>> It feels fixed pahole should be done under some flag
>> otherwise when people update the pahole the existing and older
>> kernels might stop building with warns:
>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>> ...
>>

Good point, something like

--skip_inconsistent_proto	Skip functions that have multiple inconsistent
				function prototypes sharing the same name, or
				have optimized-out parameters.

? Implementation needs a bit of thought though because we're
not really doing the same thing that we were before. Previously we
were adding the first instance of a function in the CU we came across.
Probably safest to resurrect that behaviour for the legacy
non-skip-inconsistent-proto case I think. The final patch handling
inconsistent function prototypes will need to be reworked a bit to 
support this, since we tossed this approach and used saving/merging 
multiple instances in the tree instead.  Once I've built bpf trees I'll
have a go at getting this working.

>> Arnaldo, could you check what warns do you see with this fixed pahole
>> in bpf tree ?
> 
> Sure.
> 

I can collect this for x86_64/aarch64 too; might take a few hours
before I have the results.

>> If there are only few warns then we can manually add __used noinline
>> to these places, push to bpf tree and push to stable.
>>
>> Then in bpf-next we can clean up everything with __bpf_kfunc.
>