On 2/16/22 09:19, Andrii Nakryiko wrote: > On Tue, Feb 15, 2022 at 4:12 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> >> --- > > LGTM with a trivial styling nits. But this doesn't apply cleanly to > bpf-next (see [0]). Can you please rebase and resend. Also for > single-patch submissions we don't require cover letter, to please just > put all the description into one patch without cover letter. > > [0] https://github.com/kernel-patches/bpf/pull/2563#issuecomment-1040929960 > >> tools/bpf/bpftool/gen.c | 134 +++++++++++++++++++++++++++++++++------- >> 1 file changed, 112 insertions(+), 22 deletions(-) > > [...] > >> + >> + bpf_object__for_each_map(map, obj) { >> + if (!bpf_map__is_internal(map)) >> + continue; >> + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE)) >> + continue; >> + if (!get_map_ident(map, map_ident, sizeof(map_ident))) >> + continue; >> + >> + sec = find_type_for_map(obj, map_ident); >> + > > nit: unnecessary empty line between assignment and "error checking" > >> + if (!sec) { >> + /* best effort, couldn't find the type for this map */ >> + continue; >> + } >> + >> + sec_var = btf_var_secinfos(sec); >> + vlen = btf_vlen(sec); >> + >> + for (i = 0; i < vlen; i++, sec_var++) { >> + const struct btf_type *var = btf__type_by_id(btf, sec_var->type); >> + const char *var_name = btf__name_by_offset(btf, var->name_off); >> + __u32 var_type_id = var->type; >> + __s64 var_size = btf__resolve_size(btf, var_type_id); >> + >> + if (var_size < 0) >> + continue; >> + >> + /* static variables are not exposed through BPF skeleton */ >> + if (btf_var(var)->linkage == BTF_VAR_STATIC) >> + continue; >> + >> + var_ident[0] = '\0'; >> + strncat(var_ident, var_name, sizeof(var_ident) - 1); >> + sanitize_identifier(var_ident); >> + >> + printf("\t_Static_assert("); >> + printf("sizeof(s->%1$s->%2$s) == %3$lld, ", >> + map_ident, var_ident, var_size); >> + printf("\"unexpected size of '%1$s'\");\n", var_ident); > > nit: I'd keep this as one printf, it makes it a bit easier to follow. > >> + } >> + } > > [...] Feel free to add: Acked-by: Hengqi Chen <hengqi.chen@xxxxxxxxx> Tested-by: Hengqi Chen <hengqi.chen@xxxxxxxxx>
