> On Jul 31, 2019, at 10:18 AM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Wed, Jul 31, 2019 at 1:30 AM Song Liu <songliubraving@xxxxxx> wrote: >> >> >> >>> On Jul 30, 2019, at 11:52 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: >>> >>> On Tue, Jul 30, 2019 at 10:19 PM Song Liu <songliubraving@xxxxxx> wrote: >>>> >>>> >>>> >>>>> On Jul 30, 2019, at 6:00 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: >>>>> >>>>> On Tue, Jul 30, 2019 at 5:39 PM Song Liu <songliubraving@xxxxxx> wrote: >>>>>> >>>>>> >>>>>> >>>>>>> On Jul 30, 2019, at 12:53 PM, Andrii Nakryiko <andriin@xxxxxx> 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@xxxxxx> >> >> [...] >> >>>>>> >>>>> >>>>> 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 > > Problem is too big and strong word to describe this :). It hurts > readability and will often quite artificially force either wrapping > the line or unnecessarily splitting declaration and assignment. Void * > on the other hand is short and usually is in the same line as target > type declaration, if not, you'll have to find local variable > declaration to double-check type, if you are unsure. > > Using (void *) + implicit cast to target pointer type is not > unprecedented in libbpf: > > $ rg ' = \((const )?struct \w+ \*\)' tools/lib/bpf/ | wc -l > 52 > $ rg ' = \((const )?void \*\)' tools/lib/bpf/ | wc -l > 35 > > 52 vs 35 is majority overall, but not by a landslide. > >> (void *) feels like a bigger problem. > > Why do you feel it's a problem? void * conveys that we have a piece of > memory that we will need to reinterpret as some concrete pointer type. > That's what we are doing, skipping btf_type and then interpreting > memory after common btf_type prefix is some other type, depending on > actual BTF kind. I don't think void * is misleading in any way. (void *) hides some problem. For example: struct type_a *ptr_a = NULL; struct type_b *ptr_b = NULL; /* we want this */ ptr_a = (struct type_a *)data; ptr_b = (struct type_b *)(data + offset); /* typo, should be ptr_b, compiler will complain */ ptr_a = (struct type_b *)(data + offset); /* typo, should be ptr_b, compiler will ignore */ ptr_a = (void *)(data + offset); Such typo is not very rare. And it may be really painful to debug. That being said, I think we have spent too much time on this. I will let you make the final call. Either way: Acked-by: Song Liu <songliubraving@xxxxxx>
