* [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration
@ 2019-10-21 16:57 Andrii Nakryiko
2019-10-21 17:18 ` Toke Høiland-Jørgensen
2019-10-21 19:01 ` Jakub Sitnicki
0 siblings, 2 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2019-10-21 16:57 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
LIBBPF_OPTS is implemented as a mix of field declaration and memset
+ assignment. This makes it neither variable declaration nor purely
statements, which is a problem, because you can't mix it with either
other variable declarations nor other function statements, because C90
compiler mode emits warning on mixing all that together.
This patch changes LIBBPF_OPTS into a strictly declaration of variable
and solves this problem, as can be seen in case of bpftool, which
previously would emit compiler warning, if done this way (LIBBPF_OPTS as
part of function variables declaration block).
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/bpf/bpftool/prog.c | 6 +++---
tools/lib/bpf/libbpf.h | 13 +++++++------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 27da96a797ab..1a7e8ddf8232 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1093,6 +1093,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
{
struct bpf_object_load_attr load_attr = { 0 };
enum bpf_prog_type common_prog_type = BPF_PROG_TYPE_UNSPEC;
+ LIBBPF_OPTS(bpf_object_open_opts, open_opts,
+ .relaxed_maps = relaxed_maps,
+ );
enum bpf_attach_type expected_attach_type;
struct map_replace *map_replace = NULL;
struct bpf_program *prog = NULL, *pos;
@@ -1106,9 +1109,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
const char *file;
int idx, err;
- LIBBPF_OPTS(bpf_object_open_opts, open_opts,
- .relaxed_maps = relaxed_maps,
- );
if (!REQ_ARGS(2))
return -1;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 0fdf086beba7..bf105e9e866f 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -77,12 +77,13 @@ struct bpf_object_open_attr {
* bytes, but that's the best way I've found and it seems to work in practice.
*/
#define LIBBPF_OPTS(TYPE, NAME, ...) \
- struct TYPE NAME; \
- memset(&NAME, 0, sizeof(struct TYPE)); \
- NAME = (struct TYPE) { \
- .sz = sizeof(struct TYPE), \
- __VA_ARGS__ \
- }
+ struct TYPE NAME = ({ \
+ memset(&NAME, 0, sizeof(struct TYPE)); \
+ (struct TYPE) { \
+ .sz = sizeof(struct TYPE), \
+ __VA_ARGS__ \
+ }; \
+ })
struct bpf_object_open_opts {
/* size of this struct, for forward/backward compatiblity */
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration
2019-10-21 16:57 [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration Andrii Nakryiko
@ 2019-10-21 17:18 ` Toke Høiland-Jørgensen
2019-10-21 17:38 ` Andrii Nakryiko
2019-10-21 19:01 ` Jakub Sitnicki
1 sibling, 1 reply; 7+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-21 17:18 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, netdev, ast, daniel
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Andrii Nakryiko <andriin@fb.com> writes:
> LIBBPF_OPTS is implemented as a mix of field declaration and memset
> + assignment. This makes it neither variable declaration nor purely
> statements, which is a problem, because you can't mix it with either
> other variable declarations nor other function statements, because C90
> compiler mode emits warning on mixing all that together.
>
> This patch changes LIBBPF_OPTS into a strictly declaration of variable
> and solves this problem, as can be seen in case of bpftool, which
> previously would emit compiler warning, if done this way (LIBBPF_OPTS as
> part of function variables declaration block).
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/bpf/bpftool/prog.c | 6 +++---
> tools/lib/bpf/libbpf.h | 13 +++++++------
> 2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 27da96a797ab..1a7e8ddf8232 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1093,6 +1093,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> {
> struct bpf_object_load_attr load_attr = { 0 };
> enum bpf_prog_type common_prog_type = BPF_PROG_TYPE_UNSPEC;
> + LIBBPF_OPTS(bpf_object_open_opts, open_opts,
> + .relaxed_maps = relaxed_maps,
> + );
> enum bpf_attach_type expected_attach_type;
> struct map_replace *map_replace = NULL;
> struct bpf_program *prog = NULL, *pos;
> @@ -1106,9 +1109,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> const char *file;
> int idx, err;
>
> - LIBBPF_OPTS(bpf_object_open_opts, open_opts,
> - .relaxed_maps = relaxed_maps,
> - );
>
> if (!REQ_ARGS(2))
> return -1;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 0fdf086beba7..bf105e9e866f 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -77,12 +77,13 @@ struct bpf_object_open_attr {
> * bytes, but that's the best way I've found and it seems to work in practice.
> */
> #define LIBBPF_OPTS(TYPE, NAME, ...) \
> - struct TYPE NAME; \
> - memset(&NAME, 0, sizeof(struct TYPE)); \
> - NAME = (struct TYPE) { \
> - .sz = sizeof(struct TYPE), \
> - __VA_ARGS__ \
> - }
> + struct TYPE NAME = ({ \
> + memset(&NAME, 0, sizeof(struct TYPE)); \
> + (struct TYPE) { \
> + .sz = sizeof(struct TYPE), \
Wait, you can stick arbitrary code inside a variable initialisation
block like this? How does that work? Is everything before the (struct
type) just ignored (and is that a cast)?
-Toke
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration
2019-10-21 17:18 ` Toke Høiland-Jørgensen
@ 2019-10-21 17:38 ` Andrii Nakryiko
2019-10-21 18:19 ` Yonghong Song
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2019-10-21 17:38 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Mon, Oct 21, 2019 at 10:18 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andriin@fb.com> writes:
>
> > LIBBPF_OPTS is implemented as a mix of field declaration and memset
> > + assignment. This makes it neither variable declaration nor purely
> > statements, which is a problem, because you can't mix it with either
> > other variable declarations nor other function statements, because C90
> > compiler mode emits warning on mixing all that together.
> >
> > This patch changes LIBBPF_OPTS into a strictly declaration of variable
> > and solves this problem, as can be seen in case of bpftool, which
> > previously would emit compiler warning, if done this way (LIBBPF_OPTS as
> > part of function variables declaration block).
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
[...]
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 0fdf086beba7..bf105e9e866f 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -77,12 +77,13 @@ struct bpf_object_open_attr {
> > * bytes, but that's the best way I've found and it seems to work in practice.
> > */
> > #define LIBBPF_OPTS(TYPE, NAME, ...) \
> > - struct TYPE NAME; \
> > - memset(&NAME, 0, sizeof(struct TYPE)); \
> > - NAME = (struct TYPE) { \
> > - .sz = sizeof(struct TYPE), \
> > - __VA_ARGS__ \
> > - }
> > + struct TYPE NAME = ({ \
> > + memset(&NAME, 0, sizeof(struct TYPE)); \
> > + (struct TYPE) { \
> > + .sz = sizeof(struct TYPE), \
>
> Wait, you can stick arbitrary code inside a variable initialisation
> block like this? How does that work? Is everything before the (struct
> type) just ignored (and is that a cast)?
Well, you definitely can still arbitrary code into a ({ }) expression
block, that's not that surprising.
The surprising bit that I discovered just recently was that stuff like
this compiles and works correctly, try it:
void *x = &x;
printf("%lx == %lx\n", x, &x);
So I'm using the fact that variable address is available inside
variable initialization block.
Beyond that, it's just a fancy, but standard (struct bla){ ...
initializer list ...} syntax (it's not a struct initializer syntax,
mind you, it's a struct assignment from struct literal). Fancy for
sure, but it works and solves problems I mentioned in commit
description.
>
> -Toke
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration
2019-10-21 17:38 ` Andrii Nakryiko
@ 2019-10-21 18:19 ` Yonghong Song
0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2019-10-21 18:19 UTC (permalink / raw)
To: Andrii Nakryiko, Toke Høiland-Jørgensen
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On 10/21/19 10:38 AM, Andrii Nakryiko wrote:
> On Mon, Oct 21, 2019 at 10:18 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andriin@fb.com> writes:
>>
>>> LIBBPF_OPTS is implemented as a mix of field declaration and memset
>>> + assignment. This makes it neither variable declaration nor purely
>>> statements, which is a problem, because you can't mix it with either
>>> other variable declarations nor other function statements, because C90
>>> compiler mode emits warning on mixing all that together.
>>>
>>> This patch changes LIBBPF_OPTS into a strictly declaration of variable
>>> and solves this problem, as can be seen in case of bpftool, which
>>> previously would emit compiler warning, if done this way (LIBBPF_OPTS as
>>> part of function variables declaration block).
>>>
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>> ---
>
> [...]
>
>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>> index 0fdf086beba7..bf105e9e866f 100644
>>> --- a/tools/lib/bpf/libbpf.h
>>> +++ b/tools/lib/bpf/libbpf.h
>>> @@ -77,12 +77,13 @@ struct bpf_object_open_attr {
>>> * bytes, but that's the best way I've found and it seems to work in practice.
>>> */
>>> #define LIBBPF_OPTS(TYPE, NAME, ...) \
>>> - struct TYPE NAME; \
>>> - memset(&NAME, 0, sizeof(struct TYPE)); \
>>> - NAME = (struct TYPE) { \
>>> - .sz = sizeof(struct TYPE), \
>>> - __VA_ARGS__ \
>>> - }
>>> + struct TYPE NAME = ({ \
>>> + memset(&NAME, 0, sizeof(struct TYPE)); \
>>> + (struct TYPE) { \
>>> + .sz = sizeof(struct TYPE), \
This ({ statements; ...; value; }) is used by bcc rewriter as well.
>>
>> Wait, you can stick arbitrary code inside a variable initialisation
>> block like this? How does that work? Is everything before the (struct
>> type) just ignored (and is that a cast)?
>
> Well, you definitely can still arbitrary code into a ({ }) expression
> block, that's not that surprising.
> The surprising bit that I discovered just recently was that stuff like
> this compiles and works correctly, try it:
>
> void *x = &x;
> printf("%lx == %lx\n", x, &x);
'void *x' just takes the address of the 'x' in the current scope.
It may looks like a use before define. but it actually works.
LGTM.
Acked-by: Yonghong Song <yhs@fb.com>
>
> So I'm using the fact that variable address is available inside
> variable initialization block.
>
> Beyond that, it's just a fancy, but standard (struct bla){ ...
> initializer list ...} syntax (it's not a struct initializer syntax,
> mind you, it's a struct assignment from struct literal). Fancy for
> sure, but it works and solves problems I mentioned in commit
> description.
>
>>
>> -Toke
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration
2019-10-21 16:57 [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration Andrii Nakryiko
2019-10-21 17:18 ` Toke Høiland-Jørgensen
@ 2019-10-21 19:01 ` Jakub Sitnicki
2019-10-21 23:51 ` Andrii Nakryiko
1 sibling, 1 reply; 7+ messages in thread
From: Jakub Sitnicki @ 2019-10-21 19:01 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team
On Mon, Oct 21, 2019 at 06:57 PM CEST, Andrii Nakryiko wrote:
> LIBBPF_OPTS is implemented as a mix of field declaration and memset
> + assignment. This makes it neither variable declaration nor purely
> statements, which is a problem, because you can't mix it with either
> other variable declarations nor other function statements, because C90
> compiler mode emits warning on mixing all that together.
>
> This patch changes LIBBPF_OPTS into a strictly declaration of variable
> and solves this problem, as can be seen in case of bpftool, which
> previously would emit compiler warning, if done this way (LIBBPF_OPTS as
> part of function variables declaration block).
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
Just a suggestion - macro helpers like this usually have DECLARE in
their name. At least in the kernel. For instance DECLARE_COMPLETION.
-Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration
2019-10-21 19:01 ` Jakub Sitnicki
@ 2019-10-21 23:51 ` Andrii Nakryiko
2019-10-22 7:27 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2019-10-21 23:51 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
On Mon, Oct 21, 2019 at 12:01 PM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Mon, Oct 21, 2019 at 06:57 PM CEST, Andrii Nakryiko wrote:
> > LIBBPF_OPTS is implemented as a mix of field declaration and memset
> > + assignment. This makes it neither variable declaration nor purely
> > statements, which is a problem, because you can't mix it with either
> > other variable declarations nor other function statements, because C90
> > compiler mode emits warning on mixing all that together.
> >
> > This patch changes LIBBPF_OPTS into a strictly declaration of variable
> > and solves this problem, as can be seen in case of bpftool, which
> > previously would emit compiler warning, if done this way (LIBBPF_OPTS as
> > part of function variables declaration block).
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
>
> Just a suggestion - macro helpers like this usually have DECLARE in
> their name. At least in the kernel. For instance DECLARE_COMPLETION.
Yes, it makes sense. This will cause some extra code churn, but it's
not too late. Will rename in v2 and fix current usages.
>
> -Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration
2019-10-21 23:51 ` Andrii Nakryiko
@ 2019-10-22 7:27 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 7+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-22 7:27 UTC (permalink / raw)
To: Andrii Nakryiko, Jakub Sitnicki
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Mon, Oct 21, 2019 at 12:01 PM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Mon, Oct 21, 2019 at 06:57 PM CEST, Andrii Nakryiko wrote:
>> > LIBBPF_OPTS is implemented as a mix of field declaration and memset
>> > + assignment. This makes it neither variable declaration nor purely
>> > statements, which is a problem, because you can't mix it with either
>> > other variable declarations nor other function statements, because C90
>> > compiler mode emits warning on mixing all that together.
>> >
>> > This patch changes LIBBPF_OPTS into a strictly declaration of variable
>> > and solves this problem, as can be seen in case of bpftool, which
>> > previously would emit compiler warning, if done this way (LIBBPF_OPTS as
>> > part of function variables declaration block).
>> >
>> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>> > ---
>>
>> Just a suggestion - macro helpers like this usually have DECLARE in
>> their name. At least in the kernel. For instance DECLARE_COMPLETION.
>
> Yes, it makes sense. This will cause some extra code churn, but it's
> not too late. Will rename in v2 and fix current usages.
While you're respinning, maybe add a comment explaining what it is
you're doing? It certainly broke the C parser in my head, so maybe a
hint would be good for others as well :)
-Toke
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-22 7:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 16:57 [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration Andrii Nakryiko
2019-10-21 17:18 ` Toke Høiland-Jørgensen
2019-10-21 17:38 ` Andrii Nakryiko
2019-10-21 18:19 ` Yonghong Song
2019-10-21 19:01 ` Jakub Sitnicki
2019-10-21 23:51 ` Andrii Nakryiko
2019-10-22 7:27 ` Toke Høiland-Jørgensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).