[X86] Save/restore base pointer register when it is clobbered

[LuoYuanke]

Summary

The base pointer register is reserved by compiler when there is
dynamic size alloca and stack realign in a function. However the
base pointer register is not defined in X86 ABI, so user can use
this register in inline assembly. The inline assembly would
clobber base pointer register without being awared by user. This
patch to save and restore the base pointer register when compiler
detect it is clobbered. Below is the example code for such case.

extern int bar(void *p);
long long foo(size_t size, char c, int id) {
  __attribute__((__aligned__(64))) int a;
  char *p = (char *)alloca(size);
  asm volatile ("nop"::"S"(405):);
  asm volatile ("movl %0, %1"::"r"(id), "m"(a):);
  p[2] = 8;
  memset(p, c, size);
  return bar(p);
}

[craig.topper]

Incorrect title? You’re saving and restoring not disabling right?

[LuoYuanke]

In D144541#4143733, @craig.topper wrote:

Incorrect title? You’re saving and restoring not disabling right?

Thanks, Craig. Changed the title.

[craig.topper]

llvm/test/CodeGen/X86/x86-64-baseptr.ll

123

If ebx needs to be restored how can you use it in the address?

[LuoYuanke]

llvm/test/CodeGen/X86/x86-64-baseptr.ll

123

Opps, l'll refine the patch. Thanks, Craig.

[pengfei]

Looks great!

[LuoYuanke]

Address Craig's comments.

[LuoYuanke]

llvm/test/CodeGen/X86/i386-baseptr.ll

43

I'll check if the disp is set right.

[LuoYuanke]

llvm/test/CodeGen/X86/i386-baseptr.ll

43

It seems there is a bug in https://reviews.llvm.org/D6388, I create https://reviews.llvm.org/D144625 to fix this bug.

[LuoYuanke]

Rebase and update test case.

[efriedma]

What happens if an inline asm both has a memory reference, and clobbers the base pointer? How do we refer to memory?

[LuoYuanke]

In D144541#4152619, @efriedma wrote:

What happens if an inline asm both has a memory reference, and clobbers the base pointer? How do we refer to memory?

That's a good question. Maybe exchange ebp and esi. Base pointer (esi) is used to reference the stack slot before realignment and frame pointer (ebp) is used to reference stack slot after realignment. Since ebp is reserved in ABI, user can not clobber ebp in inline assembly. What do you think?

[efriedma]

In D144541#4152873, @LuoYuanke wrote:

In D144541#4152619, @efriedma wrote:

What happens if an inline asm both has a memory reference, and clobbers the base pointer? How do we refer to memory?

That's a good question. Maybe exchange ebp and esi. Base pointer (esi) is used to reference the stack slot before realignment and frame pointer (ebp) is used to reference stack slot after realignment. Since ebp is reserved in ABI, user can not clobber ebp in inline assembly. What do you think?

Possible alternatives:

• Use ebp... but this messes with unwinding, and doesn't actually solve the issue in general (if the inline asm references function arguments, those references are ebp-relative).
• Use some other register not used by the inline asm as a temporary base pointer.
• We use a different prologue that avoids the need for a base pointer. See discussion at https://github.com/llvm/llvm-project/issues/17204

[LuoYuanke]

Possible alternatives:

• Use ebp... but this messes with unwinding, and doesn't actually solve the issue in general (if the inline asm references function arguments, those references are ebp-relative).

Why it would mess unwinding? The cfi instruction can indicate %esi to compute CFA. The assembly code may look like below. Yes, there is still issue if inline asm references function arguments after modifying %esi.

        .cfi_startproc
# %bb.0:                                # %entry
        pushl   %esi
        .cfi_def_cfa_offset 8
        .cfi_offset %esi, -8
        movl    %esp, %esi       // esi is the frame pointer
        .cfi_def_cfa_register %esi
        pushl   %ebx
        pushl   %ebp
        andl    $-128, %esp
        subl    $128, %esp
        movl    %esp, %ebp        // ebp is the base pointer
        .cfi_offset %ebp, -16
        .cfi_offset %ebx, -12
        ...

• Use some other register not used by the inline asm as a temporary base pointer.

That register (other register) may also be clobbered in another inline asm. We need analyze all the inline asm of a function to choose a base pointer register.

• We use a different prologue that avoids the need for a base pointer. See discussion at https://github.com/llvm/llvm-project/issues/17204

It seems gcc generate special cfi information (cfi_escape) after realign stack. Don't know what ech cif_escape means. Could you write some psuedo code for the prologue?

