On Wed, Feb 23, 2022 at 2:02 PM Delyan Kratunov <delyank@xxxxxx> wrote: > > When emitting type declarations in skeletons, bpftool will now also emit > static assertions on the size of the data/bss/rodata/etc fields. This > ensures that in situations where userspace and kernel types have the same > name but differ in size we do not silently produce incorrect results but > instead break the build. > > This was reported in [1] and as expected the repro in [2] fails to build > on the new size assert after this change. > > [1]: Closes: https://github.com/libbpf/libbpf/issues/433 > [2]: https://github.com/fuweid/iovisor-bcc-pr-3777 > > Signed-off-by: Delyan Kratunov <delyank@xxxxxx> > Acked-by: Hengqi Chen <hengqi.chen@xxxxxxxxx> > Tested-by: Hengqi Chen <hengqi.chen@xxxxxxxxx> > --- > v3 -> v4: > - rebase > - style fixes > I added a few tweaks (see below) and applied to bpf-next. Thanks! > v2 -> v3: > - group all static asserts in one function at the end of the file > - only use macros in C++ mode > > v1 -> v2: > - drop the stdint approach in favor of static asserts right after the structs > > tools/bpf/bpftool/gen.c | 133 +++++++++++++++++++++++++++++++++------- > 1 file changed, 111 insertions(+), 22 deletions(-) > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > index f8c1413523a3..a42545bcb12d 100644 > --- a/tools/bpf/bpftool/gen.c > +++ b/tools/bpf/bpftool/gen.c > @@ -209,15 +209,38 @@ static int codegen_datasec_def(struct bpf_object *obj, > return 0; > } > > +static const struct btf_type *find_type_for_map(struct bpf_object *obj, > + const char *map_ident) this doesn't need entire bpf_object, it's enough to pass struct btf directly, I changed it to take just btf > +{ > + struct btf *btf = bpf_object__btf(obj); > + int n = btf__type_cnt(btf), i; > + char sec_ident[256]; > + [...] > +/* Emit type size asserts for all top-level fields in memory-mapped internal maps. > + */ > +static void codegen_asserts(struct bpf_object *obj, const char *obj_name) > +{ > + struct btf *btf = bpf_object__btf(obj); > + struct bpf_map *map; > + struct btf_var_secinfo *sec_var; > + int i, vlen; > + const struct btf_type *sec; > + char map_ident[256], var_ident[256]; > + > + codegen("\ > + \n\ > + \n\ > + #ifdef __cplusplus \n\ > + #define _Static_assert static_assert \n\ > + #endif \n\ I moved this _Static_assert business inside the <skel>__type_asserts() function. I also thought that if in the future we want to add some other asserts, then having a more generic "<skel>__assert()" name would be more appropriate, so I renamed it to just "<skel>__assert". > + \n\ > + __attribute__((unused)) static void \n\ > + %1$s__type_asserts(struct %1$s *s) \n\ > + { \n\ > + ", obj_name); > + [...] > + var_ident[0] = '\0'; > + strncat(var_ident, var_name, sizeof(var_ident) - 1); > + sanitize_identifier(var_ident); > + > + printf("\t_Static_assert(sizeof(s->%1$s->%2$s) == %3$lld, \"unexpected size of '%2$s'\");\n", > + map_ident, var_ident, var_size); __s64 isn't %lld on each supported architecture. I just used long and %ld instead of __s64 to avoid compilation warnings. > + } > + } > + codegen("\ > + \n\ > + } \n\ > + \n\ > + #ifdef __cplusplus \n\ > + #undef _Static_assert \n\ > + #endif \n\ > + "); > +} > + > + [...]
