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

[Ard Biesheuvel]

On Fri, 30 Oct 2020 at 04:22, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Oct 29, 2020 at 05:28:11PM -0700, Nick Desaulniers wrote:
> >
> > We already know that -fno-asynchronous-unwind-tables get dropped,
> > hence this patch.
>
> On arm64 only. Not on x86
>
> > And we know -fomit-frame-pointer or
> > -fno-omit-frame-pointer I guess gets dropped, hence your ask.
>
> yep. that one is bugged.
>
> > 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
>
> true.
> Few month back I've checked that strict-aliasing and no-common flags
> from your list are not dropped by this attr in gcc [6789].
> I've also checked that no-red-zone and model=kernel preserved as well.
>
> > 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 is a risk.
> Is it a footgun? Sure.
> Yet. gcc testsuite is using __attribute__((optimize)).
> And some of these tests were added _after_ offical gcc doc said that this
> attribute is broken.
> imo it's like 'beware of the dog' sign.
>
> > 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.
>
> ok. Applied this first patch to bpf tree and will get it to Linus soon.
> Second patch that is splitting interpreter out because of this mess
> is dropped. The effect of gcse on performance is questionable.
> iirc some interpreters used to do -fno-gcse to gain performance.

That is absolutely fine. I only included the second patch to address
Daniel's concern that -fno-gcse could affect unrelated code living in
the same source file as __bpf_prog_run(), but if you don't care about
that, nor will I.