[llvm]$ cat t.cpp
#include <alloca.h>
#include <string.h>
#include <stdint.h>
#include <stdlib.h>

extern int bar(void *p);
long long foo(size_t size, char c, int id) {
  __attribute__((__aligned__(128))) int a;
  char *p = (char *)alloca(size);
  asm volatile ("nop"::"c"(405):);
  asm volatile ("movl %0, %1"::"r"(id), "m"(a), "S"(405):);
  p[2] = 8;
  memset(p, c, size);
  return bar(p);
}

[llvm]$ g++ t.cpp -m32 -S -O2 -o -
        .file   "t.cpp"
        .text
        .p2align 4
        .globl  _Z3foojci
        .type   _Z3foojci, @function
_Z3foojci:
.LFB33:
        .cfi_startproc
        leal    4(%esp), %ecx
        .cfi_def_cfa 1, 0
        andl    $-128, %esp
        pushl   -4(%ecx)
        pushl   %ebp
        movl    %esp, %ebp
        .cfi_escape 0x10,0x5,0x2,0x75,0
        pushl   %edi
        pushl   %esi
        pushl   %ebx
        pushl   %ecx
        .cfi_escape 0xf,0x3,0x75,0x70,0x6
        .cfi_escape 0x10,0x7,0x2,0x75,0x7c
        .cfi_escape 0x10,0x6,0x2,0x75,0x78
        .cfi_escape 0x10,0x3,0x2,0x75,0x74

[efriedma]

Why it would mess unwinding?

I meant fp-based unwinding for profiling etc., not DWARF unwind.

That register (other register) may also be clobbered in another inline asm

We don't have to use the same register for each inline asm.

Don't know what ech cif_escape means

llvm-readobj --unwind can decode the DWARF for you.

The important thing is that it allocates a variable amount of space between the arguments and the frame pointer, and saves the incoming value of esp on the stack.

[LuoYuanke]

In D144541#4155710, @efriedma wrote:

Why it would mess unwinding?

I meant fp-based unwinding for profiling etc., not DWARF unwind.

That register (other register) may also be clobbered in another inline asm

We don't have to use the same register for each inline asm.

The base pointer register is reserved by compiler, we can't reserve too much register?

Don't know what ech cif_escape means

llvm-readobj --unwind can decode the DWARF for you.

The important thing is that it allocates a variable amount of space between the arguments and the frame pointer, and saves the incoming value of esp on the stack.

It seems this option is the best one. What do you think?

[efriedma]

In D144541#4157339, @LuoYuanke wrote:

In D144541#4155710, @efriedma wrote:

That register (other register) may also be clobbered in another inline asm

We don't have to use the same register for each inline asm.

The base pointer register is reserved by compiler, we can't reserve too much register?

You wouldn't reserve a register, just add some instructions around the inline asm to save/restore the base pointer. It's sort of messy, but probably workable.

Don't know what ech cif_escape means

llvm-readobj --unwind can decode the DWARF for you.

The important thing is that it allocates a variable amount of space between the arguments and the frame pointer, and saves the incoming value of esp on the stack.

It seems this option is the best one. What do you think?

This is the most straightforward/maintainable, probably.

[LuoYuanke]

This is the most straightforward/maintainable, probably.

I create https://reviews.llvm.org/D145650 for it, but it seems that is more complex and bug-prone. Since this patch fix part of the issue, can we land this patch first?

llvm/include/llvm/CodeGen/TargetLowering.h

if (ConstraintCode == "o")
       return InlineAsm::Constraint_o;
     if (ConstraintCode == "X")
       return InlineAsm::Constraint_X;
     if (ConstraintCode == "p")
       return InlineAsm::Constraint_p;
     return InlineAsm::Constraint_Unknown;
   }
 
+  virtual void lowerInlineAsmRegClobber(MachineFunction &MF,
+                                        Register Reg) const {
+    return;
+  }
+
   /// Try to replace an X constraint, which matches anything, with another that
   /// has more specific requirements based on the type of the corresponding
   /// operand.  This returns null if there is no replacement to make.
   virtual const char *LowerXConstraint(EVT ConstraintVT) const;
 
   /// Lower the specified operand into the Ops vector.  If it is invalid, don't
   /// add anything to Ops.
   virtual void LowerAsmOperandForConstraint(SDValue Op, std::string &Constraint,

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

auto DetectWriteToReservedRegister = [&]() {
           const char *RegName = TRI.getName(Reg);
           emitInlineAsmError(Call, "write to reserved register '" +
                                        Twine(RegName) + "'");
           return true;
         }
       }
       return false;
     };
