On Tue, Oct 15, 2019 at 8:15 AM Alexei Starovoitov <ast@xxxxxx> wrote:
>
> On 10/14/19 4:49 PM, Andrii Nakryiko wrote:
> > BTF offset reloc was generalized in recent Clang into field relocation,
> > capturing extra u32 field, specifying what aspect of captured field
> > needs to be relocated. This changes .BTF.ext's record size for this
> > relocation from 12 bytes to 16 bytes. Given these format changes
> > happened in Clang before official released version, it's ok to not
> > support outdated 12-byte record size w/o breaking ABI.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> ...> -/* The minimum bpf_offset_reloc checked by the loader
> > +/* bpf_field_reloc_kind encodes which aspect of captured field has to be
> > + * adjusted by relocations. Currently supported values are:
> > + * - BPF_FI_BYTE_OFFSET: field offset (in bytes);
> > + * - BPF_FI_EXISTS: field existence (1 if field exists, 0 - otherwise);
> > + */
> > +enum bpf_field_reloc_kind {
> > + BPF_FRK_BYTE_OFFSET = 0,
> > + BPF_FRK_EXISTS = 2,
> > +};
>
> Comment above doesn't match the enum.
he-he, you can see that I went back and forth with names :)
> Also in patch 4 :
> +enum bpf_field_info_kind {
> + BPF_FI_BYTE_OFFSET = 0, /* field byte offset */
> + BPF_FI_EXISTS = 2, /* whether field exists in target kernel */
> +};
>
> Do you expect that .btf.ext encoding to be different from
> argument passed into __builtin_field_info() ?
> May be better to design the interface that it always matches
> and keep single enum for both?
> I don't like either FRK or FI abbreviations.
> May be
> BPF_FIELD_BYTE_OFFSET
> BPF_FIELD_EXISTS ?
>
> These constants would need to be exposed in bpf_core_read.h and
> will become uapi as soon as we release next libbpf at
> the end of this bpf-next cycle. At that point llvm side would have
> to stick to these constants as well.
> It would have to understand them as arguments and use the same in .btf.ext.
> Does it make sense?
yeah, it does. My thinking was that we already have two built-ins that
share the same BTF relocation (__builtin_preserve_access_index() and
__builtin_preserve_field_info()), so I figured it might be worth-while
to not couple low-level relocation and bulit-in parameters together,
because it might be the case in the future where we'll be emitting
multiple relos from single built-in or some other arrangement.
But I think it's ok to couple enum definitions for now. In the end,
it's just numbers, so if we ever need to diverge, it will be possible.
The only exposed constants are those that are in bpf_core_read.h and
are only supposed to be passed into __builtin_preserve_field_info().
libbpf's enum bpf_field_reloc_kind is internal, so we can change it
whenever we need to.
btw, I'm going to add bpf_core_field_exists() macro to
bpf_core_read.h, similar to bpf_core_read() macro, hope it's not very
controversial.
>
