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 15:02, 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!
>>
>> Arnaldo, will I send a v3 series incorporating the above fix
>> to patch 1?
> 
> I can fix it here. Done, I;ll force push it to the 'next' branch.
> 
> Also I noted the index_idx usage in parameter__new(), it can be -1 when
> processing:
> 
>  <1><2eb2>: Abbrev Number: 18 (DW_TAG_subroutine_type)
>     <2eb3>   DW_AT_prototyped  : 1
>     <2eb3>   DW_AT_sibling     : <0x2ec2>
>  <2><2eb7>: Abbrev Number: 3 (DW_TAG_formal_parameter)
>     <2eb8>   DW_AT_type        : <0x414>
>  <2><2ebc>: Abbrev Number: 3 (DW_TAG_formal_parameter)
>     <2ebd>   DW_AT_type        : <0x69>
>  <2><2ec1>: Abbrev Number: 0
> 
>  And in that case we don't have the location expression:
> 
>   <1><af36>: Abbrev Number: 77 (DW_TAG_subprogram)
>     <af37>   DW_AT_external    : 1
>     <af37>   DW_AT_name        : (indirect string, offset: 0x4ff7): startup_64_setup_env
>     <af3b>   DW_AT_decl_file   : 1
>     <af3b>   DW_AT_decl_line   : 592
>     <af3d>   DW_AT_decl_column : 13
>     <af3e>   DW_AT_prototyped  : 1
>     <af3e>   DW_AT_low_pc      : 0xffffffff81000570
>     <af46>   DW_AT_high_pc     : 0x6d
>     <af4e>   DW_AT_frame_base  : 1 byte block: 9c       (DW_OP_call_frame_cfa)
>     <af50>   DW_AT_call_all_calls: 1
>     <af50>   DW_AT_sibling     : <0xb11f>
>  <2><af54>: Abbrev Number: 67 (DW_TAG_formal_parameter)
>     <af55>   DW_AT_name        : (indirect string, offset: 0x2a50d): physbase
>     <af59>   DW_AT_decl_file   : 1
>     <af59>   DW_AT_decl_line   : 592
>     <af5b>   DW_AT_decl_column : 48
>     <af5c>   DW_AT_type        : <0x4c>
>     <af60>   DW_AT_location    : 0x10 (location list)
>     <af64>   DW_AT_GNU_locviews: 0xc
> 
> I.e. its just a function _type_, not an actual function, so I'm applying
> this on top of that first patch, ok?
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index 7e05fde8a5c3ac26..253c5efaf3b55a93 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1035,7 +1035,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>  		tag__init(&parm->tag, cu, die);
>  		parm->name = attr_string(die, DW_AT_name, conf);
>  
> -		if (param_idx >= cu->nr_register_params)
> +		if (param_idx >= cu->nr_register_params || param_idx < 0)
>  			return parm;
>  		/* Parameters which use DW_AT_abstract_origin to point at
>  		 * the original parameter definition (with no name in the DIE)
> 
>

ah, great catch. thanks again!

Alan
 
> - Arnaldo
>  
>> 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.
>>>
>