Frame pointer corrupted by __cpuid after #85193

[thurstond]

"[InstCombine] Support and/or in getFreelyInvertedImpl using DeMorgan's Law" (#85193) in isolation is a correct and innocuous InstCombine patch, but it appears to be triggering a latent miscompilation bug that is particularly noticeable when using the cpuid instruction in the presence of register pressure.

Context

The cpuid instruction writes to registers RAX, RBX, RCX, and RDX. Since RBX is used as the frame pointer (if enabled), it typically needs to be preserved, such as by exchanging it back and forth with another register. For example, clang's __cpuid macro has:

/* x86-64 uses %rbx as the base register, so preserve it. */
#define __cpuid(__leaf, __eax, __ebx, __ecx, __edx) \
    __asm("  xchgq  %%rbx,%q1\n" \
          "  cpuid\n" \
          "  xchgq  %%rbx,%q1" \
        : "=a"(__eax), "=r" (__ebx), "=c"(__ecx), "=d"(__edx) \
        : "0"(__leaf))

(https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/cpuid.h#L268)

Reproducer

git clone https://github.com/pytorch/cpuinfo.git # commit 6543fec09b2f04ac4a666882998b534afc9c1349
cd cpuinfo/src
clang -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -O2 -I../include -I. x86/isa.c -msse4.2 -gmlt -c -o isa.o 
llvm-objdump -D -S isa.o | grep cpuid -A 1 -B 1

clang at revision cf5cd98:

      75: 48 87 f3                      xchgq   %rbx, %rsi
      78: 0f a2                         cpuid
      7a: 48 87 f3                      xchgq   %rbx, %rsi
      # Editor's note: this works; it is using %rsi as temporary storage, which happens to be available.
--
      99: 48 87 f3                      xchgq   %rbx, %rsi
      9c: 0f a2                         cpuid
      9e: 48 87 f3                      xchgq   %rbx, %rsi
--
      ce: 48 87 cb                      xchgq   %rbx, %rcx
      d1: 0f a2                         cpuid
      d3: 48 87 cb                      xchgq   %rbx, %rcx
      # This doesn't work, because %rcx is also modified by cpuid. As a result, %rbx is corrupted, and subsequent use of the frame pointer leads to a segfault or memory corruption.
--
     127: 48 87 f3                      xchgq   %rbx, %rsi
     12a: 0f a2                         cpuid
     12c: 48 87 f3                      xchgq   %rbx, %rsi
--
     a54: 48 87 f3                      xchgq   %rbx, %rsi
     a57: 0f a2                         cpuid
     a59: 48 87 f3                      xchgq   %rbx, %rsi
--
     a72: 48 87 f3                      xchgq   %rbx, %rsi
     a75: 0f a2                         cpuid
     a77: 48 87 f3                      xchgq   %rbx, %rsi

clang at the immediately preceding revision (6d30223) with the same compile command does not xchg %rbx with %rcx:

      75: 48 87 f3                      xchgq   %rbx, %rsi
      78: 0f a2                         cpuid
      7a: 48 87 f3                      xchgq   %rbx, %rsi
--
      99: 48 87 f3                      xchgq   %rbx, %rsi
      9c: 0f a2                         cpuid
      9e: 48 87 f3                      xchgq   %rbx, %rsi
--
      ce: 4c 87 fb                      xchgq   %rbx, %r15
      d1: 0f a2                         cpuid
      d3: 4c 87 fb                      xchgq   %rbx, %r15
--
     122: 48 87 f3                      xchgq   %rbx, %rsi
     125: 0f a2                         cpuid
     127: 48 87 f3                      xchgq   %rbx, %rsi
--
     a55: 48 87 f3                      xchgq   %rbx, %rsi
     a58: 0f a2                         cpuid
     a5a: 48 87 f3                      xchgq   %rbx, %rsi
--
     a73: 48 87 f3                      xchgq   %rbx, %rsi
     a76: 0f a2                         cpuid
     a78: 48 87 f3                      xchgq   %rbx, %rsi

N.B. Memory sanitizer with track-origins was used as a convenient way to increase register pressure, but I believe this could happen without sanitizers.

[topperc]

Register allocation did the right thing, but post-RA machine copy propagation removed a copy that was needed to make the code correct.

MCP: Replacing $r15d                                                             
     with $ecx                                                                   
     in INLINEASM &"  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}" [attdialect], $0:[regdef], implicit-def dead $eax, $1:[regdef:GR32_NOREX2], def renamable $r15d, $2:[regdef], implicit-def dead $ecx, $3:[regdef], implicit-def dead $edx, $4:[reguse tiedto:$0], $eax(tied-def 3), $5:[clobber], implicit-def dead early-clobber $df, $6:[clobber], implicit-def early-clobber $fpsw, $7:[clobber], implicit-def dead early-clobber $eflags, !32, debug-location !29; ./x86/cpuid.h:29:2 @[ x86/isa.c:52:5 ]
     from renamable $ecx = COPY killed renamable $r15d                           
MCP: After replacement: INLINEASM &"  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}" [attdialect], $0:[regdef], implicit-def dead $eax, $1:[regdef:GR32_NOREX2], def renamable $ecx, $2:[regdef], implicit-def dead $ecx, $3:[regdef], implicit-def dead $edx, $4:[reguse tiedto:$0], $eax(tied-def 3), $5:[clobber], implicit-def dead early-clobber $df, $6:[clobber], implicit-def early-clobber $fpsw, $7:[clobber], implicit-def dead early-clobber $eflags, !32, debug-location !29; ./x86/cpuid.h:29:2 @[ x86/isa.c:52:5 ]

[topperc]

Candidate fix

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 9a0ab300b21b..65c067e4874b 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -640,7 +640,7 @@ bool MachineCopyPropagation::hasImplicitOverlap(const MachineInstr &MI,
 /// The umull instruction is unpredictable unless RdHi and RdLo are different.
 bool MachineCopyPropagation::hasOverlappingMultipleDef(
     const MachineInstr &MI, const MachineOperand &MODef, Register Def) {
-  for (const MachineOperand &MIDef : MI.defs()) {
+  for (const MachineOperand &MIDef : MI.all_defs()) {
     if ((&MIDef != &MODef) && MIDef.isReg() &&
         TRI->regsOverlap(Def, MIDef.getReg()))
       return true;

[thurstond]

Candidate fix

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 9a0ab300b21b..65c067e4874b 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -640,7 +640,7 @@ bool MachineCopyPropagation::hasImplicitOverlap(const MachineInstr &MI,
 /// The umull instruction is unpredictable unless RdHi and RdLo are different.
 bool MachineCopyPropagation::hasOverlappingMultipleDef(
     const MachineInstr &MI, const MachineOperand &MODef, Register Def) {
-  for (const MachineOperand &MIDef : MI.defs()) {
+  for (const MachineOperand &MIDef : MI.all_defs()) {
     if ((&MIDef != &MODef) && MIDef.isReg() &&
         TRI->regsOverlap(Def, MIDef.getReg()))
       return true;

Thanks! That's a very elegant fix 😁