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.
