On Tue, May 05, 2020 at 03:28:23PM -0500, Josh Poimboeuf wrote:

On Tue, May 05, 2020 at 12:53:20PM -0700, Alexei Starovoitov wrote:

quoted

On Tue, May 05, 2020 at 01:11:08PM -0500, Josh Poimboeuf wrote:

quoted

On Tue, May 05, 2020 at 10:43:00AM -0700, Alexei Starovoitov wrote:

quoted

quoted

Or, if you want to minimize the patch's impact on other arches, and keep
the current patch the way it is (with bug fixed and changed patch
description), that's fine too.  I can change the patch description
accordingly.

Or if you want me to measure the performance impact of the +40% code
growth, and *then* decide what to do, that's also fine.  But you'd need
to tell me what tests to run.

I'd like to minimize the risk and avoid code churn,
so how about we step back and debug it first?
Which version of gcc are you using and what .config?
I've tried:
Linux version 5.7.0-rc2 (gcc version 10.0.1 20200505 (prerelease) (GCC)
CONFIG_UNWINDER_ORC=y
# CONFIG_RETPOLINE is not set

and objtool didn't complain.
I would like to reproduce it first before making any changes.

Revert

  3193c0836f20 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")

and compile with retpolines off (and either ORC or FP, doesn't matter).

I'm using GCC 9.3.1:

  kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x8dc: sibling call from callable instruction with modified stack frame

That's the original issue described in that commit.

I see something different.
With gcc 8, 9, and 10 and CCONFIG_UNWINDER_FRAME_POINTER=y
I see:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x4837: call without frame pointer save/setup
and sure enough assembly code for ___bpf_prog_run does not countain frame setup
though -fno-omit-frame-pointer flag was passed at command line.
Then I did:
static u64 /*__no_fgcse*/ ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
and the assembly had proper frame, but objtool wasn't happy:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x480a: sibling call from callable instruction with modified stack frame

gcc 6.3 doesn't have objtool warning with and without -fno-gcse.

Looks like we have two issues here.
First gcc 8, 9 and 10 have a severe bug with __attribute__((optimize("")))
In this particular case passing -fno-gcse somehow overruled -fno-omit-frame-pointer
which is serious issue. powerpc is using __nostackprotector. I don't understand
how it can keep working with newer gcc-s. May be got lucky.
Plenty of other projects use various __attribute__((optimize("")))
they all have to double check that their vesion of GCC produces correct code.
Can somebody reach out to gcc folks for explanation?

Right.  I've mentioned this several times now.  That's why my patch
reverts 3193c0836f20.  I don't see any other way around it.  The GCC
manual even says this attribute should not be used in production code.

What you mentioned in commit log is:
"It doesn't append options to the command-line arguments.  Instead
it starts from a blank slate.  And according to recent GCC documentation
it's not recommended for production use."

I don't think anyone could have guessed from such description that it kills
-fno-omit-frame-pointer but it doesn't reduce optimization level to -O0
and it doesn't kill -D, -m, -I, -std= and other flags.

As far as workaround I prefer the following:
From 94bbc27c5a70d78846a5cb675df4cf8732883564 Mon Sep 17 00:00:00 2001
From: Alexei Starovoitov <ast@kernel.org>
Date: Tue, 5 May 2020 16:52:41 -0700
Subject: [PATCH] bpf,objtool: tweak interpreter compilation flags to help objtool

tbd

Fixes: 3193c0836f20 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
Reported-by: Randy Dunlap <redacted>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/compiler-gcc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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?