Re: [PATCH v5] x86: use builtins to read eflags

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Nick Desaulniers <ndesaulniers@google.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>,
	Bill Wendling <morbo@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<x86@kernel.org>, "Nathan Chancellor" <nathan@kernel.org>,
	Juergen Gross <jgross@suse.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	Andy Lutomirski <luto@kernel.org>,
	"llvm@lists.linux.dev" <llvm@lists.linux.dev>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-toolchains <linux-toolchains@vger.kernel.org>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>
Subject: Re: [PATCH v5] x86: use builtins to read eflags
Date: Thu, 17 Mar 2022 21:05:52 +0000	[thread overview]
Message-ID: <fd89333f-e470-a295-baf6-a736c55caeb5@citrix.com> (raw)
In-Reply-To: <CAHk-=whJfKN8Jag=8DS=pbZR3TY90znUOP6Km+TLRJ9dZEgNqw@mail.gmail.com>

On 17/03/2022 18:52, Linus Torvalds wrote:
> On Thu, Mar 17, 2022 at 11:00 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>>> One change you may see due to this patch is the compiler moving the
>>>> "pop %..." instruction away from the "pushf" instruction. This could
>>>> happen if the compiler determines that it could produce better code by
>>>> doing so---e.g. to reduce register pressure. The "gcc -O2" example
>>>> below shows this code movement.
> Honestly, that part worries me a _lot_.
>
> Why?
>
> Because clang in particular has already messed up eflags handling
> once, by spilling condition codes (in the form of eflags) onto the
> stack, and then loading them later with a "popf".
>
> And it did so across a function call THAT MODIFIED 'IF'. This was a
> major bug in clang back in 2015 or so, and made it unusable for the
> kernel.
>
> See for example
>
>     https://lore.kernel.org/all/CA+icZUU7y5ATSLV_0TGzi5m5deWADLmAMBkAT32FKGyUWNSJSA@mail.gmail.com/
>
> for some kernel discussion, and separately
>
>     https://lists.llvm.org/pipermail/llvm-dev/2015-July/088774.html
>
> for just llvm discussions.
>
> It is perhaps telling that the LLVM discussion I found seems to talk
> more about the performance impact, not about the fact that THE
> GENERATED CODE WAS WRONG.
>
> That compiler bug would basically enable or disable interrupts in
> random places - because clang developers thought that 'eflags' is only
> about the condition codes.

I was idly playing with this builtin in response to the thread, and
found that GCC has a far more serious bug than Clang had.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104971

If the optimiser concluded that the result of the readflags builtin is
unused, it generates:

broken:
    pushf
    ret

~Andrew