On Fri, Jul 1, 2022 at 10:12 AM James Hilliard
[off-list ref] wrote:

On Thu, Jun 30, 2022 at 3:51 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Mon, Jun 27, 2022 at 9:43 PM James Hilliard
[off-list ref] wrote:

quoted

On Mon, Jun 27, 2022 at 5:16 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Thu, Jun 9, 2022 at 4:27 PM James Hilliard [off-list ref] wrote:

quoted

On Thu, Jun 9, 2022 at 12:13 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Wed, Jun 8, 2022 at 11:24 PM James Hilliard
[off-list ref] wrote:

quoted

It seems the gcc preprocessor breaks unless pragmas are wrapped
individually inside macros when surrounding __attribute__.

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 <redacted>
---
Changes v2 -> v3:
  - just fix SEC pragma
Changes v1 -> v2:
  - replace typeof with __typeof__ instead of changing pragma macros
---
 tools/lib/bpf/bpf_helpers.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index fb04eaf367f1..66d23c47c206 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h

@@ -22,11 +22,12 @@
  * To allow use of SEC() with externs (e.g., for extern .maps declarations),
  * make sure __attribute__((unused)) doesn't trigger compilation warning.
  */
+#define DO_PRAGMA(x) _Pragma(#x)
 #define SEC(name) \
-       _Pragma("GCC diagnostic push")                                      \
-       _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"")          \
+       DO_PRAGMA("GCC diagnostic push")                                    \
+       DO_PRAGMA("GCC diagnostic ignored \"-Wignored-attributes\"")        \
        __attribute__((section(name), used))                                \
-       _Pragma("GCC diagnostic pop")                                       \
+       DO_PRAGMA("GCC diagnostic pop")                                     \

I'm not going to accept this unless I can repro it in the first place.
Using -std=c17 doesn't trigger such issue. Please provide the repro
first. Building systemd is not a repro, unfortunately. Please try to
do it based on libbpf-bootstrap ([0])

  [0] https://github.com/libbpf/libbpf-bootstrap

Seems to reproduce just fine already there with:
https://github.com/libbpf/libbpf-bootstrap/blob/31face36d469a0e3e4c4ac1cafc66747d3150930/examples/c/minimal.bpf.c

See here:
$ /home/buildroot/buildroot/output/per-package/libbpf/host/bin/bpf-gcc
-Winline -O2 -mframe-limit=32767 -mco-re -gbtf -std=gnu17 -v
-D__x86_64__ -mlittle-endian -I
/home/buildroot/buildroot/output/per-package/libbpf/host/x86_64-buildroot-linux-gnu/sysroot/usr/include
minimal.bpf.c -o minimal.bpf.o
Using built-in specs.
COLLECT_GCC=/home/buildroot/buildroot/output/per-package/libbpf/host/bin/bpf-gcc.br_real
COLLECT_LTO_WRAPPER=/home/buildroot/buildroot/output/per-package/libbpf/host/bin/../libexec/gcc/bpf-buildroot-none/12.1.0/lto-wrapper
Target: bpf-buildroot-none
Configured with: ./configure
--prefix=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host
--sysconfdir=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host/etc
--localstatedir=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host/var
--enable-shared --disable-static --disable-gtk-doc
--disable-gtk-doc-html --disable-doc --disable-docs
--disable-documentation --disable-debug --with-xmlto=no --with-fop=no
--disable-nls --disable-dependency-tracking
--target=bpf-buildroot-none
--prefix=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host
--sysconfdir=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host/etc
--enable-languages=c --with-gnu-ld --enable-static
--disable-decimal-float --disable-gcov --disable-libssp
--disable-multilib --disable-shared
--with-gmp=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host
--with-mpc=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host
--with-mpfr=/home/buildroot/buildroot/output/per-package/host-gcc-bpf/host
--with-pkgversion='Buildroot 2022.05-118-ge052166011-dirty'
--with-bugurl=http://bugs.buildroot.net/ --without-zstd --without-isl
--without-cloog
Thread model: single
Supported LTO compression algorithms: zlib
gcc version 12.1.0 (Buildroot 2022.05-118-ge052166011-dirty)
COLLECT_GCC_OPTIONS='--sysroot=/home/buildroot/buildroot/output/per-package/libbpf/host/x86_64-buildroot-linux-gnu/sysroot'
'-Winline' '-O2' '-mframe-limit=32767' '-mco-re' '-gbtf' '-std=gnu17'
'-v' '-D' '__x86_64__' '-mlittle-endian' '-I'
'/home/buildroot/buildroot/output/per-package/libbpf/host/x86_64-buildroot-linux-gnu/sysroot/usr/include'
'-o' 'minimal.bpf.o' '-dumpdir' 'minimal.bpf.o-'
 /home/buildroot/buildroot/output/per-package/libbpf/host/bin/../libexec/gcc/bpf-buildroot-none/12.1.0/cc1
