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

[Delyan Kratunov <delyank@fb.com>]

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.

> 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.


> _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).

> >         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.


> 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.