* [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
@ 2022-06-06 13:27 James Hilliard
2022-06-06 18:02 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: James Hilliard @ 2022-06-06 13:27 UTC (permalink / raw)
To: bpf
Cc: James Hilliard, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh,
open list:BPF (Safe dynamic programs and tools),
open list
It seems the gcc preprocessor breaks unless pragmas are wrapped
individually inside macros.
Fixes errors like:
error: expected identifier or '(' before '#pragma'
106 | SEC("cgroup/bind6")
| ^~~
error: expected '=', ',', ';', 'asm' or '__attribute__' before '#pragma'
114 | char _license[] SEC("license") = "GPL";
| ^~~
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
tools/lib/bpf/bpf_helpers.h | 26 ++++++++++++++------------
tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++------------
2 files changed, 28 insertions(+), 24 deletions(-)
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index fb04eaf367f1..6d159082727d 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -22,11 +22,13 @@
* To allow use of SEC() with externs (e.g., for extern .maps declarations),
* make sure __attribute__((unused)) doesn't trigger compilation warning.
*/
+#define __gcc_helpers_pragma(x) _Pragma(#x)
+#define __gcc_helpers_diag_pragma(x) __gcc_helpers_pragma("GCC diagnostic " #x)
#define SEC(name) \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"") \
+ __gcc_helpers_diag_pragma(push) \
+ __gcc_helpers_diag_pragma(ignored "-Wignored-attributes") \
__attribute__((section(name), used)) \
- _Pragma("GCC diagnostic pop") \
+ __gcc_helpers_diag_pragma(pop)
/* Avoid 'linux/stddef.h' definition of '__always_inline'. */
#undef __always_inline
@@ -215,10 +217,10 @@ enum libbpf_tristate {
static const char ___fmt[] = fmt; \
unsigned long long ___param[___bpf_narg(args)]; \
\
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ __gcc_helpers_diag_pragma(push) \
+ __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
___bpf_fill(___param, args); \
- _Pragma("GCC diagnostic pop") \
+ __gcc_helpers_diag_pragma(pop) \
\
bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
___param, sizeof(___param)); \
@@ -233,10 +235,10 @@ enum libbpf_tristate {
static const char ___fmt[] = fmt; \
unsigned long long ___param[___bpf_narg(args)]; \
\
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ __gcc_helpers_diag_pragma(push) \
+ __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
___bpf_fill(___param, args); \
- _Pragma("GCC diagnostic pop") \
+ __gcc_helpers_diag_pragma(pop) \
\
bpf_snprintf(out, out_size, ___fmt, \
___param, sizeof(___param)); \
@@ -264,10 +266,10 @@ enum libbpf_tristate {
static const char ___fmt[] = fmt; \
unsigned long long ___param[___bpf_narg(args)]; \
\
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ __gcc_helpers_diag_pragma(push) \
+ __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
___bpf_fill(___param, args); \
- _Pragma("GCC diagnostic pop") \
+ __gcc_helpers_diag_pragma(pop) \
\
bpf_trace_vprintk(___fmt, sizeof(___fmt), \
___param, sizeof(___param)); \
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 01ce121c302d..e08ffc290b3e 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -422,16 +422,18 @@ struct pt_regs;
* This is useful when using BPF helpers that expect original context
* as one of the parameters (e.g., for bpf_perf_event_output()).
*/
+#define __gcc_tracing_pragma(x) _Pragma(#x)
+#define __gcc_tracing_diag_pragma(x) __gcc_tracing_pragma("GCC diagnostic " #x)
#define BPF_PROG(name, args...) \
name(unsigned long long *ctx); \
static __attribute__((always_inline)) typeof(name(0)) \
____##name(unsigned long long *ctx, ##args); \
typeof(name(0)) name(unsigned long long *ctx) \
{ \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ __gcc_tracing_diag_pragma(push) \
+ __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
return ____##name(___bpf_ctx_cast(args)); \
- _Pragma("GCC diagnostic pop") \
+ __gcc_tracing_diag_pragma(pop) \
} \
static __attribute__((always_inline)) typeof(name(0)) \
____##name(unsigned long long *ctx, ##args)
@@ -462,10 +464,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
____##name(struct pt_regs *ctx, ##args); \
typeof(name(0)) name(struct pt_regs *ctx) \
{ \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ __gcc_tracing_diag_pragma(push) \
+ __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
return ____##name(___bpf_kprobe_args(args)); \
- _Pragma("GCC diagnostic pop") \
+ __gcc_tracing_diag_pragma(pop) \
} \
static __attribute__((always_inline)) typeof(name(0)) \
____##name(struct pt_regs *ctx, ##args)
@@ -486,10 +488,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
____##name(struct pt_regs *ctx, ##args); \
typeof(name(0)) name(struct pt_regs *ctx) \
{ \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ __gcc_tracing_diag_pragma(push) \
+ __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
return ____##name(___bpf_kretprobe_args(args)); \
- _Pragma("GCC diagnostic pop") \
+ __gcc_tracing_diag_pragma(pop) \
} \
static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
@@ -520,10 +522,10 @@ ____##name(struct pt_regs *ctx, ##args); \
typeof(name(0)) name(struct pt_regs *ctx) \
{ \
struct pt_regs *regs = PT_REGS_SYSCALL_REGS(ctx); \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ __gcc_tracing_diag_pragma(push) \
+ __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
return ____##name(___bpf_syscall_args(args)); \
- _Pragma("GCC diagnostic pop") \
+ __gcc_tracing_diag_pragma(pop) \
} \
static __attribute__((always_inline)) typeof(name(0)) \
____##name(struct pt_regs *ctx, ##args)
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
2022-06-06 13:27 [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h James Hilliard
@ 2022-06-06 18:02 ` Andrii Nakryiko
2022-06-06 21:20 ` James Hilliard
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2022-06-06 18:02 UTC (permalink / raw)
To: James Hilliard
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, open list:BPF (Safe dynamic programs and tools),
open list
On Mon, Jun 6, 2022 at 6:28 AM James Hilliard <james.hilliard1@gmail.com> wrote:
>
> It seems the gcc preprocessor breaks unless pragmas are wrapped
> individually inside macros.
>
> Fixes errors like:
> error: expected identifier or '(' before '#pragma'
> 106 | SEC("cgroup/bind6")
> | ^~~
>
> error: expected '=', ',', ';', 'asm' or '__attribute__' before '#pragma'
> 114 | char _license[] SEC("license") = "GPL";
> | ^~~
>
We've been using this macro in this form for a while with no errors.
How do you get these errors in the first place? _Pragma is supposed to
be a full equivalent of #pragma specifically to be able to be used in
macros, so these work-arounds shouldn't be necessary. Let's first try
to root cause this.
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
> tools/lib/bpf/bpf_helpers.h | 26 ++++++++++++++------------
> tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++------------
> 2 files changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index fb04eaf367f1..6d159082727d 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -22,11 +22,13 @@
> * To allow use of SEC() with externs (e.g., for extern .maps declarations),
> * make sure __attribute__((unused)) doesn't trigger compilation warning.
> */
> +#define __gcc_helpers_pragma(x) _Pragma(#x)
> +#define __gcc_helpers_diag_pragma(x) __gcc_helpers_pragma("GCC diagnostic " #x)
> #define SEC(name) \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"") \
> + __gcc_helpers_diag_pragma(push) \
> + __gcc_helpers_diag_pragma(ignored "-Wignored-attributes") \
> __attribute__((section(name), used)) \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_helpers_diag_pragma(pop)
>
> /* Avoid 'linux/stddef.h' definition of '__always_inline'. */
> #undef __always_inline
> @@ -215,10 +217,10 @@ enum libbpf_tristate {
> static const char ___fmt[] = fmt; \
> unsigned long long ___param[___bpf_narg(args)]; \
> \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + __gcc_helpers_diag_pragma(push) \
> + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> ___bpf_fill(___param, args); \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_helpers_diag_pragma(pop) \
> \
> bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> ___param, sizeof(___param)); \
> @@ -233,10 +235,10 @@ enum libbpf_tristate {
> static const char ___fmt[] = fmt; \
> unsigned long long ___param[___bpf_narg(args)]; \
> \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + __gcc_helpers_diag_pragma(push) \
> + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> ___bpf_fill(___param, args); \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_helpers_diag_pragma(pop) \
> \
> bpf_snprintf(out, out_size, ___fmt, \
> ___param, sizeof(___param)); \
> @@ -264,10 +266,10 @@ enum libbpf_tristate {
> static const char ___fmt[] = fmt; \
> unsigned long long ___param[___bpf_narg(args)]; \
> \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + __gcc_helpers_diag_pragma(push) \
> + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> ___bpf_fill(___param, args); \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_helpers_diag_pragma(pop) \
> \
> bpf_trace_vprintk(___fmt, sizeof(___fmt), \
> ___param, sizeof(___param)); \
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index 01ce121c302d..e08ffc290b3e 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -422,16 +422,18 @@ struct pt_regs;
> * This is useful when using BPF helpers that expect original context
> * as one of the parameters (e.g., for bpf_perf_event_output()).
> */
> +#define __gcc_tracing_pragma(x) _Pragma(#x)
> +#define __gcc_tracing_diag_pragma(x) __gcc_tracing_pragma("GCC diagnostic " #x)
> #define BPF_PROG(name, args...) \
> name(unsigned long long *ctx); \
> static __attribute__((always_inline)) typeof(name(0)) \
> ____##name(unsigned long long *ctx, ##args); \
> typeof(name(0)) name(unsigned long long *ctx) \
> { \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + __gcc_tracing_diag_pragma(push) \
> + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> return ____##name(___bpf_ctx_cast(args)); \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_tracing_diag_pragma(pop) \
> } \
> static __attribute__((always_inline)) typeof(name(0)) \
> ____##name(unsigned long long *ctx, ##args)
> @@ -462,10 +464,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> ____##name(struct pt_regs *ctx, ##args); \
> typeof(name(0)) name(struct pt_regs *ctx) \
> { \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + __gcc_tracing_diag_pragma(push) \
> + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> return ____##name(___bpf_kprobe_args(args)); \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_tracing_diag_pragma(pop) \
> } \
> static __attribute__((always_inline)) typeof(name(0)) \
> ____##name(struct pt_regs *ctx, ##args)
> @@ -486,10 +488,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> ____##name(struct pt_regs *ctx, ##args); \
> typeof(name(0)) name(struct pt_regs *ctx) \
> { \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + __gcc_tracing_diag_pragma(push) \
> + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> return ____##name(___bpf_kretprobe_args(args)); \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_tracing_diag_pragma(pop) \
> } \
> static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>
> @@ -520,10 +522,10 @@ ____##name(struct pt_regs *ctx, ##args); \
> typeof(name(0)) name(struct pt_regs *ctx) \
> { \
> struct pt_regs *regs = PT_REGS_SYSCALL_REGS(ctx); \
> - _Pragma("GCC diagnostic push") \
> - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + __gcc_tracing_diag_pragma(push) \
> + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> return ____##name(___bpf_syscall_args(args)); \
> - _Pragma("GCC diagnostic pop") \
> + __gcc_tracing_diag_pragma(pop) \
> } \
> static __attribute__((always_inline)) typeof(name(0)) \
> ____##name(struct pt_regs *ctx, ##args)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
2022-06-06 18:02 ` Andrii Nakryiko
@ 2022-06-06 21:20 ` James Hilliard
2022-06-07 23:21 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: James Hilliard @ 2022-06-06 21:20 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, open list:BPF (Safe dynamic programs and tools),
open list
On Mon, Jun 6, 2022 at 12:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jun 6, 2022 at 6:28 AM James Hilliard <james.hilliard1@gmail.com> wrote:
> >
> > It seems the gcc preprocessor breaks unless pragmas are wrapped
> > individually inside macros.
> >
> > Fixes errors like:
> > error: expected identifier or '(' before '#pragma'
> > 106 | SEC("cgroup/bind6")
> > | ^~~
> >
> > error: expected '=', ',', ';', 'asm' or '__attribute__' before '#pragma'
> > 114 | char _license[] SEC("license") = "GPL";
> > | ^~~
> >
>
> We've been using this macro in this form for a while with no errors.
> How do you get these errors in the first place?
I was attempting to compile the systemd bpf programs using gcc 12.1.
https://github.com/systemd/systemd/tree/main/src/core/bpf
> _Pragma is supposed to
> be a full equivalent of #pragma specifically to be able to be used in
> macros, so these work-arounds shouldn't be necessary.
I did try and style this like the nested macro example here:
https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
> Let's first try
> to root cause this.
I was looking around and it seems there's a bunch of gcc preprocessor
pragma issues in general, restyling this seemed to be the best option
at the moment since a lot looked to be unfixed:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55578
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89718
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91669
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> > tools/lib/bpf/bpf_helpers.h | 26 ++++++++++++++------------
> > tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++------------
> > 2 files changed, 28 insertions(+), 24 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index fb04eaf367f1..6d159082727d 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -22,11 +22,13 @@
> > * To allow use of SEC() with externs (e.g., for extern .maps declarations),
> > * make sure __attribute__((unused)) doesn't trigger compilation warning.
> > */
> > +#define __gcc_helpers_pragma(x) _Pragma(#x)
> > +#define __gcc_helpers_diag_pragma(x) __gcc_helpers_pragma("GCC diagnostic " #x)
> > #define SEC(name) \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"") \
> > + __gcc_helpers_diag_pragma(push) \
> > + __gcc_helpers_diag_pragma(ignored "-Wignored-attributes") \
> > __attribute__((section(name), used)) \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_helpers_diag_pragma(pop)
> >
> > /* Avoid 'linux/stddef.h' definition of '__always_inline'. */
> > #undef __always_inline
> > @@ -215,10 +217,10 @@ enum libbpf_tristate {
> > static const char ___fmt[] = fmt; \
> > unsigned long long ___param[___bpf_narg(args)]; \
> > \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + __gcc_helpers_diag_pragma(push) \
> > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > ___bpf_fill(___param, args); \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_helpers_diag_pragma(pop) \
> > \
> > bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> > ___param, sizeof(___param)); \
> > @@ -233,10 +235,10 @@ enum libbpf_tristate {
> > static const char ___fmt[] = fmt; \
> > unsigned long long ___param[___bpf_narg(args)]; \
> > \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + __gcc_helpers_diag_pragma(push) \
> > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > ___bpf_fill(___param, args); \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_helpers_diag_pragma(pop) \
> > \
> > bpf_snprintf(out, out_size, ___fmt, \
> > ___param, sizeof(___param)); \
> > @@ -264,10 +266,10 @@ enum libbpf_tristate {
> > static const char ___fmt[] = fmt; \
> > unsigned long long ___param[___bpf_narg(args)]; \
> > \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + __gcc_helpers_diag_pragma(push) \
> > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > ___bpf_fill(___param, args); \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_helpers_diag_pragma(pop) \
> > \
> > bpf_trace_vprintk(___fmt, sizeof(___fmt), \
> > ___param, sizeof(___param)); \
> > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> > index 01ce121c302d..e08ffc290b3e 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -422,16 +422,18 @@ struct pt_regs;
> > * This is useful when using BPF helpers that expect original context
> > * as one of the parameters (e.g., for bpf_perf_event_output()).
> > */
> > +#define __gcc_tracing_pragma(x) _Pragma(#x)
> > +#define __gcc_tracing_diag_pragma(x) __gcc_tracing_pragma("GCC diagnostic " #x)
> > #define BPF_PROG(name, args...) \
> > name(unsigned long long *ctx); \
> > static __attribute__((always_inline)) typeof(name(0)) \
> > ____##name(unsigned long long *ctx, ##args); \
> > typeof(name(0)) name(unsigned long long *ctx) \
> > { \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + __gcc_tracing_diag_pragma(push) \
> > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > return ____##name(___bpf_ctx_cast(args)); \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_tracing_diag_pragma(pop) \
> > } \
> > static __attribute__((always_inline)) typeof(name(0)) \
> > ____##name(unsigned long long *ctx, ##args)
> > @@ -462,10 +464,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > ____##name(struct pt_regs *ctx, ##args); \
> > typeof(name(0)) name(struct pt_regs *ctx) \
> > { \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + __gcc_tracing_diag_pragma(push) \
> > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > return ____##name(___bpf_kprobe_args(args)); \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_tracing_diag_pragma(pop) \
> > } \
> > static __attribute__((always_inline)) typeof(name(0)) \
> > ____##name(struct pt_regs *ctx, ##args)
> > @@ -486,10 +488,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > ____##name(struct pt_regs *ctx, ##args); \
> > typeof(name(0)) name(struct pt_regs *ctx) \
> > { \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + __gcc_tracing_diag_pragma(push) \
> > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > return ____##name(___bpf_kretprobe_args(args)); \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_tracing_diag_pragma(pop) \
> > } \
> > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> >
> > @@ -520,10 +522,10 @@ ____##name(struct pt_regs *ctx, ##args); \
> > typeof(name(0)) name(struct pt_regs *ctx) \
> > { \
> > struct pt_regs *regs = PT_REGS_SYSCALL_REGS(ctx); \
> > - _Pragma("GCC diagnostic push") \
> > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + __gcc_tracing_diag_pragma(push) \
> > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > return ____##name(___bpf_syscall_args(args)); \
> > - _Pragma("GCC diagnostic pop") \
> > + __gcc_tracing_diag_pragma(pop) \
> > } \
> > static __attribute__((always_inline)) typeof(name(0)) \
> > ____##name(struct pt_regs *ctx, ##args)
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
2022-06-06 21:20 ` James Hilliard
@ 2022-06-07 23:21 ` Andrii Nakryiko
2022-06-08 7:13 ` James Hilliard
2022-06-09 6:32 ` James Hilliard
0 siblings, 2 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2022-06-07 23:21 UTC (permalink / raw)
To: James Hilliard
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, open list:BPF (Safe dynamic programs and tools),
open list
On Mon, Jun 6, 2022 at 2:20 PM James Hilliard <james.hilliard1@gmail.com> wrote:
>
> On Mon, Jun 6, 2022 at 12:02 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jun 6, 2022 at 6:28 AM James Hilliard <james.hilliard1@gmail.com> wrote:
> > >
> > > It seems the gcc preprocessor breaks unless pragmas are wrapped
> > > individually inside macros.
> > >
> > > Fixes errors like:
> > > error: expected identifier or '(' before '#pragma'
> > > 106 | SEC("cgroup/bind6")
> > > | ^~~
> > >
> > > error: expected '=', ',', ';', 'asm' or '__attribute__' before '#pragma'
> > > 114 | char _license[] SEC("license") = "GPL";
> > > | ^~~
> > >
> >
> > We've been using this macro in this form for a while with no errors.
> > How do you get these errors in the first place?
>
> I was attempting to compile the systemd bpf programs using gcc 12.1.
> https://github.com/systemd/systemd/tree/main/src/core/bpf
It would be great to be able to repro it as part of selftests. Can you
try gcc 12 with selftests/bpf and see if you get the same problem?
>
> > _Pragma is supposed to
> > be a full equivalent of #pragma specifically to be able to be used in
> > macros, so these work-arounds shouldn't be necessary.
>
> I did try and style this like the nested macro example here:
> https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
If you are referring to DO_PRAGMA example? That example is done that
way to do argument stringification, but not because _Pragma can't be
used as is in macros.
>
> > Let's first try
> > to root cause this.
>
> I was looking around and it seems there's a bunch of gcc preprocessor
> pragma issues in general, restyling this seemed to be the best option
> at the moment since a lot looked to be unfixed:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55578
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89718
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91669
>
I don't like the obscurity of the changes in this patch and don't see
how it fundamentally changes anything. So I'd like to actually try to
be able to repro it and see what other solutions there are before
committing to this.
I also suspect that it's only the SEC() macro that's problematic and
we shouldn't touch any other macro at all. But again, I'd like to get
a repro first.
> >
> > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > > ---
> > > tools/lib/bpf/bpf_helpers.h | 26 ++++++++++++++------------
> > > tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++------------
> > > 2 files changed, 28 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > > index fb04eaf367f1..6d159082727d 100644
> > > --- a/tools/lib/bpf/bpf_helpers.h
> > > +++ b/tools/lib/bpf/bpf_helpers.h
> > > @@ -22,11 +22,13 @@
> > > * To allow use of SEC() with externs (e.g., for extern .maps declarations),
> > > * make sure __attribute__((unused)) doesn't trigger compilation warning.
> > > */
> > > +#define __gcc_helpers_pragma(x) _Pragma(#x)
> > > +#define __gcc_helpers_diag_pragma(x) __gcc_helpers_pragma("GCC diagnostic " #x)
> > > #define SEC(name) \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"") \
> > > + __gcc_helpers_diag_pragma(push) \
> > > + __gcc_helpers_diag_pragma(ignored "-Wignored-attributes") \
> > > __attribute__((section(name), used)) \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_helpers_diag_pragma(pop)
> > >
> > > /* Avoid 'linux/stddef.h' definition of '__always_inline'. */
> > > #undef __always_inline
> > > @@ -215,10 +217,10 @@ enum libbpf_tristate {
> > > static const char ___fmt[] = fmt; \
> > > unsigned long long ___param[___bpf_narg(args)]; \
> > > \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > + __gcc_helpers_diag_pragma(push) \
> > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > ___bpf_fill(___param, args); \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_helpers_diag_pragma(pop) \
> > > \
> > > bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> > > ___param, sizeof(___param)); \
> > > @@ -233,10 +235,10 @@ enum libbpf_tristate {
> > > static const char ___fmt[] = fmt; \
> > > unsigned long long ___param[___bpf_narg(args)]; \
> > > \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > + __gcc_helpers_diag_pragma(push) \
> > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > ___bpf_fill(___param, args); \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_helpers_diag_pragma(pop) \
> > > \
> > > bpf_snprintf(out, out_size, ___fmt, \
> > > ___param, sizeof(___param)); \
> > > @@ -264,10 +266,10 @@ enum libbpf_tristate {
> > > static const char ___fmt[] = fmt; \
> > > unsigned long long ___param[___bpf_narg(args)]; \
> > > \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > + __gcc_helpers_diag_pragma(push) \
> > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > ___bpf_fill(___param, args); \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_helpers_diag_pragma(pop) \
> > > \
> > > bpf_trace_vprintk(___fmt, sizeof(___fmt), \
> > > ___param, sizeof(___param)); \
> > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> > > index 01ce121c302d..e08ffc290b3e 100644
> > > --- a/tools/lib/bpf/bpf_tracing.h
> > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > @@ -422,16 +422,18 @@ struct pt_regs;
> > > * This is useful when using BPF helpers that expect original context
> > > * as one of the parameters (e.g., for bpf_perf_event_output()).
> > > */
> > > +#define __gcc_tracing_pragma(x) _Pragma(#x)
> > > +#define __gcc_tracing_diag_pragma(x) __gcc_tracing_pragma("GCC diagnostic " #x)
> > > #define BPF_PROG(name, args...) \
> > > name(unsigned long long *ctx); \
> > > static __attribute__((always_inline)) typeof(name(0)) \
> > > ____##name(unsigned long long *ctx, ##args); \
> > > typeof(name(0)) name(unsigned long long *ctx) \
> > > { \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > + __gcc_tracing_diag_pragma(push) \
> > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > return ____##name(___bpf_ctx_cast(args)); \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_tracing_diag_pragma(pop) \
> > > } \
> > > static __attribute__((always_inline)) typeof(name(0)) \
> > > ____##name(unsigned long long *ctx, ##args)
> > > @@ -462,10 +464,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > > ____##name(struct pt_regs *ctx, ##args); \
> > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > { \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > + __gcc_tracing_diag_pragma(push) \
> > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > return ____##name(___bpf_kprobe_args(args)); \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_tracing_diag_pragma(pop) \
> > > } \
> > > static __attribute__((always_inline)) typeof(name(0)) \
> > > ____##name(struct pt_regs *ctx, ##args)
> > > @@ -486,10 +488,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > > ____##name(struct pt_regs *ctx, ##args); \
> > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > { \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > + __gcc_tracing_diag_pragma(push) \
> > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > return ____##name(___bpf_kretprobe_args(args)); \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_tracing_diag_pragma(pop) \
> > > } \
> > > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> > >
> > > @@ -520,10 +522,10 @@ ____##name(struct pt_regs *ctx, ##args); \
> > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > { \
> > > struct pt_regs *regs = PT_REGS_SYSCALL_REGS(ctx); \
> > > - _Pragma("GCC diagnostic push") \
> > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > + __gcc_tracing_diag_pragma(push) \
> > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > return ____##name(___bpf_syscall_args(args)); \
> > > - _Pragma("GCC diagnostic pop") \
> > > + __gcc_tracing_diag_pragma(pop) \
> > > } \
> > > static __attribute__((always_inline)) typeof(name(0)) \
> > > ____##name(struct pt_regs *ctx, ##args)
> > > --
> > > 2.25.1
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
2022-06-07 23:21 ` Andrii Nakryiko
@ 2022-06-08 7:13 ` James Hilliard
2022-06-09 6:32 ` James Hilliard
1 sibling, 0 replies; 6+ messages in thread
From: James Hilliard @ 2022-06-08 7:13 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, open list:BPF (Safe dynamic programs and tools),
open list
On Tue, Jun 7, 2022 at 5:21 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jun 6, 2022 at 2:20 PM James Hilliard <james.hilliard1@gmail.com> wrote:
> >
> > On Mon, Jun 6, 2022 at 12:02 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Jun 6, 2022 at 6:28 AM James Hilliard <james.hilliard1@gmail.com> wrote:
> > > >
> > > > It seems the gcc preprocessor breaks unless pragmas are wrapped
> > > > individually inside macros.
> > > >
> > > > Fixes errors like:
> > > > error: expected identifier or '(' before '#pragma'
> > > > 106 | SEC("cgroup/bind6")
> > > > | ^~~
> > > >
> > > > error: expected '=', ',', ';', 'asm' or '__attribute__' before '#pragma'
> > > > 114 | char _license[] SEC("license") = "GPL";
> > > > | ^~~
> > > >
> > >
> > > We've been using this macro in this form for a while with no errors.
> > > How do you get these errors in the first place?
> >
> > I was attempting to compile the systemd bpf programs using gcc 12.1.
> > https://github.com/systemd/systemd/tree/main/src/core/bpf
>
> It would be great to be able to repro it as part of selftests. Can you
> try gcc 12 with selftests/bpf and see if you get the same problem?
>
> >
> > > _Pragma is supposed to
> > > be a full equivalent of #pragma specifically to be able to be used in
> > > macros, so these work-arounds shouldn't be necessary.
> >
> > I did try and style this like the nested macro example here:
> > https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
>
>
> If you are referring to DO_PRAGMA example? That example is done that
> way to do argument stringification, but not because _Pragma can't be
> used as is in macros.
>
> >
> > > Let's first try
> > > to root cause this.
> >
> > I was looking around and it seems there's a bunch of gcc preprocessor
> > pragma issues in general, restyling this seemed to be the best option
> > at the moment since a lot looked to be unfixed:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55578
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89718
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91669
> >
>
> I don't like the obscurity of the changes in this patch and don't see
> how it fundamentally changes anything. So I'd like to actually try to
> be able to repro it and see what other solutions there are before
> committing to this.
>
> I also suspect that it's only the SEC() macro that's problematic and
> we shouldn't touch any other macro at all. But again, I'd like to get
> a repro first.
Seems typeof was triggering the issue, replacing it with __typeof__ seems
to fix it:
https://lore.kernel.org/bpf/20220608064004.1493239-1-james.hilliard1@gmail.com/
Seems similar to this issue:
https://patches.dpdk.org/project/dpdk/patch/2601191342CEEE43887BDE71AB977258213F3012@irsmsx105.ger.corp.intel.com/
Looks like gcc and llvm document there's some differences between __typeof__
and typeof with __typeof__ being the more portable variant:
https://gcc.gnu.org/onlinedocs/gcc/Typeof.html
https://clang.llvm.org/docs/UsersManual.html#differences-between-various-standard-modes
>
> > >
> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > > > ---
> > > > tools/lib/bpf/bpf_helpers.h | 26 ++++++++++++++------------
> > > > tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++------------
> > > > 2 files changed, 28 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > > > index fb04eaf367f1..6d159082727d 100644
> > > > --- a/tools/lib/bpf/bpf_helpers.h
> > > > +++ b/tools/lib/bpf/bpf_helpers.h
> > > > @@ -22,11 +22,13 @@
> > > > * To allow use of SEC() with externs (e.g., for extern .maps declarations),
> > > > * make sure __attribute__((unused)) doesn't trigger compilation warning.
> > > > */
> > > > +#define __gcc_helpers_pragma(x) _Pragma(#x)
> > > > +#define __gcc_helpers_diag_pragma(x) __gcc_helpers_pragma("GCC diagnostic " #x)
> > > > #define SEC(name) \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wignored-attributes") \
> > > > __attribute__((section(name), used)) \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop)
> > > >
> > > > /* Avoid 'linux/stddef.h' definition of '__always_inline'. */
> > > > #undef __always_inline
> > > > @@ -215,10 +217,10 @@ enum libbpf_tristate {
> > > > static const char ___fmt[] = fmt; \
> > > > unsigned long long ___param[___bpf_narg(args)]; \
> > > > \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > > ___bpf_fill(___param, args); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop) \
> > > > \
> > > > bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> > > > ___param, sizeof(___param)); \
> > > > @@ -233,10 +235,10 @@ enum libbpf_tristate {
> > > > static const char ___fmt[] = fmt; \
> > > > unsigned long long ___param[___bpf_narg(args)]; \
> > > > \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > > ___bpf_fill(___param, args); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop) \
> > > > \
> > > > bpf_snprintf(out, out_size, ___fmt, \
> > > > ___param, sizeof(___param)); \
> > > > @@ -264,10 +266,10 @@ enum libbpf_tristate {
> > > > static const char ___fmt[] = fmt; \
> > > > unsigned long long ___param[___bpf_narg(args)]; \
> > > > \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > > ___bpf_fill(___param, args); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop) \
> > > > \
> > > > bpf_trace_vprintk(___fmt, sizeof(___fmt), \
> > > > ___param, sizeof(___param)); \
> > > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> > > > index 01ce121c302d..e08ffc290b3e 100644
> > > > --- a/tools/lib/bpf/bpf_tracing.h
> > > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > > @@ -422,16 +422,18 @@ struct pt_regs;
> > > > * This is useful when using BPF helpers that expect original context
> > > > * as one of the parameters (e.g., for bpf_perf_event_output()).
> > > > */
> > > > +#define __gcc_tracing_pragma(x) _Pragma(#x)
> > > > +#define __gcc_tracing_diag_pragma(x) __gcc_tracing_pragma("GCC diagnostic " #x)
> > > > #define BPF_PROG(name, args...) \
> > > > name(unsigned long long *ctx); \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(unsigned long long *ctx, ##args); \
> > > > typeof(name(0)) name(unsigned long long *ctx) \
> > > > { \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_ctx_cast(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(unsigned long long *ctx, ##args)
> > > > @@ -462,10 +464,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args); \
> > > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > > { \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_kprobe_args(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args)
> > > > @@ -486,10 +488,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args); \
> > > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > > { \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_kretprobe_args(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> > > >
> > > > @@ -520,10 +522,10 @@ ____##name(struct pt_regs *ctx, ##args); \
> > > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > > { \
> > > > struct pt_regs *regs = PT_REGS_SYSCALL_REGS(ctx); \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_syscall_args(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args)
> > > > --
> > > > 2.25.1
> > > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
2022-06-07 23:21 ` Andrii Nakryiko
2022-06-08 7:13 ` James Hilliard
@ 2022-06-09 6:32 ` James Hilliard
1 sibling, 0 replies; 6+ messages in thread
From: James Hilliard @ 2022-06-09 6:32 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, open list:BPF (Safe dynamic programs and tools),
open list
On Tue, Jun 7, 2022 at 5:21 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jun 6, 2022 at 2:20 PM James Hilliard <james.hilliard1@gmail.com> wrote:
> >
> > On Mon, Jun 6, 2022 at 12:02 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Jun 6, 2022 at 6:28 AM James Hilliard <james.hilliard1@gmail.com> wrote:
> > > >
> > > > It seems the gcc preprocessor breaks unless pragmas are wrapped
> > > > individually inside macros.
> > > >
> > > > Fixes errors like:
> > > > error: expected identifier or '(' before '#pragma'
> > > > 106 | SEC("cgroup/bind6")
> > > > | ^~~
> > > >
> > > > error: expected '=', ',', ';', 'asm' or '__attribute__' before '#pragma'
> > > > 114 | char _license[] SEC("license") = "GPL";
> > > > | ^~~
> > > >
> > >
> > > We've been using this macro in this form for a while with no errors.
> > > How do you get these errors in the first place?
> >
> > I was attempting to compile the systemd bpf programs using gcc 12.1.
> > https://github.com/systemd/systemd/tree/main/src/core/bpf
>
> It would be great to be able to repro it as part of selftests. Can you
> try gcc 12 with selftests/bpf and see if you get the same problem?
>
> >
> > > _Pragma is supposed to
> > > be a full equivalent of #pragma specifically to be able to be used in
> > > macros, so these work-arounds shouldn't be necessary.
> >
> > I did try and style this like the nested macro example here:
> > https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
>
>
> If you are referring to DO_PRAGMA example? That example is done that
> way to do argument stringification, but not because _Pragma can't be
> used as is in macros.
>
> >
> > > Let's first try
> > > to root cause this.
> >
> > I was looking around and it seems there's a bunch of gcc preprocessor
> > pragma issues in general, restyling this seemed to be the best option
> > at the moment since a lot looked to be unfixed:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55578
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89718
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91669
> >
>
> I don't like the obscurity of the changes in this patch and don't see
> how it fundamentally changes anything. So I'd like to actually try to
> be able to repro it and see what other solutions there are before
> committing to this.
>
> I also suspect that it's only the SEC() macro that's problematic and
> we shouldn't touch any other macro at all. But again, I'd like to get
> a repro first.
Ok, yeah looks like it's just the SEC() macro, resent with just that changed:
https://lore.kernel.org/bpf/20220609062412.3950380-1-james.hilliard1@gmail.com/
Seems there's a separate issue with -std=c17 and typeof():
https://lore.kernel.org/bpf/20220609062829.293217-1-james.hilliard1@gmail.com/
>
> > >
> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > > > ---
> > > > tools/lib/bpf/bpf_helpers.h | 26 ++++++++++++++------------
> > > > tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++------------
> > > > 2 files changed, 28 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > > > index fb04eaf367f1..6d159082727d 100644
> > > > --- a/tools/lib/bpf/bpf_helpers.h
> > > > +++ b/tools/lib/bpf/bpf_helpers.h
> > > > @@ -22,11 +22,13 @@
> > > > * To allow use of SEC() with externs (e.g., for extern .maps declarations),
> > > > * make sure __attribute__((unused)) doesn't trigger compilation warning.
> > > > */
> > > > +#define __gcc_helpers_pragma(x) _Pragma(#x)
> > > > +#define __gcc_helpers_diag_pragma(x) __gcc_helpers_pragma("GCC diagnostic " #x)
> > > > #define SEC(name) \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wignored-attributes") \
> > > > __attribute__((section(name), used)) \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop)
> > > >
> > > > /* Avoid 'linux/stddef.h' definition of '__always_inline'. */
> > > > #undef __always_inline
> > > > @@ -215,10 +217,10 @@ enum libbpf_tristate {
> > > > static const char ___fmt[] = fmt; \
> > > > unsigned long long ___param[___bpf_narg(args)]; \
> > > > \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > > ___bpf_fill(___param, args); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop) \
> > > > \
> > > > bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> > > > ___param, sizeof(___param)); \
> > > > @@ -233,10 +235,10 @@ enum libbpf_tristate {
> > > > static const char ___fmt[] = fmt; \
> > > > unsigned long long ___param[___bpf_narg(args)]; \
> > > > \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > > ___bpf_fill(___param, args); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop) \
> > > > \
> > > > bpf_snprintf(out, out_size, ___fmt, \
> > > > ___param, sizeof(___param)); \
> > > > @@ -264,10 +266,10 @@ enum libbpf_tristate {
> > > > static const char ___fmt[] = fmt; \
> > > > unsigned long long ___param[___bpf_narg(args)]; \
> > > > \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_helpers_diag_pragma(push) \
> > > > + __gcc_helpers_diag_pragma(ignored "-Wint-conversion") \
> > > > ___bpf_fill(___param, args); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_helpers_diag_pragma(pop) \
> > > > \
> > > > bpf_trace_vprintk(___fmt, sizeof(___fmt), \
> > > > ___param, sizeof(___param)); \
> > > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> > > > index 01ce121c302d..e08ffc290b3e 100644
> > > > --- a/tools/lib/bpf/bpf_tracing.h
> > > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > > @@ -422,16 +422,18 @@ struct pt_regs;
> > > > * This is useful when using BPF helpers that expect original context
> > > > * as one of the parameters (e.g., for bpf_perf_event_output()).
> > > > */
> > > > +#define __gcc_tracing_pragma(x) _Pragma(#x)
> > > > +#define __gcc_tracing_diag_pragma(x) __gcc_tracing_pragma("GCC diagnostic " #x)
> > > > #define BPF_PROG(name, args...) \
> > > > name(unsigned long long *ctx); \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(unsigned long long *ctx, ##args); \
> > > > typeof(name(0)) name(unsigned long long *ctx) \
> > > > { \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_ctx_cast(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(unsigned long long *ctx, ##args)
> > > > @@ -462,10 +464,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args); \
> > > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > > { \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_kprobe_args(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args)
> > > > @@ -486,10 +488,10 @@ static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args); \
> > > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > > { \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_kretprobe_args(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> > > >
> > > > @@ -520,10 +522,10 @@ ____##name(struct pt_regs *ctx, ##args); \
> > > > typeof(name(0)) name(struct pt_regs *ctx) \
> > > > { \
> > > > struct pt_regs *regs = PT_REGS_SYSCALL_REGS(ctx); \
> > > > - _Pragma("GCC diagnostic push") \
> > > > - _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > > > + __gcc_tracing_diag_pragma(push) \
> > > > + __gcc_tracing_diag_pragma(ignored "-Wint-conversion") \
> > > > return ____##name(___bpf_syscall_args(args)); \
> > > > - _Pragma("GCC diagnostic pop") \
> > > > + __gcc_tracing_diag_pragma(pop) \
> > > > } \
> > > > static __attribute__((always_inline)) typeof(name(0)) \
> > > > ____##name(struct pt_regs *ctx, ##args)
> > > > --
> > > > 2.25.1
> > > >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-09 6:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-06 13:27 [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h James Hilliard
2022-06-06 18:02 ` Andrii Nakryiko
2022-06-06 21:20 ` James Hilliard
2022-06-07 23:21 ` Andrii Nakryiko
2022-06-08 7:13 ` James Hilliard
2022-06-09 6:32 ` James Hilliard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.