Message ID | 20200826030922.2591203-1-andriin@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next] libbpf: fix compilation warnings for 64-bit printf args | expand |
On Tue, Aug 25, 2020 at 08:09:21PM -0700, Andrii Nakryiko wrote: > Add __pu64 and __ps64 (sort of like "printf u64 and s64") for libbpf-internal > use only in printf-like situations to avoid compilation warnings due to > %lld/%llu mismatch with a __u64/__s64 due to some architecture defining the > latter as either `long` or `long long`. Use that on all %lld/%llu cases in > libbpf.c. > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Fixes: eacaaed784e2 ("libbpf: Implement enum value-based CO-RE relocations") > Fixes: 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF") > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/lib/bpf/libbpf.c | 15 ++++++++------- > tools/lib/bpf/libbpf_internal.h | 11 +++++++++++ > 2 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 2e2523d8bb6d..211eb0d9020c 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -1529,12 +1529,12 @@ static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val, > { > if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) { > pr_warn("extern (kcfg) %s=%llu should be integer\n", > - ext->name, (unsigned long long)value); > + ext->name, (__pu64)value); > return -EINVAL; > } > if (!is_kcfg_value_in_range(ext, value)) { > pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n", > - ext->name, (unsigned long long)value, ext->kcfg.sz); > + ext->name, (__pu64)value, ext->kcfg.sz); > return -ERANGE; > } > switch (ext->kcfg.sz) { > @@ -2823,7 +2823,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj) > obj->efile.bss = data; > obj->efile.bss_shndx = idx; > } else { > - pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name, sh.sh_size); > + pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name, > + (size_t)sh.sh_size); > } > } > > @@ -5244,7 +5245,7 @@ static int bpf_core_patch_insn(struct bpf_program *prog, > if (res->validate && imm != orig_val) { > pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n", > bpf_program__title(prog, false), relo_idx, > - insn_idx, imm, orig_val, new_val); > + insn_idx, (__pu64)imm, orig_val, new_val); > return -EINVAL; > } > > @@ -5252,7 +5253,7 @@ static int bpf_core_patch_insn(struct bpf_program *prog, > insn[1].imm = 0; /* currently only 32-bit values are supported */ > pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", > bpf_program__title(prog, false), relo_idx, insn_idx, > - imm, new_val); > + (__pu64)imm, new_val); > break; > } > default: > @@ -7782,8 +7783,8 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj, > st_ops = map->st_ops; > pr_debug("struct_ops reloc %s: for %lld value %lld shdr_idx %u rel.r_offset %zu map->sec_offset %zu name %d (\'%s\')\n", > map->name, > - (long long)(rel.r_info >> 32), > - (long long)sym.st_value, > + (__ps64)(rel.r_info >> 32), > + (__ps64)sym.st_value, > shdr_idx, (size_t)rel.r_offset, > map->sec_offset, sym.st_name, name); > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > index 4d1c366fca2c..7ad3c4b9917c 100644 > --- a/tools/lib/bpf/libbpf_internal.h > +++ b/tools/lib/bpf/libbpf_internal.h > @@ -69,6 +69,17 @@ extern void libbpf_print(enum libbpf_print_level level, > const char *format, ...) > __attribute__((format(printf, 2, 3))); > > +/* These types are for casting 64-bit arguments of printf-like functions to > + * avoid compiler warnings on various architectures that define size_t, __u64, > + * uint64_t, etc as either unsigned long or unsigned long long (similarly for > + * signed variants). Use these typedefs only for these purposes. Alternative > + * is PRIu64 (and similar) macros, requiring stitching printf format strings > + * which are extremely ugly and should be avoided in libbpf code base. With > + * arguments casted to __pu64/__ps64, always use %llu/%lld in format string. > + */ > +typedef unsigned long long __pu64; > +typedef long long __ps64; I think these extra typedefs will cause confusion. Original approach of open coding type casts to long long and unsigned long long is imo cleaner.
On Wed, Aug 26, 2020 at 9:08 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 08:09:21PM -0700, Andrii Nakryiko wrote: > > Add __pu64 and __ps64 (sort of like "printf u64 and s64") for libbpf-internal > > use only in printf-like situations to avoid compilation warnings due to > > %lld/%llu mismatch with a __u64/__s64 due to some architecture defining the > > latter as either `long` or `long long`. Use that on all %lld/%llu cases in > > libbpf.c. > > > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > > Fixes: eacaaed784e2 ("libbpf: Implement enum value-based CO-RE relocations") > > Fixes: 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF") > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > tools/lib/bpf/libbpf.c | 15 ++++++++------- > > tools/lib/bpf/libbpf_internal.h | 11 +++++++++++ > > 2 files changed, 19 insertions(+), 7 deletions(-) > > [...] > > > > +/* These types are for casting 64-bit arguments of printf-like functions to > > + * avoid compiler warnings on various architectures that define size_t, __u64, > > + * uint64_t, etc as either unsigned long or unsigned long long (similarly for > > + * signed variants). Use these typedefs only for these purposes. Alternative > > + * is PRIu64 (and similar) macros, requiring stitching printf format strings > > + * which are extremely ugly and should be avoided in libbpf code base. With > > + * arguments casted to __pu64/__ps64, always use %llu/%lld in format string. > > + */ > > +typedef unsigned long long __pu64; > > +typedef long long __ps64; > > I think these extra typedefs will cause confusion. Original approach > of open coding type casts to long long and unsigned long long is imo cleaner. Fair enough. Sent v2 with just direct "unsigned long long" casts.
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 2e2523d8bb6d..211eb0d9020c 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -1529,12 +1529,12 @@ static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val, { if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) { pr_warn("extern (kcfg) %s=%llu should be integer\n", - ext->name, (unsigned long long)value); + ext->name, (__pu64)value); return -EINVAL; } if (!is_kcfg_value_in_range(ext, value)) { pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n", - ext->name, (unsigned long long)value, ext->kcfg.sz); + ext->name, (__pu64)value, ext->kcfg.sz); return -ERANGE; } switch (ext->kcfg.sz) { @@ -2823,7 +2823,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj) obj->efile.bss = data; obj->efile.bss_shndx = idx; } else { - pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name, sh.sh_size); + pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name, + (size_t)sh.sh_size); } } @@ -5244,7 +5245,7 @@ static int bpf_core_patch_insn(struct bpf_program *prog, if (res->validate && imm != orig_val) { pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n", bpf_program__title(prog, false), relo_idx, - insn_idx, imm, orig_val, new_val); + insn_idx, (__pu64)imm, orig_val, new_val); return -EINVAL; } @@ -5252,7 +5253,7 @@ static int bpf_core_patch_insn(struct bpf_program *prog, insn[1].imm = 0; /* currently only 32-bit values are supported */ pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", bpf_program__title(prog, false), relo_idx, insn_idx, - imm, new_val); + (__pu64)imm, new_val); break; } default: @@ -7782,8 +7783,8 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj, st_ops = map->st_ops; pr_debug("struct_ops reloc %s: for %lld value %lld shdr_idx %u rel.r_offset %zu map->sec_offset %zu name %d (\'%s\')\n", map->name, - (long long)(rel.r_info >> 32), - (long long)sym.st_value, + (__ps64)(rel.r_info >> 32), + (__ps64)sym.st_value, shdr_idx, (size_t)rel.r_offset, map->sec_offset, sym.st_name, name); diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h index 4d1c366fca2c..7ad3c4b9917c 100644 --- a/tools/lib/bpf/libbpf_internal.h +++ b/tools/lib/bpf/libbpf_internal.h @@ -69,6 +69,17 @@ extern void libbpf_print(enum libbpf_print_level level, const char *format, ...) __attribute__((format(printf, 2, 3))); +/* These types are for casting 64-bit arguments of printf-like functions to + * avoid compiler warnings on various architectures that define size_t, __u64, + * uint64_t, etc as either unsigned long or unsigned long long (similarly for + * signed variants). Use these typedefs only for these purposes. Alternative + * is PRIu64 (and similar) macros, requiring stitching printf format strings + * which are extremely ugly and should be avoided in libbpf code base. With + * arguments casted to __pu64/__ps64, always use %llu/%lld in format string. + */ +typedef unsigned long long __pu64; +typedef long long __ps64; + #define __pr(level, fmt, ...) \ do { \ libbpf_print(level, "libbpf: " fmt, ##__VA_ARGS__); \
Add __pu64 and __ps64 (sort of like "printf u64 and s64") for libbpf-internal use only in printf-like situations to avoid compilation warnings due to %lld/%llu mismatch with a __u64/__s64 due to some architecture defining the latter as either `long` or `long long`. Use that on all %lld/%llu cases in libbpf.c. Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> Fixes: eacaaed784e2 ("libbpf: Implement enum value-based CO-RE relocations") Fixes: 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF") Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/lib/bpf/libbpf.c | 15 ++++++++------- tools/lib/bpf/libbpf_internal.h | 11 +++++++++++ 2 files changed, 19 insertions(+), 7 deletions(-)