Received-SPF: pass (google.com: domain of linux-kernel+bounces-59692-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240208220604.140859-1-seanjc@google.com>
In-Reply-To: <20240208220604.140859-1-seanjc@google.com>
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Fri, 9 Feb 2024 09:03:22 -0800
Message-ID: <CAKwvOdk_obRUkD6WQHhS9uoFVe3HrgqH5h+FpqsNNgmj4cmvCQ@mail.gmail.com>
Subject: Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11
 (and earlier)
To: Sean Christopherson <seanjc@google.com>, quic_apinski@quicinc.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>, linux-kernel@vger.kernel.org, 
	Masahiro Yamada <masahiroy@kernel.org>, Peter Zijlstra <peterz@infradead.org>, kvm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

+ Andrew

Andrew, here's the full thread, I cut most of it out:
https://lore.kernel.org/lkml/20240208220604.140859-1-seanjc@google.com/
.

On Thu, Feb 8, 2024 at 2:06=E2=80=AFPM Sean Christopherson <seanjc@google.c=
om> wrote:
>
> Explicitly require gcc-12+ to enable asm goto with outputs on gcc to avoi=
d
> what is effectively a data corruption bug on gcc-11.  As per
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html, "asm goto" is
> *supposed* be implicitly volatile, but gcc-11 fails to treat it as such.
> When compiling with -O2, failure to treat the asm block as volatile can
> result in the entire block being discarded during optimization.
>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D103979

Shoot, I was cc'ed on that bug (I think I noticed it in testing as
well, and pointed it out to Andrew on IRC who then cc'ed me to it). I
probably should have asked if that would cause issues at some point
for the kernel.

I took a look at the test case added in that bug; it doesn't compile
until gcc-13 (specifically gcc 13.2, not gcc 13.1).  I'm curious since
the bug says the fix was backported to gcc-12 and gcc-13. Are there
specific versions of those that contain the fix? If so, should Sean
amend his version checks below? For instance, was the fix backported
to gcc 12.3, so users of gcc 12.2 would still have issues?  I can't
tell in godbolt since the added test case doesn't compile until gcc
13.2. https://godbolt.org/z/eqaa7dfo3

My implementation in clang still has some issues, too.  It's hard to
get new control flow right, and there are minimal users outside the
kernel to help us validate.

So as much of a fan of feature detection as I am, I admit some of
these edge cases aren't perfect, and we may need to result to version
detection when such bugs become observable to users.

I'm happy to ack this patch, but I would like to wait for feedback
from Andrew as to whether we can be even more precise with avoiding
more specific versions of gcc 12 and 13 (if necessary).

> Fixes: 587f17018a2c ("Kconfig: add config option for asm goto w/ outputs"=
)
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: kvm@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>
> Linus, I'm sending to you directly as this seems urgent enough to apply
> straightaway, and this obviously affects much more than the build system.
>
>  init/Kconfig | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index deda3d14135b..f4e46d64c1e7 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -82,6 +82,11 @@ config CC_CAN_LINK_STATIC
>         default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG=
_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
>
>  config CC_HAS_ASM_GOTO_OUTPUT
> +       # gcc-11 has a nasty bug where it doesn't treat asm goto as volat=
ile,
> +       # which can result in asm blocks being dropped when compiling wit=
h -02.
> +       # Note, explicitly forcing volatile doesn't entirely fix the bug!
> +       # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D103979
> +       depends on !CC_IS_GCC || GCC_VERSION >=3D 120000

LGTM; but we might need to be more specific about avoiding certain min
versions of gcc 13 and 12.

>         def_bool $(success,echo 'int foo(int x) { asm goto ("": "=3Dr"(x)=
 ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
>
>  config CC_HAS_ASM_GOTO_TIED_OUTPUT
>
> base-commit: 047371968ffc470769f541d6933e262dc7085456
> --
> 2.43.0.687.g38aa6559b0-goog
>


--=20
Thanks,
~Nick Desaulniers