Skip base pointer usage if SP isn't touched by asm

[rnk]

Summary

Now we emit a fatal error if we needed to realign the stack and we had stack adjusting inline asm. We could try to do better by analyzing the inline asm constraints for conflicts with our chosen base register, but that is hard to do through the CodeGen / Target layers.

Fixes miscompiles in PR16830, but we now fail to compile some programs.

[rnk]

• Add a fatal error.

[rnk]

Ping. I almost forgot about this because I worked around it in llvm/lib/Support/Host.cpp.

[rnk]

• Analyze the inline asm for implicit defs and add them to clobbers
• Only force the use of esi when the inline asm actually clobbers esp

[sunfish]

Reading this patch, everything looks fine, though that's no guarantee that there aren't any unanticipated interactions.

[rnk]

Thanks!

What I anticipate is that someone somewhere will hit the fatal error path with:

• a stack realignment
• inline asm
• a dynamic alloca

... and complain. But that's OK, because it's better than a miscompile. :)

[rnk]

Closed by commit rL196876 (authored by @rnk).

llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h

class MachineFrameInfo {
   /// Whether the local object blob needs to be allocated together. If not,
   /// PEI should ignore the isPreAllocated flags on the stack objects and
   /// just allocate them normally.
   bool UseLocalStackAllocationBlock;
 