+    auto DetectBasePtrRegister = [&]() {
+      MachineFunction &MF = DAG.getMachineFunction();
+      for (unsigned Reg : OpInfo.AssignedRegs.Regs) {
+        if (Register::isPhysicalRegister(Reg)) {
+          TLI.lowerInlineAsmRegClobber(MF, Reg);
+        }
+      }
+    };
     assert((OpInfo.ConstraintType != TargetLowering::C_Address ||
             (OpInfo.Type == InlineAsm::isInput &&
              !OpInfo.isMatchingInputConstraint())) &&
            "Only address as input operand is allowed.");
 
     switch (OpInfo.Type) {
     case InlineAsm::isOutput:
       if (OpInfo.ConstraintType == TargetLowering::C_Memory) {
case InlineAsm::isOutput:
           emitInlineAsmError(
               Call, "couldn't allocate output register for constraint '" +
                         Twine(OpInfo.ConstraintCode) + "'");
           return;
         }
 
         if (DetectWriteToReservedRegister())
           return;
+        DetectBasePtrRegister();
 
         // Add information to the INLINEASM node to know that this register is
         // set.
         OpInfo.AssignedRegs.AddInlineAsmOperands(
             OpInfo.isEarlyClobber ? InlineAsm::Kind_RegDefEarlyClobber
                                   : InlineAsm::Kind_RegDef,
             false, 0, getCurSDLoc(), DAG, AsmNodeOperands);
       }
case InlineAsm::isLabel: {
         emitInlineAsmError(Call,
                            "couldn't allocate input reg for constraint '" +
                                Twine(OpInfo.ConstraintCode) + "'");
         return;
       }
 
       if (DetectWriteToReservedRegister())
         return;
+      DetectBasePtrRegister();
 
       SDLoc dl = getCurSDLoc();
 
       OpInfo.AssignedRegs.getCopyToRegs(InOperandVal, DAG, dl, Chain, &Flag,
                                         &Call);
 
       OpInfo.AssignedRegs.AddInlineAsmOperands(InlineAsm::Kind_RegUse, false, 0,
                                                dl, DAG, AsmNodeOperands);
       break;
     }
     case InlineAsm::isClobber:
       // Add the clobbered value to the operand list, so that the register
       // allocator is aware that the physreg got clobbered.
-      if (!OpInfo.AssignedRegs.Regs.empty())
+      if (!OpInfo.AssignedRegs.Regs.empty()) {
         OpInfo.AssignedRegs.AddInlineAsmOperands(InlineAsm::Kind_Clobber,
                                                  false, 0, getCurSDLoc(), DAG,
                                                  AsmNodeOperands);
+        DetectBasePtrRegister();
+      }
       break;
     }
   }
 
   // Finish up input operands.  Set the input chain and add the flag last.
   AsmNodeOperands[InlineAsm::Op_InputChain] = Chain;
   if (Flag.getNode()) AsmNodeOperands.push_back(Flag);
 

llvm/lib/Target/X86/X86FrameLowering.h

private:
   unsigned getPSPSlotOffsetFromSP(const MachineFunction &MF) const;
 
   unsigned getWinEHFuncletFrameSize(const MachineFunction &MF) const;
 
   /// Materialize the catchret target MBB in RAX.
   void emitCatchRetReturnValue(MachineBasicBlock &MBB,
                                MachineBasicBlock::iterator MBBI,
                                MachineInstr *CatchRet) const;
+
+  /// Save and restore base pointer register around MI which clobber the
+  /// register.
+  void saveRestoreBasePointerReg(MachineFunction &MF) const;
 };
 
 } // End llvm namespace
 
 #endif

llvm/lib/Target/X86/X86FrameLowering.cpp

void X86FrameLowering::adjustFrameForMsvcCxxEh(MachineFunction &MF) const {
   DebugLoc DL = MBB.findDebugLoc(MBBI);
   addFrameReference(BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64mi32)),
                     UnwindHelpFI)
       .addImm(-2);
 }
 
 void X86FrameLowering::processFunctionBeforeFrameIndicesReplaced(
     MachineFunction &MF, RegScavenger *RS) const {
+  saveRestoreBasePointerReg(MF);
   if (STI.is32Bit() && MF.hasEHFunclets())
     restoreWinEHStackPointersInParent(MF);
 }
 
 void X86FrameLowering::restoreWinEHStackPointersInParent(
     MachineFunction &MF) const {
   // 32-bit functions have to restore stack pointers when control is transferred
   // back to the parent function. These blocks are identified as eh pads that
   // are not funclet entries.
   bool IsSEH = isAsynchronousEHPersonality(
       classifyEHPersonality(MF.getFunction().getPersonalityFn()));
   for (MachineBasicBlock &MBB : MF) {
     bool NeedsRestore = MBB.isEHPad() && !MBB.isEHFuncletEntry();
     if (NeedsRestore)
       restoreWin32EHStackPointers(MBB, MBB.begin(), DebugLoc(),
                                   /*RestoreSP=*/IsSEH);
   }
 }
