Re: [PATCH] arm64: fix unreachable code issue with cmpxchg

[Arnd Bergmann]

On Tue, Sep 10, 2019 at 3:24 PM Will Deacon <will@xxxxxxxxxx> wrote:
> On Tue, Sep 10, 2019 at 10:04:24AM +0200, Arnd Bergmann wrote:
> > On Tue, Sep 10, 2019 at 9:46 AM Will Deacon <will@xxxxxxxxxx> wrote:
> > - In theory, CONFIG_OPTIMIZE_INLINING is the right thing to do -- the compilers
> >   also make some particularly bad decisions around inlining when each inline
> >   turns into an __always_inline, as has been the case in Linux for a long time.
> >   I think in most cases, we get better object code with CONFIG_OPTIMIZE_INLINING
> >   and in the cases where this is worse, it may be better to fix the compiler.
> >   The new "asm_inline"  macro should also help with that.
>
> Sure, in theory, but it looks like there isn't a single arm64 compiler out
> there which gets it right.

I don't see anything architecture specific in here. When the option was
made generic instead of x86 specific, I fixed a ton of bugs that showed
up all over the place. If we don't want it on arm64, I'd suggest making
it a per-architecture opt-in instead of an opt-out.

> >
> >     | commit 4f81c5350b44bcc501ab6f8a089b16d064b4d2f6
> >     | Author: Jeff Dike <jdike@xxxxxxxxxxx>
> >     | Date:   Mon Jul 7 13:36:56 2008 -0400
> >     |
> >     |     [UML] fix gcc ICEs and unresolved externs
> >     [...]
> >     |    This patch reintroduces unit-at-a-time for gcc >= 4.0,
> > bringing back the
> >     |    possibility of Uli's crash.  If that happens, we'll debug it.
> >
> >     it's still default-off and thus opt-in.
>
> This appears to be fixing an ICE, whereas the issue reported recently for
> arm64 gcc was silent miscompilation of atomics in some cases. Unfortunately,
> I can't seem to find the thread :/ Mark, you were on that one too, right?

Sorry, that reference was unclear, I meant the text for commit 3f9b5cc01856,
which in turn contains a citation of the earlier 4f81c5350b44bc commit.

> > - The inlining decisions of gcc and clang are already very different, and
> >    the bugs we are finding around that are much more common than
> >    the difference between CONFIG_OPTIMIZE_INLINING=y/n on a
> >    given compiler.
>
> Sorry, not sure that you're getting at here.
>
> Anyway, the second version of your patch looks fine, but I would still
> prefer to go the extra mile and disable CONFIG_OPTIMIZE_INLINING altogether
> given that I don't think it's a safe option to enable for us.

The point is that function inlining frequently causes all kinds of problems
when code was written in a way that is not entirely reproducible but
depends on the behavior of a particular implementation. I've fixed
lots of bugs based on any of these:

- gcc-4.0 and higher started ignoring 'inline' without
  __attribute__((always_inline)), so a workaround got applied
  in 2.6.26, and this turned into CONFIG_OPTIMIZE_INLINING=n
  later
- gcc -O2 makes different decisions compared to -Os and -O3,
  which is an endless source of "uninitialized variable" warnings
  and similar problems
- Some configuration options like KASAN grow the code to result
  in less inlining
- clang and gcc behave completely differently
- gcc is traditionally bad at guessing the size of inline assembly
  to make a good decision
- newer compilers tend to get better at identifying which functions
  benefit from inlining, which changes the balance

CONFIG_OPTIMIZE_INLINING clearly adds to that mess, but it's
not the worst part. The only real solution tends to be to write
portable and correct code rather than making assumptions
about compiler behavior.

    Arnd