Fix issues with libbpf BTF typed dump code. Patch 1 addresses handling
of unaligned data. Patch 2 fixes issues Andrii noticed when compiling
on ppc64le. Patch 3 simplifies typed dump by getting rid of allocation
of dump data structure which tracks dump state etc.
Changes since v1:
- Andrii suggested using a function instead of a macro for checking
alignment of data, and pointed out that we need to consider dump
ptr size versus native pointer size (patch 1)
Alan Maguire (3):
libbpf: clarify/fix unaligned data issues for btf typed dump
libbpf: fix compilation errors on ppc64le for btf dump typed data
libbpf: btf typed dump does not need to allocate dump data
tools/lib/bpf/btf_dump.c | 39 ++++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
--
1.8.3.1
Andrii reports:
"ppc64le arch doesn't like the %lld:
In file included from btf_dump.c:22:
btf_dump.c: In function 'btf_dump_type_data_check_overflow':
libbpf_internal.h:111:22: error: format '%lld' expects argument of
type 'long long int', but argument 3 has type '__s64' {aka 'long int'}
[-Werror=format=]
111 | libbpf_print(level, "libbpf: " fmt, ##__VA_ARGS__); \
| ^~~~~~~~~~
libbpf_internal.h:114:27: note: in expansion of macro '__pr'
114 | #define pr_warn(fmt, ...) __pr(LIBBPF_WARN, fmt, ##__VA_ARGS__)
| ^~~~
btf_dump.c:1992:3: note: in expansion of macro 'pr_warn'
1992 | pr_warn("unexpected size [%lld] for id [%u]\n",
| ^~~~~~~
btf_dump.c:1992:32: note: format string is defined here
1992 | pr_warn("unexpected size [%lld] for id [%u]\n",
| ~~~^
| |
| long long int
| %ld
Cast to size_t and use %zu."
Reported-by: Andrii Nakryiko <[email protected]>
Signed-off-by: Alan Maguire <[email protected]>
---
tools/lib/bpf/btf_dump.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 814a538..e5fbfb8 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -2011,8 +2011,8 @@ static int btf_dump_type_data_check_overflow(struct btf_dump *d,
__s64 size = btf__resolve_size(d->btf, id);
if (size < 0 || size >= INT_MAX) {
- pr_warn("unexpected size [%lld] for id [%u]\n",
- size, id);
+ pr_warn("unexpected size [%zu] for id [%u]\n",
+ (size_t)size, id);
return -EINVAL;
}
--
1.8.3.1
On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <[email protected]> wrote:
>
> Fix issues with libbpf BTF typed dump code. Patch 1 addresses handling
> of unaligned data. Patch 2 fixes issues Andrii noticed when compiling
> on ppc64le. Patch 3 simplifies typed dump by getting rid of allocation
> of dump data structure which tracks dump state etc.
>
> Changes since v1:
>
> - Andrii suggested using a function instead of a macro for checking
> alignment of data, and pointed out that we need to consider dump
> ptr size versus native pointer size (patch 1)
>
> Alan Maguire (3):
> libbpf: clarify/fix unaligned data issues for btf typed dump
> libbpf: fix compilation errors on ppc64le for btf dump typed data
> libbpf: btf typed dump does not need to allocate dump data
>
> tools/lib/bpf/btf_dump.c | 39 ++++++++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> --
> 1.8.3.1
>
Thank you for the quick follow up. But see all the comments I left and
the fix ups I had to do. Just because the changes are small doesn't
mean you should get sloppy about making them. Please be a bit more
thorough in future patches.
Applied to bpf-next.
On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <[email protected]> wrote:
>
> Andrii reports:
>
> "ppc64le arch doesn't like the %lld:
>
> In file included from btf_dump.c:22:
> btf_dump.c: In function 'btf_dump_type_data_check_overflow':
> libbpf_internal.h:111:22: error: format '%lld' expects argument of
> type 'long long int', but argument 3 has type '__s64' {aka 'long int'}
> [-Werror=format=]
> 111 | libbpf_print(level, "libbpf: " fmt, ##__VA_ARGS__); \
> | ^~~~~~~~~~
> libbpf_internal.h:114:27: note: in expansion of macro '__pr'
> 114 | #define pr_warn(fmt, ...) __pr(LIBBPF_WARN, fmt, ##__VA_ARGS__)
> | ^~~~
> btf_dump.c:1992:3: note: in expansion of macro 'pr_warn'
> 1992 | pr_warn("unexpected size [%lld] for id [%u]\n",
> | ^~~~~~~
> btf_dump.c:1992:32: note: format string is defined here
> 1992 | pr_warn("unexpected size [%lld] for id [%u]\n",
> | ~~~^
> | |
> | long long int
> | %ld
>
> Cast to size_t and use %zu."
>
Quoting me isn't a great commit message by itself, tbh. Reworded.
> Reported-by: Andrii Nakryiko <[email protected]>
> Signed-off-by: Alan Maguire <[email protected]>
> ---
> tools/lib/bpf/btf_dump.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 814a538..e5fbfb8 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -2011,8 +2011,8 @@ static int btf_dump_type_data_check_overflow(struct btf_dump *d,
> __s64 size = btf__resolve_size(d->btf, id);
>
> if (size < 0 || size >= INT_MAX) {
> - pr_warn("unexpected size [%lld] for id [%u]\n",
> - size, id);
> + pr_warn("unexpected size [%zu] for id [%u]\n",
> + (size_t)size, id);
> return -EINVAL;
> }
>
> --
> 1.8.3.1
>
On Fri, Jul 16, 2021 at 5:32 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <[email protected]> wrote:
> >
> > Fix issues with libbpf BTF typed dump code. Patch 1 addresses handling
> > of unaligned data. Patch 2 fixes issues Andrii noticed when compiling
> > on ppc64le. Patch 3 simplifies typed dump by getting rid of allocation
> > of dump data structure which tracks dump state etc.
> >
> > Changes since v1:
> >
> > - Andrii suggested using a function instead of a macro for checking
> > alignment of data, and pointed out that we need to consider dump
> > ptr size versus native pointer size (patch 1)
> >
> > Alan Maguire (3):
> > libbpf: clarify/fix unaligned data issues for btf typed dump
> > libbpf: fix compilation errors on ppc64le for btf dump typed data
> > libbpf: btf typed dump does not need to allocate dump data
> >
> > tools/lib/bpf/btf_dump.c | 39 ++++++++++++++++++++++++++++++---------
> > 1 file changed, 30 insertions(+), 9 deletions(-)
> >
> > --
> > 1.8.3.1
> >
>
> Thank you for the quick follow up. But see all the comments I left and
One more thing. Please do reply to the rest of my questions and
concerns on the original patch set. You tend to just address most of
the feedback in the next revision, but I'd appreciate it if you could
reply at least with a simple "ok" or more elaborate answer where
warranted. It makes the code reviewing process a bit easier.
There are still big-endian concerns and an error propagation question
at least that you haven't addressed neither in the follow up patches
nor in an email reply. Please do so, preferably both.
> the fix ups I had to do. Just because the changes are small doesn't
> mean you should get sloppy about making them. Please be a bit more
> thorough in future patches.
>
> Applied to bpf-next.