+
+void X86FrameLowering::saveRestoreBasePointerReg(MachineFunction &MF) const {
+  X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
+  if (!TRI->hasBasePointer(MF))
+    return;
+
+  Register BasePtr = TRI->getBaseRegister();
+  for (MachineBasicBlock &MBB : MF) {
+    for (MachineBasicBlock::iterator MII = MBB.begin(); MII != MBB.end();
+         ++MII) {
+      if (!MII->isInlineAsm())
+        continue;
+      for (const MachineOperand &MO : MII->operands()) {
+        if (!MO.isReg())
+          continue;
+        Register Reg = MO.getReg();
+        if (!Register::isPhysicalRegister(Reg))
+          continue;
+        if (!TRI->isSuperOrSubRegisterEq(BasePtr, Reg))
+          continue;
+
+        MII++;
+        DebugLoc DL;
+        assert(X86FI->getRestoreBasePointer());
+        Register FramePtr = TRI->getFrameRegister(MF);
+        unsigned Opc = Uses64BitFramePtr ? X86::MOV64rm : X86::MOV32rm;
+        addRegOffset(BuildMI(MBB, MII, DL, TII.get(Opc), BasePtr), FramePtr,
+                     false, X86FI->getRestoreBasePointerOffset());
+      }
+    }
+  }
+}

llvm/lib/Target/X86/X86ISelLowering.h

public:
 
     unsigned
     getInlineAsmMemConstraint(StringRef ConstraintCode) const override {
       if (ConstraintCode == "v")
         return InlineAsm::Constraint_v;
       return TargetLowering::getInlineAsmMemConstraint(ConstraintCode);
     }
 
+    void lowerInlineAsmRegClobber(MachineFunction &MF,
+                                  Register Reg) const override;
+
     /// Handle Lowering flag assembly outputs.
     SDValue LowerAsmOutputForConstraint(SDValue &Chain, SDValue &Flag,
                                         const SDLoc &DL,
                                         const AsmOperandInfo &Constraint,
                                         SelectionDAG &DAG) const override;
 
     /// Given a physical register constraint
     /// (e.g. {edx}), return the register number and the register class for the

llvm/lib/Target/X86/X86ISelLowering.cpp

       Res.first = 0;
       Res.second = nullptr;
     }
   }
 
   return Res;
 }
 
