Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Nick Desaulniers]

On Wed, Oct 28, 2020 at 4:20 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Oct 29, 2020 at 12:10:52AM +0100, Ard Biesheuvel wrote:
> > On Wed, 28 Oct 2020 at 23:59, Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > I'm totally fine with making
> > > #define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))
> > > to be gcc+x86 only.
> > > I'd like to get rid of it, but objtool is not smart enough to understand
> > > generated asm without it.
> >
> > I'll defer to the x86 folks to make the final call here, but I would
> > be perfectly happy doing
> >
> > index d1e3c6896b71..68ddb91fbcc6 100644
> > --- a/include/linux/compiler-gcc.h
> > +++ b/include/linux/compiler-gcc.h
> > @@ -176,4 +176,6 @@
> >  #define __diag_GCC_8(s)
> >  #endif
> >
> > +#ifdef CONFIG_X86
> >  #define __no_fgcse __attribute__((optimize("-fno-gcse")))
> > +#endif
>
> If you're going to submit this patch could you please add
> ,-fno-omit-frame-pointer
> to the above as well?

You'll be playing whack-a-mole with other -f flags that should have
been used, which changes even based on the config.  The -fsanitize=
flags come to mind with the sanitizers.

defconfig shows:
$ make LLVM=1 -j71 kernel/bpf/core.o V=1 2>&1 | grep "\-f"
the following -f flags set:

-fno-strict-aliasing
-fno-common
-fshort-wchar
-fno-PIE
-fno-asynchronous-unwind-tables
-fno-delete-null-pointer-checks
-fomit-frame-pointer
-fmacro-prefix-map=./=
-fstack-protector-strong

We already know that -fno-asynchronous-unwind-tables get dropped,
hence this patch.  And we know -fomit-frame-pointer or
-fno-omit-frame-pointer I guess gets dropped, hence your ask.  We
might not know the full extent which other flags get dropped with the
optimize attribute, but I'd argue that my list above can all result in
pretty bad bugs when accidentally omitted (ok, maybe not -fshort-wchar
or -fmacro-prefix-map, idk what those do) or when mixed with code that
has different values those flags control.  Searching GCC's bug tracker
for `__attribute__((optimize` turns up plenty of reports to make me
think this attribute maybe doesn't work the way folks suspect or
intend: https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=__attribute__%28%28optimize&list_id=283390.

There's plenty of folks arguing against the use of the optimize
attribute in favor of the command line flag.  I urge you to please
reconsider the request.

> Frankly I'm more worried that -Os will generate incorrect code.

If you have observed bugs as a result of setting
CONFIG_CC_OPTIMIZE_FOR_SIZE, we would love to help you get to the
bottom of them and help you debug them.  But we should also remain
vigilant against rejecting progress on the status quo for known issues
over hypothetical issues without proper regard for evidence.
Correctness is the chief concern of a compiler; that it generates
incorrect code unless default-on optimizations are explicitly disabled
would be concerning, if that was in fact the case.  Such a bug report
would be invaluable to this code base, and likely others.  I trust
you've seen bugs here, but I would like to help verify this claim.

> All compilers have bugs. Kernel has bugs. What can go wrong?

This is more terrifyingly precise and infinitely wise than you may
have initially intended.  That my phone and laptop don't catch fire
simultaneously now is nothing short of miraculous.  I'm still holding
my breath.

--
Thanks,
~Nick Desaulniers