On Tue, Jun 7, 2022 at 5:21 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Mon, Jun 6, 2022 at 2:20 PM James Hilliard <james.hilliard1@xxxxxxxxx> wrote:
> >
> > On Mon, Jun 6, 2022 at 12:02 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Jun 6, 2022 at 6:28 AM James Hilliard <james.hilliard1@xxxxxxxxx> 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@xxxxxxxxx/
Seems similar to this issue:
https://patches.dpdk.org/project/dpdk/patch/2601191342CEEE43887BDE71AB977258213F3012@xxxxxxxxxxxxxxxxxxxxxxxxxxxx/
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@xxxxxxxxx>
> > > > ---
> > > > 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
> > > >