   /// Whether the "realign-stack" option is on.
   bool RealignOption;
 
+  /// True if the function includes inline assembly that adjusts the stack
+  /// pointer.
+  bool HasInlineAsmWithSPAdjust;
+
   const TargetFrameLowering *getFrameLowering() const;
 public:
     explicit MachineFrameInfo(const TargetMachine &TM, bool RealignOpt)
     : TM(TM), RealignOption(RealignOpt) {
     StackSize = NumFixedObjects = OffsetAdjustment = MaxAlignment = 0;
     HasVarSizedObjects = false;
     FrameAddressTaken = false;
     ReturnAddressTaken = false;
public:
   /// prolog/epilog code insertion.
   bool adjustsStack() const { return AdjustsStack; }
   void setAdjustsStack(bool V) { AdjustsStack = V; }
 
   /// hasCalls - Return true if the current function has any function calls.
   bool hasCalls() const { return HasCalls; }
   void setHasCalls(bool V) { HasCalls = V; }
 
+  /// Returns true if the function contains any stack-adjusting inline assembly.
+  bool hasInlineAsmWithSPAdjust() const { return HasInlineAsmWithSPAdjust; }
+  void setHasInlineAsmWithSPAdjust(bool B) { HasInlineAsmWithSPAdjust = B; }
+
   /// getMaxCallFrameSize - Return the maximum size of a call frame that must be
   /// allocated for an outgoing function call.  This is only available if
   /// CallFrameSetup/Destroy pseudo instructions are used by the target, and
   /// then only during or after prolog/epilog code insertion.
   ///
   unsigned getMaxCallFrameSize() const { return MaxCallFrameSize; }
   void setMaxCallFrameSize(unsigned S) { MaxCallFrameSize = S; }
 

llvm/trunk/include/llvm/CodeGen/MachineFunction.h

class MachineFunction {
 
   /// ExposesReturnsTwice - True if the function calls setjmp or related
   /// functions with attribute "returns twice", but doesn't have
   /// the attribute itself.
   /// This is used to limit optimizations which cannot reason
   /// about the control flow of such functions.
   bool ExposesReturnsTwice;
 
-  /// True if the function includes MS-style inline assembly.
-  bool HasMSInlineAsm;
+  /// True if the function includes any inline assembly.
+  bool HasInlineAsm;
 
   MachineFunction(const MachineFunction &) LLVM_DELETED_FUNCTION;
   void operator=(const MachineFunction&) LLVM_DELETED_FUNCTION;
 public:
   MachineFunction(const Function *Fn, const TargetMachine &TM,
                   unsigned FunctionNum, MachineModuleInfo &MMI,
                   GCModuleInfo* GMI);
   ~MachineFunction();
public:
   }
 
   /// setCallsSetJmp - Set a flag that indicates if there's a call to
   /// a "returns twice" function.
   void setExposesReturnsTwice(bool B) {
     ExposesReturnsTwice = B;
   }
 
-  /// Returns true if the function contains any MS-style inline assembly.
-  bool hasMSInlineAsm() const {
-    return HasMSInlineAsm;
+  /// Returns true if the function contains any inline assembly.
+  bool hasInlineAsm() const {
+    return HasInlineAsm;
   }
 
-  /// Set a flag that indicates that the function contains MS-style inline
-  /// assembly.
-  void setHasMSInlineAsm(bool B) {
-    HasMSInlineAsm = B;
+  /// Set a flag that indicates that the function contains inline assembly.
+  void setHasInlineAsm(bool B) {
+    HasInlineAsm = B;
   }
   
   /// getInfo - Keep track of various per-function pieces of information for
   /// backends that would like to do so.
   ///
   template<typename Ty>
   Ty *getInfo() {
     if (!MFInfo) {

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

else if (!Regs.empty() &&
     const MachineRegisterInfo &MRI = DAG.getMachineFunction().getRegInfo();
     const TargetRegisterClass *RC = MRI.getRegClass(Regs.front());
     Flag = InlineAsm::getFlagWordForRegClass(Flag, RC->getID());
   }
 
   SDValue Res = DAG.getTargetConstant(Flag, MVT::i32);
   Ops.push_back(Res);
 
+  unsigned SP = TLI.getStackPointerRegisterToSaveRestore();
   for (unsigned Value = 0, Reg = 0, e = ValueVTs.size(); Value != e; ++Value) {
     unsigned NumRegs = TLI.getNumRegisters(*DAG.getContext(), ValueVTs[Value]);
     MVT RegisterVT = RegVTs[Value];
     for (unsigned i = 0; i != NumRegs; ++i) {
       assert(Reg < Regs.size() && "Mismatch in # registers expected");
-      Ops.push_back(DAG.getRegister(Regs[Reg++], RegisterVT));
+      unsigned TheReg = Regs[Reg++];
+      Ops.push_back(DAG.getRegister(TheReg, RegisterVT));
+
+      // Notice if we clobbered the stack pointer.  Yes, inline asm can do this.
+      if (TheReg == SP && Code == InlineAsm::Kind_Clobber) {
+        MachineFrameInfo *MFI = DAG.getMachineFunction().getFrameInfo();
+        MFI->setHasInlineAsmWithSPAdjust(true);
+      }
     }
   }
 }
 
 void SelectionDAGBuilder::init(GCFunctionInfo *gfi, AliasAnalysis &aa,
                                const TargetLibraryInfo *li) {
   AA = &aa;
   GFI = gfi;

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
 
   if (UseMBPI && OptLevel != CodeGenOpt::None)
     FuncInfo->BPI = &getAnalysis<BranchProbabilityInfo>();
   else
     FuncInfo->BPI = 0;
 
   SDB->init(GFI, *AA, LibInfo);
 
-  MF->setHasMSInlineAsm(false);
+  MF->setHasInlineAsm(false);
+  MF->getFrameInfo()->setHasInlineAsmWithSPAdjust(false);
+
   SelectAllBasicBlocks(Fn);
 
   // If the first basic block in the function has live ins that need to be
   // copied into vregs, emit the copies into the top of the block before
   // emitting the code for the block.
   MachineBasicBlock *EntryMBB = MF->begin();
   RegInfo->EmitLiveInCopies(EntryMBB, TRI, TII);
 
for (unsigned i = 0, e = FuncInfo->ArgDbgValues.size(); i != e; ++i) {
     }
   }
 
   // Determine if there are any calls in this machine function.
   MachineFrameInfo *MFI = MF->getFrameInfo();
   for (MachineFunction::const_iterator I = MF->begin(), E = MF->end(); I != E;
        ++I) {
 
-    if (MFI->hasCalls() && MF->hasMSInlineAsm())
+    if (MFI->hasCalls() && MF->hasInlineAsm())
       break;
 
     const MachineBasicBlock *MBB = I;
     for (MachineBasicBlock::const_iterator II = MBB->begin(), IE = MBB->end();
          II != IE; ++II) {
       const MCInstrDesc &MCID = TM.getInstrInfo()->get(II->getOpcode());
       if ((MCID.isCall() && !MCID.isReturn()) ||
           II->isStackAligningInlineAsm()) {
         MFI->setHasCalls(true);
       }
-      if (II->isMSInlineAsm()) {
-        MF->setHasMSInlineAsm(true);
+      if (II->isInlineAsm()) {
+        MF->setHasInlineAsm(true);
       }
     }
   }
 
   // Determine if there is a call to setjmp in the machine function.
   MF->setExposesReturnsTwice(Fn.callsFunctionThatReturnsTwice());
 
   // Replace forward-declared registers with the registers containing

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

for (unsigned i = 1, e = Info.ParsedOperands.size(); i != e; ++i) {
         AsmStrRewrites.push_back(AsmRewrite(AOK_Output, Start, SymName.size()));
       } else {
         InputDecls.push_back(OpDecl);
         InputDeclsAddressOf.push_back(Operand->needAddressOf());
         InputConstraints.push_back(Operand->getConstraint().str());
         AsmStrRewrites.push_back(AsmRewrite(AOK_Input, Start, SymName.size()));
       }
     }
+
+    // Consider implicit defs to be clobbers.  Think of cpuid and push.
+    const uint16_t *ImpDefs = Desc.getImplicitDefs();
+    for (unsigned I = 0, E = Desc.getNumImplicitDefs(); I != E; ++I)
+      ClobberRegs.push_back(ImpDefs[I]);
   }
 
   // Set the number of Outputs and Inputs.
   NumOutputs = OutputDecls.size();
   NumInputs = InputDecls.size();
 
   // Set the unique clobbers.
   array_pod_sort(ClobberRegs.begin(), ClobberRegs.end());

llvm/trunk/lib/Target/X86/X86FrameLowering.cpp

 bool X86FrameLowering::hasFP(const MachineFunction &MF) const {
   const MachineFrameInfo *MFI = MF.getFrameInfo();
   const MachineModuleInfo &MMI = MF.getMMI();
   const TargetRegisterInfo *RegInfo = TM.getRegisterInfo();
 
   return (MF.getTarget().Options.DisableFramePointerElim(MF) ||
           RegInfo->needsStackRealignment(MF) ||
           MFI->hasVarSizedObjects() ||
-          MFI->isFrameAddressTaken() || MF.hasMSInlineAsm() ||
+          MFI->isFrameAddressTaken() || MFI->hasInlineAsmWithSPAdjust() ||
           MF.getInfo<X86MachineFunctionInfo>()->getForceFramePointer() ||
           MMI.callsUnwindInit() || MMI.callsEHReturn());
 }
 
 static unsigned getSUBriOpcode(unsigned IsLP64, int64_t Imm) {
   if (IsLP64) {
     if (isInt<8>(Imm))
       return X86::SUB64ri8;

llvm/trunk/lib/Target/X86/X86RegisterInfo.cpp

BitVector X86RegisterInfo::getReservedRegs(const MachineFunction &MF) const {
   if (hasBasePointer(MF)) {
     CallingConv::ID CC = MF.getFunction()->getCallingConv();
     const uint32_t* RegMask = getCallPreservedMask(CC);
     if (MachineOperand::clobbersPhysReg(RegMask, getBaseRegister()))
       report_fatal_error(
         "Stack realignment in presence of dynamic allocas is not supported with"
         "this calling convention.");
 
+    // FIXME: Do a proper analysis of the inline asm to see if it actually
+    // conflicts with the base register we chose.
+    if (MF.hasInlineAsm())
+      report_fatal_error("Stack realignment in presence of dynamic stack "
+                         "adjustments is not supported with inline assembly.");
+
     for (MCSubRegIterator I(getBaseRegister(), this, /*IncludeSelf=*/true);
          I.isValid(); ++I)
       Reserved.set(*I);
   }
 
   // Mark the segment registers as reserved.
   Reserved.set(X86::CS);
   Reserved.set(X86::SS);

 //===----------------------------------------------------------------------===//
 
 bool X86RegisterInfo::hasBasePointer(const MachineFunction &MF) const {
    const MachineFrameInfo *MFI = MF.getFrameInfo();
 
    if (!EnableBasePointer)
      return false;
 
-   // When we need stack realignment and there are dynamic allocas, we can't
-   // reference off of the stack pointer, so we reserve a base pointer.
-   //
-   // This is also true if the function contain MS-style inline assembly.  We
-   // do this because if any stack changes occur in the inline assembly, e.g.,
-   // "pusha", then any C local variable or C argument references in the
-   // inline assembly will be wrong because the SP is not properly tracked.
-   if ((needsStackRealignment(MF) && MFI->hasVarSizedObjects()) ||
-       MF.hasMSInlineAsm())
-     return true;
-
-   return false;
+   // When we need stack realignment, we can't address the stack from the frame
+   // pointer.  When we have dynamic allocas or stack-adjusting inline asm, we
+   // can't address variables from the stack pointer.  MS inline asm can
+   // reference locals while also adjusting the stack pointer.  When we can't
+   // use both the SP and the FP, we need a separate base pointer register.
+   bool CantUseFP = needsStackRealignment(MF);
+   bool CantUseSP =
+       MFI->hasVarSizedObjects() || MFI->hasInlineAsmWithSPAdjust();
+   return CantUseFP && CantUseSP;
 }
 
 bool X86RegisterInfo::canRealignStack(const MachineFunction &MF) const {
   if (MF.getFunction()->hasFnAttribute("no-realign-stack"))
     return false;
 
   const MachineFrameInfo *MFI = MF.getFrameInfo();
   const MachineRegisterInfo *MRI = &MF.getRegInfo();

llvm/trunk/test/CodeGen/X86/inline-asm-stack-realign.ll

+; RUN: not llc -march x86 < %s 2>&1 | FileCheck %s
+
+; We don't currently support realigning the stack and adjusting the stack
+; pointer in inline asm.  This commonly happens in MS inline assembly using
+; push and pop.
+
+; CHECK: Stack realignment in presence of dynamic stack adjustments is not supported with inline assembly
+
+define i32 @foo() {
+entry:
+  %r = alloca i32, align 16
+  store i32 -1, i32* %r, align 16
+  call void asm sideeffect inteldialect "push esi\0A\09xor esi, esi\0A\09mov dword ptr $0, esi\0A\09pop esi", "=*m,~{flags},~{esi},~{esp},~{dirflag},~{fpsr},~{flags}"(i32* %r)
+  %0 = load i32* %r, align 16
+  ret i32 %0
+}

llvm/trunk/test/CodeGen/X86/inline-asm-stack-realign2.ll

+; RUN: not llc -march x86 < %s 2>&1 | FileCheck %s
+
+; We don't currently support realigning the stack and adjusting the stack
+; pointer in inline asm.  This can even happen in GNU asm.
+
+; CHECK: Stack realignment in presence of dynamic stack adjustments is not supported with inline assembly
+
+define i32 @foo() {
+entry:
+  %r = alloca i32, align 16
+  store i32 -1, i32* %r, align 16
+  call void asm sideeffect "push %esi\0A\09xor %esi, %esi\0A\09mov %esi, $0\0A\09pop %esi", "=*m,~{flags},~{esi},~{esp},~{dirflag},~{fpsr},~{flags}"(i32* %r)
+  %0 = load i32* %r, align 16
+  ret i32 %0
+}

llvm/trunk/test/CodeGen/X86/ms-inline-asm.ll

 ; RUN: llc < %s -march=x86 -mcpu=core2 | FileCheck %s
 
 define i32 @t1() nounwind {
 entry:
   %0 = tail call i32 asm sideeffect inteldialect "mov eax, $1\0A\09mov $0, eax", "=r,r,~{eax},~{dirflag},~{fpsr},~{flags}"(i32 1) nounwind
   ret i32 %0
 ; CHECK: t1
-; CHECK: movl %esp, %ebp
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
 ; CHECK: mov eax, ecx
 ; CHECK: mov ecx, eax
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 }
 
 define void @t2() nounwind {
 entry:
   call void asm sideeffect inteldialect "mov eax, $$1", "~{eax},~{dirflag},~{fpsr},~{flags}"() nounwind
   ret void
 ; CHECK: t2
-; CHECK: movl %esp, %ebp
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
 ; CHECK: mov eax, 1
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 }
 
 define void @t3(i32 %V) nounwind {
 entry:
   %V.addr = alloca i32, align 4
   store i32 %V, i32* %V.addr, align 4
   call void asm sideeffect inteldialect "mov eax, DWORD PTR [$0]", "*m,~{eax},~{dirflag},~{fpsr},~{flags}"(i32* %V.addr) nounwind
   ret void
 ; CHECK: t3
-; CHECK: movl %esp, %ebp
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
 ; CHECK: mov eax, DWORD PTR {{[[esp]}}
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 }
 
 %struct.t18_type = type { i32, i32 }
 
 define i32 @t18() nounwind {
 entry:
   %foo = alloca %struct.t18_type, align 4
   %a = getelementptr inbounds %struct.t18_type* %foo, i32 0, i32 0
   store i32 1, i32* %a, align 4
   %b = getelementptr inbounds %struct.t18_type* %foo, i32 0, i32 1
   store i32 2, i32* %b, align 4
   call void asm sideeffect inteldialect "lea ebx, foo\0A\09mov eax, [ebx].0\0A\09mov [ebx].4, ecx", "~{eax},~{dirflag},~{fpsr},~{flags}"() nounwind
   %b1 = getelementptr inbounds %struct.t18_type* %foo, i32 0, i32 1
   %0 = load i32* %b1, align 4
   ret i32 %0
 ; CHECK: t18
-; CHECK: movl %esp, %ebp
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
 ; CHECK: lea ebx, foo
 ; CHECK: mov eax, [ebx].0
 ; CHECK: mov [ebx].4, ecx
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 }
 
 define void @t19_helper() nounwind {
 entry:
   ret void
 }
 
 define void @t19() nounwind {
 entry:
   call void asm sideeffect inteldialect "call $0", "r,~{dirflag},~{fpsr},~{flags}"(void ()* @t19_helper) nounwind
   ret void
 ; CHECK-LABEL: t19:
-; CHECK: movl %esp, %ebp
 ; CHECK: movl ${{_?}}t19_helper, %eax
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
 ; CHECK: call eax
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 }
 
 @results = global [2 x i32] [i32 3, i32 2], align 4
 
 define i32* @t30() nounwind ssp {
 entry:
   %res = alloca i32*, align 4
   call void asm sideeffect inteldialect "lea edi, dword ptr $0", "*m,~{edi},~{dirflag},~{fpsr},~{flags}"([2 x i32]* @results) nounwind
   call void asm sideeffect inteldialect "mov dword ptr $0, edi", "=*m,~{dirflag},~{fpsr},~{flags}"(i32** %res) nounwind
   %0 = load i32** %res, align 4
   ret i32* %0
 ; CHECK-LABEL: t30:
-; CHECK: movl %esp, %ebp
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
 ; CHECK: lea edi, dword ptr [{{_?}}results]
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
-; CHECK: mov dword ptr [esi], edi
+; CHECK: mov dword ptr [esp], edi
+; CHECK: .att_syntax
+; CHECK: {{## InlineAsm End|#NO_APP}}
+; CHECK: movl (%esp), %eax
+}
+
+; Stack realignment plus MS inline asm that does *not* adjust the stack is no
+; longer an error.
+
+define i32 @t31() {
+entry:
+  %val = alloca i32, align 16
+  store i32 -1, i32* %val, align 16
+  call void asm sideeffect inteldialect "mov dword ptr $0, esp", "=*m,~{dirflag},~{fpsr},~{flags}"(i32* %val) #1
+  %sp = load i32* %val, align 16
+  ret i32 %sp
+; CHECK-LABEL: t31:
+; CHECK: pushl %ebp
+; CHECK: movl %esp, %ebp
+; CHECK: andl $-16, %esp
+; CHECK: {{## InlineAsm Start|#APP}}
+; CHECK: .intel_syntax
+; CHECK: mov dword ptr [esp], esp
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
-; CHECK: movl (%esi), %eax
+; CHECK: movl (%esp), %eax
+; CHECK: ret
 }