Message ID | 20191021165744.2116648-1-andriin@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next] libbpf: make LIBBPF_OPTS macro strictly a variable declaration | expand |
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
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 >
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 >>
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
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
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
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 */
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(-)