On Thu, May 07, 2020 at 09:07:33AM -0500, Josh Poimboeuf wrote:

On Wed, May 06, 2020 at 05:03:57PM -0700, Alexei Starovoitov wrote:

quoted

quoted

quoted

quoted

quoted

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index d7ee4c6bad48..05104c3cc033 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h

@@ -171,4 +171,4 @@
 #define __diag_GCC_8(s)
 #endif

-#define __no_fgcse __attribute__((optimize("-fno-gcse")))
+#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))
--

2.23.0

I've tested it with gcc 8,9,10 and clang 11 with FP=y and with ORC=y.
All works.
I think it's safer to go with frame pointers even for ORC=y considering
all the pain this issue had caused. Even if objtool gets confused again
in the future __bpf_prog_run() will have frame pointers and kernel stack
unwinding can fall back from ORC to FP for that frame.
wdyt?

It seems dangerous to me.  The GCC manual recommends against it.

The manual can says that it's broken. That won't stop the world from using it.
Just google projects that are using it. For example: qt, lz4, unreal engine, etc
Telling compiler to disable gcse via flag is a guaranteed way to avoid
that optimization that breaks objtool whereas messing with C code is nothing
but guess work. gcc can still do gcse.

But the manual's right, it is broken.  How do you know other important
flags won't also be stripped?

What flags are you worried about?
I've checked that important things like -mno-red-zone, -fsanitize are preserved.

It's not any specific flags I'm worried about, it's all of them.  There
are a lot of possibilities, with all the different configs, and arches.
Flags are usually added for a good reason, so one randomly missing flag
could have unforeseen results.

And I don't have any visibility into how GCC decides which flags to
drop, and when.  But the docs aren't comforting.

That doc change landed 5 years ago:
https://patchwork.ozlabs.org/project/gcc/patch/20151213081911.GA320@x4/
Sure it's 'broken' by whatever definition of broken.
Yet gcc has
$ git grep '__attribute__((optimize' gcc/testsuite/|wc -l
34 tests to make sure it stays working.
And gcc is using it to bootstrap itself. See LIBGCC2_UNWIND_ATTRIBUTE.
The doc is expressing desire and trying to discourage its use,
but that attribute is not going anywhere.

Even if things seem to work now, that could (silently) change at any
point in time.  This time objtool warned about the missing frame
pointer, but that's not necessarily going to happen for other flags.

If we go this route, I would much rather do -fno-gcse on a file-wide
basis.

The fix for broken commit 3193c0836f20 has to be backported all the way
to 5.3 release. I'd like to minimize conflicts.
For that very reason I'm not even renaming #define __no_fgcse.