* [PATCH bpf-next 0/2] bpftool: fix newline for struct with padding only fields
@ 2022-09-30 16:49 Eduard Zingerman
2022-09-30 16:49 ` [PATCH bpf-next 1/2] " Eduard Zingerman
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eduard Zingerman @ 2022-09-30 16:49 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team; +Cc: Eduard Zingerman
Hi Everyone,
a small fix for bpftool, copying commit message from the first patch
as it explains the modification.
An update for `bpftool btf dump file ... format c`.
Add a missing newline print for structures that consist of
anonymous-only padding fields. E.g. here is struct bpf_timer from
vmlinux.h before this patch:
struct bpf_timer {
long: 64;
long: 64;};
And after this patch:
struct bpf_dynptr {
long: 64;
long: 64;
};
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
Eduard Zingerman (2):
bpftool: fix newline for struct with padding only fields
selftests/bpf: verify newline for struct with padding only fields
tools/lib/bpf/btf_dump.c | 15 +++++++++------
.../bpf/progs/btf_dump_test_case_padding.c | 16 ++++++++++++++++
2 files changed, 25 insertions(+), 6 deletions(-)
--
2.37.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/2] bpftool: fix newline for struct with padding only fields
2022-09-30 16:49 [PATCH bpf-next 0/2] bpftool: fix newline for struct with padding only fields Eduard Zingerman
@ 2022-09-30 16:49 ` Eduard Zingerman
2022-09-30 23:01 ` Andrii Nakryiko
2022-09-30 16:49 ` [PATCH bpf-next 2/2] selftests/bpf: verify " Eduard Zingerman
2022-09-30 22:59 ` [PATCH bpf-next 0/2] bpftool: fix " Andrii Nakryiko
2 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2022-09-30 16:49 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team; +Cc: Eduard Zingerman
An update for `bpftool btf dump file ... format c`.
Add a missing newline print for structures that consist of
anonymous-only padding fields. E.g. here is struct bpf_timer from
vmlinux.h before this patch:
struct bpf_timer {
long: 64;
long: 64;};
And after this patch:
struct bpf_dynptr {
long: 64;
long: 64;
};
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/lib/bpf/btf_dump.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 4221f73a74d0..ebbba19ac122 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -852,7 +852,7 @@ static int chip_away_bits(int total, int at_most)
return total % at_most ? : at_most;
}
-static void btf_dump_emit_bit_padding(const struct btf_dump *d,
+static bool btf_dump_emit_bit_padding(const struct btf_dump *d,
int cur_off, int m_off, int m_bit_sz,
int align, int lvl)
{
@@ -861,10 +861,10 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
if (off_diff <= 0)
/* no gap */
- return;
+ return false;
if (m_bit_sz == 0 && off_diff < align * 8)
/* natural padding will take care of a gap */
- return;
+ return false;
while (off_diff > 0) {
const char *pad_type;
@@ -886,6 +886,8 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, pad_bits);
off_diff -= pad_bits;
}
+
+ return true;
}
static void btf_dump_emit_struct_fwd(struct btf_dump *d, __u32 id,
@@ -906,6 +908,7 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
bool is_struct = btf_is_struct(t);
int align, i, packed, off = 0;
__u16 vlen = btf_vlen(t);
+ bool padding_added = false;
packed = is_struct ? btf_is_struct_packed(d->btf, id, t) : 0;
@@ -940,11 +943,11 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
/* pad at the end, if necessary */
if (is_struct) {
align = packed ? 1 : btf__align_of(d->btf, id);
- btf_dump_emit_bit_padding(d, off, t->size * 8, 0, align,
- lvl + 1);
+ padding_added = btf_dump_emit_bit_padding(d, off, t->size * 8, 0, align,
+ lvl + 1);
}
- if (vlen)
+ if (vlen || padding_added)
btf_dump_printf(d, "\n");
btf_dump_printf(d, "%s}", pfx(lvl));
if (packed)
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: verify newline for struct with padding only fields
2022-09-30 16:49 [PATCH bpf-next 0/2] bpftool: fix newline for struct with padding only fields Eduard Zingerman
2022-09-30 16:49 ` [PATCH bpf-next 1/2] " Eduard Zingerman
@ 2022-09-30 16:49 ` Eduard Zingerman
2022-09-30 23:03 ` Andrii Nakryiko
2022-09-30 22:59 ` [PATCH bpf-next 0/2] bpftool: fix " Andrii Nakryiko
2 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2022-09-30 16:49 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kernel-team; +Cc: Eduard Zingerman
Verify that `bpftool btf dump file ... format c` correctly prints
newlines for structures that consist of anonymous-only padding fields.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
.../bpf/progs/btf_dump_test_case_padding.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
index f2661c8d2d90..08e43ee38188 100644
--- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
@@ -102,12 +102,28 @@ struct zone {
struct zone_padding __pad__;
};
+/* ----- START-EXPECTED-OUTPUT ----- */
+/*
+ *struct padding_wo_named_members {
+ * long: 64;
+ * long: 64;
+ *};
+ *
+ */
+/* ------ END-EXPECTED-OUTPUT ------ */
+
+struct padding_wo_named_members {
+ long: 64;
+ long: 64;
+} __attribute__((aligned(8)));
+
int f(struct {
struct padded_implicitly _1;
struct padded_explicitly _2;
struct padded_a_lot _3;
struct padded_cache_line _4;
struct zone _5;
+ struct padding_wo_named_members _6;
} *_)
{
return 0;
--
2.37.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 0/2] bpftool: fix newline for struct with padding only fields
2022-09-30 16:49 [PATCH bpf-next 0/2] bpftool: fix newline for struct with padding only fields Eduard Zingerman
2022-09-30 16:49 ` [PATCH bpf-next 1/2] " Eduard Zingerman
2022-09-30 16:49 ` [PATCH bpf-next 2/2] selftests/bpf: verify " Eduard Zingerman
@ 2022-09-30 22:59 ` Andrii Nakryiko
2 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2022-09-30 22:59 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team
On Fri, Sep 30, 2022 at 9:50 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Hi Everyone,
>
> a small fix for bpftool, copying commit message from the first patch
> as it explains the modification.
>
> An update for `bpftool btf dump file ... format c`.
> Add a missing newline print for structures that consist of
> anonymous-only padding fields. E.g. here is struct bpf_timer from
> vmlinux.h before this patch:
>
> struct bpf_timer {
> long: 64;
> long: 64;};
>
> And after this patch:
>
> struct bpf_dynptr {
> long: 64;
> long: 64;
> };
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
>
Some general feedback about cover letter submission:
1) Contents of cover letter is embedded into merge commit when patch
set is applied, so "Hi Everyone," looks rather weird there. Best to
just get straight to explaining the purpose of cover letter.
2) Please keep capitalization ("a small fix" -> "A small fix").
3) I don't think we add Signed-off-by into the cover letter, so please
drop it for next revision.
> Eduard Zingerman (2):
> bpftool: fix newline for struct with padding only fields
> selftests/bpf: verify newline for struct with padding only fields
>
> tools/lib/bpf/btf_dump.c | 15 +++++++++------
> .../bpf/progs/btf_dump_test_case_padding.c | 16 ++++++++++++++++
> 2 files changed, 25 insertions(+), 6 deletions(-)
>
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpftool: fix newline for struct with padding only fields
2022-09-30 16:49 ` [PATCH bpf-next 1/2] " Eduard Zingerman
@ 2022-09-30 23:01 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2022-09-30 23:01 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team
On Fri, Sep 30, 2022 at 9:50 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> An update for `bpftool btf dump file ... format c`.
> Add a missing newline print for structures that consist of
> anonymous-only padding fields. E.g. here is struct bpf_timer from
> vmlinux.h before this patch:
>
> struct bpf_timer {
> long: 64;
> long: 64;};
>
> And after this patch:
>
> struct bpf_dynptr {
> long: 64;
> long: 64;
> };
Without looking at source code it wasn't clear what the original issue
was. It would be good to explain that libbpf's btf_dumper attempts to
emit empty structs with {} on the same line. But this breaks for
non-zero-sized structs due to padding.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> tools/lib/bpf/btf_dump.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 4221f73a74d0..ebbba19ac122 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -852,7 +852,7 @@ static int chip_away_bits(int total, int at_most)
> return total % at_most ? : at_most;
> }
>
> -static void btf_dump_emit_bit_padding(const struct btf_dump *d,
> +static bool btf_dump_emit_bit_padding(const struct btf_dump *d,
> int cur_off, int m_off, int m_bit_sz,
> int align, int lvl)
> {
> @@ -861,10 +861,10 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
>
> if (off_diff <= 0)
> /* no gap */
> - return;
> + return false;
> if (m_bit_sz == 0 && off_diff < align * 8)
> /* natural padding will take care of a gap */
> - return;
> + return false;
>
> while (off_diff > 0) {
> const char *pad_type;
> @@ -886,6 +886,8 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
> btf_dump_printf(d, "\n%s%s: %d;", pfx(lvl), pad_type, pad_bits);
> off_diff -= pad_bits;
> }
> +
> + return true;
> }
>
> static void btf_dump_emit_struct_fwd(struct btf_dump *d, __u32 id,
> @@ -906,6 +908,7 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
> bool is_struct = btf_is_struct(t);
> int align, i, packed, off = 0;
> __u16 vlen = btf_vlen(t);
> + bool padding_added = false;
>
> packed = is_struct ? btf_is_struct_packed(d->btf, id, t) : 0;
>
> @@ -940,11 +943,11 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
> /* pad at the end, if necessary */
> if (is_struct) {
> align = packed ? 1 : btf__align_of(d->btf, id);
> - btf_dump_emit_bit_padding(d, off, t->size * 8, 0, align,
> - lvl + 1);
> + padding_added = btf_dump_emit_bit_padding(d, off, t->size * 8, 0, align,
> + lvl + 1);
> }
>
> - if (vlen)
> + if (vlen || padding_added)
What if instead of returning the padding_added flag we just check that
struct is non-zero-sized? Clearly vlen isn't an appropriate check
here.
> btf_dump_printf(d, "\n");
> btf_dump_printf(d, "%s}", pfx(lvl));
> if (packed)
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: verify newline for struct with padding only fields
2022-09-30 16:49 ` [PATCH bpf-next 2/2] selftests/bpf: verify " Eduard Zingerman
@ 2022-09-30 23:03 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2022-09-30 23:03 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team
On Fri, Sep 30, 2022 at 9:50 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Verify that `bpftool btf dump file ... format c` correctly prints
> newlines for structures that consist of anonymous-only padding fields.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> .../bpf/progs/btf_dump_test_case_padding.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
> index f2661c8d2d90..08e43ee38188 100644
> --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
> +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c
> @@ -102,12 +102,28 @@ struct zone {
> struct zone_padding __pad__;
> };
>
> +/* ----- START-EXPECTED-OUTPUT ----- */
> +/*
> + *struct padding_wo_named_members {
> + * long: 64;
> + * long: 64;
> + *};
> + *
> + */
> +/* ------ END-EXPECTED-OUTPUT ------ */
> +
> +struct padding_wo_named_members {
> + long: 64;
> + long: 64;
> +} __attribute__((aligned(8)));
you don't really need aligned(8) attribute, if you drop it you can
have a single copy of the struct (just like padded_implicitly above)
> +
> int f(struct {
> struct padded_implicitly _1;
> struct padded_explicitly _2;
> struct padded_a_lot _3;
> struct padded_cache_line _4;
> struct zone _5;
> + struct padding_wo_named_members _6;
> } *_)
> {
> return 0;
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-30 23:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-30 16:49 [PATCH bpf-next 0/2] bpftool: fix newline for struct with padding only fields Eduard Zingerman
2022-09-30 16:49 ` [PATCH bpf-next 1/2] " Eduard Zingerman
2022-09-30 23:01 ` Andrii Nakryiko
2022-09-30 16:49 ` [PATCH bpf-next 2/2] selftests/bpf: verify " Eduard Zingerman
2022-09-30 23:03 ` Andrii Nakryiko
2022-09-30 22:59 ` [PATCH bpf-next 0/2] bpftool: fix " Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox