GCC asm goto outputs workaround (Was: "Re: [PATCH 1/3] arm64: start using 'asm goto' for get_user() when") available

[Linus Torvalds]

On Wed, 17 Jul 2024 at 09:23, Mark Rutland <mark.rutland at arm.com> wrote:
>
> As a heads-up, this is tickling a GCC bug with asm goto outputs in GCC
> versions prior to 14.1.0, and the existing asm_goto_output() workaround
> doesn't seem to be sufficent.

Uhhuh.

I've done all my testing with gcc version 13.3.1, but that is one of
the fixed versions.

> I spotted that while preparing a fixup for a thinko with
> _ASM_EXTABLE_##type##ACCESS_ERR(),

Ack. Your fix looks obviously correct to me, but yeah, the deeper
issue is the compiler bug interaction.

> | #define asm_goto_output(x...) \
> |         do { asm volatile goto(x); asm (""); } while (0)
>
> ... but testing indicates that's insufficient on arm64 and x86_64, and
> even with that I see GCC erroneously omitting assignments to variables
> in the 'goto' paths, where those are assigned an output in the non-goto
> path.
>
> As a reduced test case I have: [ snip snip ]
>
> ... which GCC 13.2.0 (at -O2) compiles to:
>
> |       cbnz    x0, .Lfailed
> |       mov     x0, #0x900d
> | .Lfailed:
> |       ret

Yeah, that test-case works fine for me, and I get the expected result:

  get_val:
  .LFB0:
        .cfi_startproc
        cbnz    x0, .L3
        mov     x0, #0x900d
        ret
        .p2align 2,,3
  .L3:
  .L2:
        mov     x0, 2989
        ret
        .cfi_endproc

but as mentioned, I have one of the gcc versions that doesn't need
that GCC_ASM_GOTO_OUTPUT_WORKAROUND:

  # Fixed in GCC 14, 13.3, 12.4 and 11.5

so my gcc-13.3.1 doesn't have the issue.

> Can anyone from the GCC side comment as to whether placing the dummy asm("")
> block after the goto labels is a sufficient workaround, or whether that's just
> avoiding the issue by chance?

I have to say that the workaround of putting the empty asm on the
target label rather than next to the "asm goto" itself would seem to
be more sensible than our current workaround, but the problem is that
the target is entirely separate.

The current workaround was very much a "this works in practice in the
(very few) cases we saw" and works syntactically.

And yes, due to how arm64 inlines get_user(), arm64 ends up seeing a
lot more effects of this.

Hmm. I was thinking that we could change the "asm_goto_output()" macro
to take the exception label as the first argument, and create some
kind of trampoline for the exception case with the workaround doing
something like

    asm goto(/* for real */ ... : temp_label);
    if (0) { temp_label: asm volatile(""); goto real_label; }

but it turns out that we actually have cases of multiple output labels
on x86: see __vmcs_readl() in arch/x86/kvm/vmx/vmx_ops.h.

So doing that kind of wrapping looks non-trivial.

Maybe we just need to give up on the workaround and say that "asm goto
with outputs only works when ASM_GOTO_OUTPUT_WORKAROUND is not set".

The set of broken gcc versions will get progressively smaller as time goes on.

                Linus