Re: [PATCH v2 bpf-next 02/12] libbpf: implement BPF CO-RE offset relocation algorithm

Song Liu
2019-07-31T08:30:22Z

> On Jul 30, 2019, at 11:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jul 30, 2019 at 10:19 PM Song Liu <songliubraving@fb.com> wrote:
>>
>>
>>
>>> On Jul 30, 2019, at 6:00 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Tue, Jul 30, 2019 at 5:39 PM Song Liu <songliubraving@fb.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Jul 30, 2019, at 12:53 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>>>>>
>>>>> This patch implements the core logic for BPF CO-RE offsets relocations.
>>>>> Every instruction that needs to be relocated has corresponding
>>>>> bpf_offset_reloc as part of BTF.ext. Relocations are performed by trying
>>>>> to match recorded "local" relocation spec against potentially many
>>>>> compatible "target" types, creating corresponding spec. Details of the
>>>>> algorithm are noted in corresponding comments in the code.
>>>>>
>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

[...]

>>>>
>>>
>>> I just picked the most succinct and non-repetitive form. It's
>>> immediately apparent which type it's implicitly converted to, so I
>>> felt there is no need to repeat it. Also, just (void *) is much
>>> shorter. :)
>>
>> _All_ other code in btf.c converts the pointer to the target type.
>
> Most in libbpf.c doesn't, though. Also, I try to preserve pointer
> constness for uses that don't modify BTF types (pretty much all of
> them in libbpf), so it becomes really verbose, despite extremely short
> variable names:
>
> const struct btf_member *m = (const struct btf_member *)(t + 1);

I don't think being verbose is a big problem here. Overusing
(void *) feels like a bigger problem.

>
> Add one or two levels of nestedness and you are wrapping this line.
>
>> In some cases, it is not apparent which type it is converted to,
>> for example:
>>
>> + m = (void *)(targ_type + 1);
>>
>> I would suggest we do implicit conversion whenever possible.
>
> Implicit conversion (`m = targ_type + 1;`) is a compilation error,
> that won't work.

I misused "implicit" here. I actually meant to say

m = ((const struct btf_member *)(t + 1);