Message-ID: 
 <DM6PR02MB40587AD6ABBF1814E9CCFA7CB84B2@DM6PR02MB4058.namprd02.prod.outlook.com>
Date: Fri, 9 Feb 2024 17:14:40 +0000
From: "Andrew Pinski (QUIC)" <quic_apinski@...cinc.com>
To: Nick Desaulniers <ndesaulniers@...gle.com>,
        Sean Christopherson
	<seanjc@...gle.com>,
        "Andrew Pinski (QUIC)" <quic_apinski@...cinc.com>
CC: Linus Torvalds <torvalds@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Masahiro
 Yamada <masahiroy@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: RE: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11
 (and earlier)

> -----Original Message-----
> From: Nick Desaulniers <ndesaulniers@...gle.com>
> Sent: Friday, February 9, 2024 9:03 AM
> To: Sean Christopherson <seanjc@...gle.com>; Andrew Pinski (QUIC)
> <quic_apinski@...cinc.com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>; linux-
> kernel@...r.kernel.org; Masahiro Yamada <masahiroy@...nel.org>; Peter
> Zijlstra <peterz@...radead.org>; kvm@...r.kernel.org
> Subject: Re: [PATCH] Kconfig: Explicitly disable asm goto w/ outputs on gcc-11
> (and earlier)
> 
> + Andrew
> 
> Andrew, here's the full thread, I cut most of it out:
> https://lore.kernel.org/lkml/20240208220604.140859-1-
> seanjc@...gle.com/

So the exact versions of GCC where this is/was fixed are:
12.4.0 (not released yet)
13.2.0
14.1.0 (not released yet)

This information is listed in the bug report via the "known to work" field though since 12.4.0 is not released yet, it is listed as 12.3.1. (13.2.0 was not released at the time it was backported so it is listed as 13.1.1). The target milestone in this case lists the lowest version # where the fix is/will be included.

Thanks,
Andrew Pinski

> .
> 
> On Thu, Feb 8, 2024 at 2:06 PM Sean Christopherson <seanjc@...gle.com>
> wrote:
> >
> > Explicitly require gcc-12+ to enable asm goto with outputs on gcc to
> > avoid 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=103979
> 
> 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@...gle.com>
> > Cc: Masahiro Yamada <masahiroy@...nel.org>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: kvm@...r.kernel.org
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@...gle.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 volatile,
> > +       # which can result in asm blocks being dropped when compiling with -
> 02.
> > +       # Note, explicitly forcing volatile doesn't entirely fix the bug!
> > +       # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103979
> > +       depends on !CC_IS_GCC || GCC_VERSION >= 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 ("":
> > "=r"(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
> >
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

Powered by blists - more mailing lists