-quiet -v -I /home/buildroot/buildroot/output/per-package/libbpf/host/x86_64-buildroot-linux-gnu/sysroot/usr/include
-iprefix /home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/bpf-buildroot-none/12.1.0/
-isysroot /home/buildroot/buildroot/output/per-package/libbpf/host/x86_64-buildroot-linux-gnu/sysroot
-D __x86_64__ minimal.bpf.c -quiet -dumpdir minimal.bpf.o- -dumpbase
minimal.bpf.c -dumpbase-ext .c -mframe-limit=32767 -mco-re
-mlittle-endian -gbtf -O2 -Winline -std=gnu17 -version -o
/tmp/cct4AXvg.s
GNU C17 (Buildroot 2022.05-118-ge052166011-dirty) version 12.1.0
(bpf-buildroot-none)
    compiled by GNU C version 12.1.0, GMP version 6.2.1, MPFR version
4.1.0, MPC version 1.2.1, isl version none
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
ignoring nonexistent directory
"/home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/bpf-buildroot-none/12.1.0/../../../../bpf-buildroot-none/sys-include"
ignoring nonexistent directory
"/home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/bpf-buildroot-none/12.1.0/../../../../bpf-buildroot-none/include"
ignoring duplicate directory
"/home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/../../lib/gcc/bpf-buildroot-none/12.1.0/include"
ignoring duplicate directory
"/home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/../../lib/gcc/bpf-buildroot-none/12.1.0/include-fixed"
ignoring nonexistent directory
"/home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/../../lib/gcc/bpf-buildroot-none/12.1.0/../../../../bpf-buildroot-none/sys-include"
ignoring nonexistent directory
"/home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/../../lib/gcc/bpf-buildroot-none/12.1.0/../../../../bpf-buildroot-none/include"
#include "..." search starts here:
#include <...> search starts here:
 /home/buildroot/buildroot/output/per-package/libbpf/host/x86_64-buildroot-linux-gnu/sysroot/usr/include
 /home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/bpf-buildroot-none/12.1.0/include
 /home/buildroot/buildroot/output/per-package/libbpf/host/bin/../lib/gcc/bpf-buildroot-none/12.1.0/include-fixed
End of search list.
GNU C17 (Buildroot 2022.05-118-ge052166011-dirty) version 12.1.0
(bpf-buildroot-none)
    compiled by GNU C version 12.1.0, GMP version 6.2.1, MPFR version
4.1.0, MPC version 1.2.1, isl version none
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 9bf241ca1a2dd4ffd7652c5e247c9be8
minimal.bpf.c:6:1: error: expected '=', ',', ';', 'asm' or
'__attribute__' before '#pragma'
    6 | char LICENSE[] SEC("license") = "Dual BSD/GPL";
      | ^~~
minimal.bpf.c:6:1: error: expected identifier or '(' before '#pragma'
minimal.bpf.c:10:1: error: expected identifier or '(' before '#pragma'
   10 | SEC("tp/syscalls/sys_enter_write")
      | ^~~

