On Wed, Nov 9, 2022 at 3:11 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> >
> > > > >
> > > > > So what if we have
> > > > >
> > > > > struct bpf_list_node {
> > > > > __u64 __opaque[2];
> > > > > } __attribute__((aligned(8)));
> > > > >
> > > > > ?
> > > > >
> > > >
> > > > Yep, can do that. Note that it's also potentially an issue for existing cases,
> > > > like bpf_spin_lock, bpf_timer, bpf_dynptr, etc. Not completely sure if changing
> > > > things now is possible, but if it is, we should probably make it for all of
> > > > them?
> > >
> > > Why not? We are not removing anything or changing sizes, so it's
> > > backwards compatible.
> > > But I have a suspicion Alexei might not like
> > > this __opaque field, so let's see what he says.
> >
> > I prefer to fix them all at once without adding a name.
> >
>
> This is not an issue with BTF per se.
>
> struct blah {
> u64: 64
> };
>
> is just an empty struct blah with 8-byte size. Both BTF and DWARF will
> record it as just
>
> struct blah {
> }
>
> and record it's size as 8 bytes.
>
> With that, there is nothing to suggest that this struct has to have
> 8-byte alignment.
>
> If we mark explicitly __attribute__((aligned(8))) then DWARF will
> additionally record alignment=8 for such struct. BTF doesn't record
> alignment, though.
>
> adding u64 fields internally will make libbpf recognize that struct
> needs at least 8-byte alignment, which is what I propose as a simple
> solution.
>
> Alternatives are:
> - extend BTF to record struct/union alignments in BTF_KIND_{STRUCT,UNION}
> - record __attribute__((aligned(8))) as a new KIND (BTF_KIND_ATTRIBUTE)
imo above two options are way better than adding __opaque which
is nothing but a workaround for a missing feature in BTF.
> Both seem like a bit of an overkill, given the work around is to have
> u64 __opaque[] fields, which we won't have to remove or rename ever
> (because those fields are not used).
There is no rush to do this workaround.
As Kumar says we can land the existing patch as-is
and add BTF_KIND_ATTR (or anything else) in the followup.
Adding __opaque now won't work, since we won't be able
to remove it later due to backward compat.
