From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: James Hilliard <james.hilliard1@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>
Subject: Re: [PATCH 1/1] libbpf: fix broken gcc pragma macros in bpf_helpers.h/bpf_tracing.h
Date: Tue, 7 Jun 2022 16:21:29 -0700 [thread overview]
Message-ID: <CAEf4BzaH0ZzEabjq43eZ4pZ7=SufYswffHwi1POn93Ty9SPJ6Q@mail.gmail.com> (raw)
In-Reply-To: <CADvTj4o2cbCpC40487=rgzSJZ8i94U4RR3=_s8ANE=phPQA6VQ@mail.gmail.com>
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
> > >
next prev parent reply other threads:[~2022-06-08 0:23 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2022-06-08 7:13 ` James Hilliard
2022-06-09 6:32 ` James Hilliard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEf4BzaH0ZzEabjq43eZ4pZ7=SufYswffHwi1POn93Ty9SPJ6Q@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=james.hilliard1@gmail.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).