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 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!

Arnaldo, will I send a v3 series incorporating the above fix
to patch 1?

With this fix in place, prefixing the kfunc functions with

__used noinline

...did the trick to ensure kfuncs were not excluded on x86_64
and aarch64.

>>
>>> Yeah, I tend to think we should try to avoid using hidden / visible
>>> attributes given that (to my knowledge) they're really more meant for
>>> controlling whether a symbol is exported from a shared object rather
>>> than controlling what the compiler is doing when it creates the
>>> compilation unit. One could imagine that in an LTO build, the compiler
>>> would still optimize the function regardless of its visibility for that
>>> reason, though it's possible I don't have the full picture.
>>
>> __visible is specifically done to prevent optimization of
>> functions that are externally visible. That should address LTO concerns.
>> We haven't seen LTO messing up anything. Just something to keep in mind.
> 
> Ah, fair enough. I was conflating that with the visibility("...")
> attribute. As you pointed out, __visible is something else entirely, and
> is meant to avoid possible issues with LTO.
> 
> One other option we could consider is enforcing that kfuncs must have
> global linkage and can't be static. If we did that, it seems like
> __visible would be a viable option. Though we'd have to verify that it
> addresses the issue w/ aarch64.
>