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@xxxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
>>>>>>>>
>>>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <alan.maguire@xxxxxxxxxx> 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.
>>>
>