+void X86TargetLowering::lowerInlineAsmRegClobber(MachineFunction &MF,
+                                                 Register Reg) const {
+  X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
+  const X86RegisterInfo *TRI = Subtarget.getRegisterInfo();
+  const X86RegisterInfo *RegInfo = Subtarget.getRegisterInfo();
+  if (!RegInfo->hasBasePointer(MF))
+    return;
+  Register BasePtr = RegInfo->getBaseRegister();
+  if (!TRI->isSuperOrSubRegisterEq(BasePtr, Reg))
+    return;
+  X86FI->setRestoreBasePointer(&MF);
+}
+
 bool X86TargetLowering::isIntDivCheap(EVT VT, AttributeList Attr) const {
   // Integer division on x86 is expensive. However, when aggressively optimizing
   // for code size, we prefer to use a div instruction, as it is usually smaller
   // than the alternative sequence.
   // The exception to this is vector division. Since x86 doesn't have vector
   // integer division, leaving the division as-is is a loss even in terms of
   // size, because it will have to be scalarized, while the alternative code
   // sequence can be performed in vector form.

llvm/test/CodeGen/X86/i386-baseptr.ll

 ; CHECK-LABEL: clobber_base:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    pushl %ebp
 ; CHECK-NEXT:    movl %esp, %ebp
 ; CHECK-NEXT:    pushl %esi
 ; CHECK-NEXT:    andl $-128, %esp
 ; CHECK-NEXT:    subl $128, %esp
 ; CHECK-NEXT:    movl %esp, %esi
+; CHECK-NEXT:    movl %esp, -16(%ebp)
 ; CHECK-NEXT:    calll helper@PLT
 ; CHECK-NEXT:    movl %esp, %ecx
 ; CHECK-NEXT:    leal 31(,%eax,4), %eax
 ; CHECK-NEXT:    andl $-32, %eax
 ; CHECK-NEXT:    movl %ecx, %edx
 ; CHECK-NEXT:    subl %eax, %edx
 ; CHECK-NEXT:    movl %edx, %esp
 ; CHECK-NEXT:    negl %eax
 ; CHECK-NEXT:    movl $405, %esi # imm = 0x195
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    movl -16(%ebp), %esi
 ; CHECK-NEXT:    movl $8, %edx
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    movl %edx, (%esi)
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:    movl $0, (%ecx,%eax)
 ; CHECK-NEXT:    leal -4(%ebp), %esp
 ; CHECK-NEXT:    popl %esi
 ; CHECK-NEXT:    popl %ebp

llvm/test/CodeGen/X86/x86-64-baseptr.ll

 ; CHECK-LABEL: clobber_base:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    pushq %rbp
 ; CHECK-NEXT:    movq %rsp, %rbp
 ; CHECK-NEXT:    pushq %rbx
 ; CHECK-NEXT:    andq $-128, %rsp
 ; CHECK-NEXT:    subq $128, %rsp
 ; CHECK-NEXT:    movq %rsp, %rbx
+; CHECK-NEXT:    movq %rsp, -48(%rbp)
 ; CHECK-NEXT:    callq helper@PLT
 ; CHECK-NEXT:    movq %rsp, %rcx
 ; CHECK-NEXT:    movl %eax, %eax
 ; CHECK-NEXT:    leaq 31(,%rax,4), %rax
 ; CHECK-NEXT:    andq $-32, %rax
 ; CHECK-NEXT:    movq %rcx, %rdx
 ; CHECK-NEXT:    subq %rax, %rdx
 ; CHECK-NEXT:    movq %rdx, %rsp
 ; CHECK-NEXT:    negq %rax
 ; CHECK-NEXT:    movl $405, %ebx # imm = 0x195
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    movq -48(%rbp), %rbx
 ; CHECK-NEXT:    movl $8, %edx
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    movl %edx, (%rbx)
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:    movl $0, (%rcx,%rax)
 ; CHECK-NEXT:    leaq -8(%rbp), %rsp
 ; CHECK-NEXT:    popq %rbx
 ; CHECK-NEXT:    popq %rbp
 ; CHECK-NEXT:    retq
 ;
 ; X32ABI-LABEL: clobber_base:
 ; X32ABI:       # %bb.0: # %entry
 ; X32ABI-NEXT:    pushq %rbp
 ; X32ABI-NEXT:    movl %esp, %ebp
 ; X32ABI-NEXT:    pushq %rbx
 ; X32ABI-NEXT:    andl $-128, %esp
 ; X32ABI-NEXT:    subl $128, %esp
 ; X32ABI-NEXT:    movl %esp, %ebx
+; X32ABI-NEXT:    movl %esp, -48(%ebp)
 ; X32ABI-NEXT:    callq helper@PLT
 ; X32ABI-NEXT:    # kill: def $eax killed $eax def $rax
 ; X32ABI-NEXT:    leal 31(,%rax,4), %eax
 ; X32ABI-NEXT:    andl $-32, %eax
 ; X32ABI-NEXT:    movl %esp, %ecx
 ; X32ABI-NEXT:    movl %ecx, %edx
 ; X32ABI-NEXT:    subl %eax, %edx
 ; X32ABI-NEXT:    negl %eax
 ; X32ABI-NEXT:    movl %edx, %esp
 ; X32ABI-NEXT:    movl $405, %ebx # imm = 0x195
 ; X32ABI-NEXT:    #APP
 ; X32ABI-NEXT:    nop
 ; X32ABI-NEXT:    #NO_APP
+; X32ABI-NEXT:    movl -48(%ebp), %ebx
 ; X32ABI-NEXT:    movl $8, %edx
 ; X32ABI-NEXT:    #APP
 ; X32ABI-NEXT:    movl %edx, (%ebx)
 ; X32ABI-NEXT:    #NO_APP
 ; X32ABI-NEXT:    movl $0, (%ecx,%eax)
 ; X32ABI-NEXT:    leal -8(%ebp), %esp
 ; X32ABI-NEXT:    popq %rbx
 ; X32ABI-NEXT:    popq %rbp