Re: [PATCH bpf-next v2 1/1] bpftool: bpf skeletons assert type sizes

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Feb 15, 2022 at 9:27 AM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Mon, 2022-02-14 at 21:11 -0800, Andrii Nakryiko wrote:
> > So doing it right after each section really pollutes the layout of the
> > skeleton's struct and hurts readability a lot.
> >
> > How about adding all those _Static_asserts in <skeleton__elf_bytes()
> > function, after the huge binary dump, to get it out of sight?
>
> I can just add a `void __attribute__((unused)) skeleton__assert_sizes()` at the
> end? Or a `struct skeleton__type_asserts`? It feels weird to just put them in
> elf_bytes, they don't belong there.

SGTM.

>
> > I think
> > if we are doing asserts, we might as well validate that not just
> > sizes, but also each variable's offset within the section is right.
>
> Sure, can do.

Alexei pointed out that it's very unlikely that we'll mess up offsets
(we have actual offset from BTF and then we control alignment in
skeleton's struct, so should never get out of sync), so let's skip
offset assertion for now.

>
>
> > _Static_assert(sizeof(s->data->in1) == 4, "invalid size of in1");
> > _Static_assert(offsetof(typeof(*skel->data), in1) == 0, "invalid
> > offset of in1");
> > ...
> > _Static_assert(sizeof(s->data_read_mostly->read_mostly_var) == 4,
> > "invalid size of read_mostly_var");
> > _Static_assert(offsetof(typeof(*skel->data_read_mostly),
> > read_mostly_var) == 0, "invalid offset of read_mostly_var");
> >
> > (void)s; /* avoid unused variable warning */
> >
> > WDYT?
>
> That's fine by me, I have no objections. I'll see if a function or a struct is
> more readable.
>
> I suspect `SIZE_ASSERT(data, in1, 4); OFFSET_ASSERT(data, in1, 0);` is probably
> most readable but I hate that I'd have to include the macros inline (to emit the
> skeleton type name).

No one should read those asserts, so putting them somewhere after
elf_bytes function and writing out _Static_assert() directly is
probably best for when one of those asserts fires. It will result in
simpler compiler error (rather than unscrambling a chain of macro
invocations). So yeah, I'd stick to a bit more verbose _Static_assert.


>
> > >         return 0;
> > >  }
> > >
> > > @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv)
> > >                                                                             \n\
> > >                 #include <bpf/skel_internal.h>                              \n\
> > >                                                                             \n\
> > > +               #ifdef __cplusplus                                          \n\
> > > +               #define BPF_STATIC_ASSERT static_assert                     \n\
> > > +               #else                                                       \n\
> > > +               #define BPF_STATIC_ASSERT _Static_assert                    \n\
> > > +               #endif                                                      \n\
> >
> > Maybe just:
> >
> > #ifdef __cplusplus
> > #define _Static_assert static_assert
> > #endif
> >
> > ? Or that doesn't work?
>
> It does work, it's just less explicit. I'd be happy to remove the macro
> expansion on the C path though, it would make diagnostics shorter.

Yep, it was my thinking that we should "optimize" for pure C case.

>
>
> > Also any such macro has to be #undef in this file, otherwise it will
> > "leak" into the user's code (as this is just a header file included in
> > user's .c files).
>
> My bad, just thought of that too.
>
> --
>
> To summarize, structurally I'll do this:
>
> 1. Put them all in one place. (tbd what type)
> 2. Put them at the end of the file.
> 3. Add offsets.
> 4. Fix up the macro usage.
>