On Fri, May 01, 2020 at 02:22:04PM -0500, Josh Poimboeuf wrote:
> On Fri, May 01, 2020 at 12:09:30PM -0700, Alexei Starovoitov wrote:
> > On Thu, Apr 30, 2020 at 02:07:43PM -0500, Josh Poimboeuf wrote:
> > > Objtool decodes instructions and follows all potential code branches
> > > within a function. But it's not an emulator, so it doesn't track
> > > register values. For that reason, it usually can't follow
> > > intra-function indirect branches, unless they're using a jump table
> > > which follows a certain format (e.g., GCC switch statement jump tables).
> > >
> > > In most cases, the generated code for the BPF jump table looks a lot
> > > like a GCC jump table, so objtool can follow it. However, with
> > > RETPOLINE=n, GCC keeps the jump table address in a register, and then
> > > does 160+ indirect jumps with it. When objtool encounters the indirect
> > > jumps, it can't tell which jump table is being used (or even whether
> > > they might be sibling calls instead).
> > >
> > > This was fixed before by disabling an optimization in ___bpf_prog_run(),
> > > using the "optimize" function attribute. However, that attribute is bad
> > > news. 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. So revert the previous fix:
> > >
> > > 3193c0836f20 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> > >
> > > With that reverted, solve the original problem in a different way by
> > > getting rid of the "goto select_insn" indirection, and instead just goto
> > > the jump table directly. This simplifies the code a bit and helps GCC
> > > generate saner code for the jump table branches, at least in the
> > > RETPOLINE=n case.
> > >
> > > But, in the RETPOLINE=y case, this simpler code actually causes GCC to
> > > generate far worse code, ballooning the function text size by +40%. So
> > > leave that code the way it was. In fact Alexei prefers to leave *all*
> > > the code the way it was, except where needed by objtool. So even
> > > non-x86 RETPOLINE=n code will continue to have "goto select_insn".
> > >
> > > This stuff is crazy voodoo, and far from ideal. But it works for now.
> > > Eventually, there's a plan to create a compiler plugin for annotating
> > > jump tables. That will make this a lot less fragile.
> >
> > I don't like this commit log.
> > Here you're saying that the code recognized by objtool is sane and good
> > whereas well optimized gcc code is somehow voodoo and bad.
> > That is just wrong.
>
> I have no idea what you're talking about.
>
> Are you saying that ballooning the function text size by 40% is well
> optimized GCC code? It seems like a bug to me. That's the only place I
> said anything bad about GCC code.
It could be a bug, but did you benchmark the speed of interpreter ?
Is it faster or slower with 40% more code ?
Did you benchmark it on other archs ?
> When I said "this stuff is crazy voodoo" I was referring to the patch
> itself. I agree it's horrible, it's only the best approach we're able
> to come up with at the moment.
please reword it then.
