Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220606132741.3462925-1-james.hilliard1@gmail.com> <CAEf4BzZ8eTqVnsLqc52=AyHeAsuVB3Nv7uBW19t2pcb9h7p2hQ@mail.gmail.com>
In-Reply-To: <CAEf4BzZ8eTqVnsLqc52=AyHeAsuVB3Nv7uBW19t2pcb9h7p2hQ@mail.gmail.com>
From:   James Hilliard <james.hilliard1@gmail.com>
Date:   Mon, 6 Jun 2022 15:20:25 -0600
Message-ID: <CADvTj4o2cbCpC40487=rgzSJZ8i94U4RR3=_s8ANE=phPQA6VQ@mail.gmail.com>
Subject: Re: [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        "open list:BPF (Safe dynamic programs and tools)" 
        <netdev@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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