So this is a bug (hard to call this a feature) in gcc (not even
bpf-gcc, I could repro with a simple gcc). Is there a bug reported for
this somewhere? Are GCC folks aware and working on the fix?

Yeah, saw a few issues that looked relevant:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55578
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90400

quoted

What's curious is that the only thing that allows to bypass this is
adding #x in macro, having #define DO_PRAGMA(x) _Pragma(x) doesn't
help.

So ideally GCC can fix this?

From the reported issues...it doesn't sound like a fix is going to be
coming all that
soon in GCC.

quoted

But either way your patch as is
erroneously passing extra quoted strings to _Pragma().

I recall the extra quotes were needed to make this work, does it work for you
without them?

quoted

I'm pondering whether it's just cleaner to define SEC() without
pragmas for GCC? It will only cause compiler warning about unnecessary
unused attribute for extern *variable* declarations, which are very
rare. Instead of relying on this quirky "fix" approach. Ideally,
though, GCC just fixes _Pragma() handling, of course.

I mean, as long as this workaround is reliable I'd say using it is the
best option
for backwards compatibility, especially since it's only needed in one place from
the looks of it.

Is it reliable, though? Adding those quotes breaks Clang (I checked)
and it doesn't work as expected with GCC as well. It stops complaining
about #pragma, but it also doesn't push -Wignored-attributes. Here's
the test:

Ok, yeah, guess my hack doesn't really work then.

quoted

#define DO_PRAGMA(x) _Pragma(#x)

#define SEC(name) \
       DO_PRAGMA("GCC diagnostic push")                                    \
       DO_PRAGMA("GCC diagnostic ignored \"-Wignored-attributes\"")        \
        __attribute__((section(name), used))                               \
       DO_PRAGMA("GCC diagnostic pop")                                     \

extern int something SEC("whatever");

int main()
{
        return something;
}


Used like this you get same warning:

$ cc test.c
test.c:10:1: warning: ‘used’ attribute ignored [-Wattributes]
   10 | extern int something SEC("whatever");
      | ^~~~~~

Removing quotes fixes Clang (linker error is expected)

$ clang test.c
/opt/rh/gcc-toolset-11/root/usr/lib/gcc/x86_64-redhat-linux/11/../../../../bin/ld:

FYI I was testing with GCC 12.1.

quoted

/tmp/test-4eec0b.o: in function `main':
test.c:(.text+0xe): undefined reference to `something'

But we get back to the original problem with GCC:

$ cc test.c
test.c:10:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’
before ‘#pragma’
   10 | extern int something SEC("whatever");
      | ^~~
test.c:10:1: error: expected identifier or ‘(’ before ‘#pragma’
test.c: In function ‘main’:
test.c:14:16: error: ‘something’ undeclared (first use in this function)
   14 |         return something;
      |                ^~~~~~~~~


So the best way forward I can propose for you is this:

Yeah, probably the best option for now.

quoted

#if __GNUC__ && !__clang__

#define SEC(name) __attribute__((section(name), used))

#else

#define SEC(name) \
        _Pragma("GCC diagnostic push")                                      \
        _Pragma("GCC diagnostic ignored \"-Wignored-attributes\"")          \
        __attribute__((section(name), used))                                \
        _Pragma("GCC diagnostic pop")                                       \

#endif

extern int something SEC("whatever");

int main()
{
        return something;
}


With some comments explaining how broken GCC is w.r.t. _Pragma. And
just live with compiler warning about used if used with externs.

Yeah, do you want to spin a patch with that? I think you probably have a better
understanding of the issue at this point than I do.

I'd appreciate it if you do that and test selftests/bpf compilation
and execution with bpf-gcc (which I don't have locally). Our CI will
take care of testing Clang compilation. Thanks!

quoted

quoted

quoted

quoted

quoted

quoted

 /* Avoid 'linux/stddef.h' definition of '__always_inline'. */
 #undef __always_inline
--
2.25.1