[PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[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

Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[Toke Høiland-Jørgensen]

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

Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[Yonghong Song]

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
>>

Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[Jakub Sitnicki]

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

Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[Andrii Nakryiko]

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

Re: [PATCH bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration

[Toke Høiland-Jørgensen]

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