[asan-asm-instrumentation] Fixed memory accesses with rbp as a base or an index register.

[ygorshenin]

Summary

Fixed memory accesses with rbp as a base or an index register.

[eugenis]

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

30

why do you need cassert? This file has used assertions before.

121

Can LocalFrameReg be part of RegisterContext?

332

Maybe RegisterContext::addBusyReg() accepting a register or an X86Operand, and then choose the frame register in InstrumentMemOperandPrologue?

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s

27

Hm, why is this test so complex? There are 4 memory accesses here => we emit prologue/epilogue 4 times, and it's not obvious which one is tested. Can you make it simpler? It looks like this test covers the new code, but I'm not 100% sure.

[ygorshenin]

Fixes.

[ygorshenin]

PTAL

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

30

And it uses them now. So, I thought, it's a right thing to include explicitly <cassert> and do not depend on an implicit inclusion.

121

Good point, thanks!

332

Good point, thanks!

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s

27

Done.

[eugenis]

LGTM w/ nits

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

81

I think these should start with a capital letter.

86

I'd prefer named constants (or an enum) instead of magic index constants 0, 1, 2.

99

This may add NoRegister to BusyRegs, better avoid it.

[ygorshenin]

Fixes.

[ygorshenin]

Many thanks!

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

81

Done.

86

Done.

99

Done.

[ygorshenin]

Closed by commit rL220283 (authored by @ygorshenin).

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetAsmParser.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/Support/CommandLine.h"
 #include <algorithm>
+#include <cassert>
+#include <vector>
 
 namespace llvm {
 namespace {
 
 static cl::opt<bool> ClAsanInstrumentAssembly(
     "asan-instrument-assembly",
     cl::desc("instrument assembly with AddressSanitizer checks"), cl::Hidden,
     cl::init(false));

 
 bool IsSmallMemAccess(unsigned AccessSize) { return AccessSize < 8; }
 
 std::string FuncName(unsigned AccessSize, bool IsWrite) {
   return std::string("__asan_report_") + (IsWrite ? "store" : "load") +
          utostr(AccessSize);
 }
 
+unsigned ChooseFrameRegister(const std::vector<unsigned> &BusyRegs) {
+  static const unsigned Candidates[] = { X86::RBP, X86::RAX, X86::RBX, X86::RCX,
+                                         X86::RDX, X86::RDI, X86::RSI };
+  for (unsigned Reg : Candidates) {
+    if (!std::count(BusyRegs.begin(), BusyRegs.end(), Reg))
+      return Reg;
+  }
+  return X86::NoRegister;
+}
+
 class X86AddressSanitizer : public X86AsmInstrumentation {
 public:
   struct RegisterContext {
     RegisterContext(unsigned AddressReg, unsigned ShadowReg,
                     unsigned ScratchReg)
         : AddressReg(AddressReg), ShadowReg(ShadowReg), ScratchReg(ScratchReg) {
     }
 
     unsigned addressReg(MVT::SimpleValueType VT) const {
       return getX86SubSuperRegister(AddressReg, VT);
     }
 
     unsigned shadowReg(MVT::SimpleValueType VT) const {
       return getX86SubSuperRegister(ShadowReg, VT);
     }
 
     unsigned scratchReg(MVT::SimpleValueType VT) const {
       return getX86SubSuperRegister(ScratchReg, VT);
     }
 
     const unsigned AddressReg;
     const unsigned ShadowReg;
     const unsigned ScratchReg;
   };
 
   X86AddressSanitizer(const MCSubtargetInfo &STI)
       : X86AsmInstrumentation(STI), RepPrefix(false), OrigSPOffset(0) {}
 
   virtual ~X86AddressSanitizer() {}
 
   // X86AsmInstrumentation implementation:
   virtual void InstrumentAndEmitInstruction(const MCInst &Inst,
                                             OperandVector &Operands,
                                             MCContext &Ctx,
                                             const MCInstrInfo &MII,
                                             MCStreamer &Out) override {
     InstrumentMOVS(Inst, Operands, Ctx, MII, Out);
     if (RepPrefix)
       EmitInstruction(Out, MCInstBuilder(X86::REP_PREFIX));
 
     InstrumentMOV(Inst, Operands, Ctx, MII, Out);
 
     RepPrefix = (Inst.getOpcode() == X86::REP_PREFIX);
     if (!RepPrefix)
       EmitInstruction(Out, Inst);
   }
 
   // Adjusts up stack and saves all registers used in instrumentation.
-  virtual void InstrumentMemOperandPrologue(const RegisterContext &RegCtx,
+  virtual void InstrumentMemOperandPrologue(unsigned LocalFrameReg,
+                                            const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) = 0;
 
   // Restores all registers used in instrumentation and adjusts stack.
-  virtual void InstrumentMemOperandEpilogue(const RegisterContext &RegCtx,
+  virtual void InstrumentMemOperandEpilogue(unsigned LocalFrameReg,
+                                            const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) = 0;
 
   virtual void InstrumentMemOperandSmall(X86Operand &Op, unsigned AccessSize,
                                          bool IsWrite,
                                          const RegisterContext &RegCtx,
                                          MCContext &Ctx, MCStreamer &Out) = 0;
   virtual void InstrumentMemOperandLarge(X86Operand &Op, unsigned AccessSize,
void X86AddressSanitizer::InstrumentMOVSBase(unsigned DstReg, unsigned SrcReg,
                                              MCContext &Ctx, MCStreamer &Out) {
   // FIXME: check whole ranges [DstReg .. DstReg + AccessSize * (CntReg - 1)]
   // and [SrcReg .. SrcReg + AccessSize * (CntReg - 1)].
   RegisterContext RegCtx(X86::RDX /* AddressReg */, X86::RAX /* ShadowReg */,
                          IsSmallMemAccess(AccessSize)
                              ? X86::RBX
                              : X86::NoRegister /* ScratchReg */);
 
-  InstrumentMemOperandPrologue(RegCtx, Ctx, Out);
+  unsigned LocalFrameReg = ChooseFrameRegister(
+      std::vector<unsigned>{ X86::RDX, X86::RAX, X86::RBX,
+                             getX86SubSuperRegister(DstReg, MVT::i64),
+                             getX86SubSuperRegister(SrcReg, MVT::i64),
+                             getX86SubSuperRegister(CntReg, MVT::i64) });
+  assert(LocalFrameReg != X86::NoRegister);
+  InstrumentMemOperandPrologue(LocalFrameReg, RegCtx, Ctx, Out);
 
   // Test (%SrcReg)
   {
     const MCExpr *Disp = MCConstantExpr::Create(0, Ctx);
     std::unique_ptr<X86Operand> Op(X86Operand::CreateMem(
         0, Disp, SrcReg, 0, AccessSize, SMLoc(), SMLoc()));
     InstrumentMemOperand(*Op, AccessSize, false /* IsWrite */, RegCtx, Ctx,
                          Out);
void X86AddressSanitizer::InstrumentMOVSBase(unsigned DstReg, unsigned SrcReg,
   // Test -1(%DstReg, %CntReg, AccessSize)
   {
     const MCExpr *Disp = MCConstantExpr::Create(-1, Ctx);
     std::unique_ptr<X86Operand> Op(X86Operand::CreateMem(
         0, Disp, DstReg, CntReg, AccessSize, SMLoc(), SMLoc()));
     InstrumentMemOperand(*Op, AccessSize, true /* IsWrite */, RegCtx, Ctx, Out);
   }
 
-  InstrumentMemOperandEpilogue(RegCtx, Ctx, Out);
+  InstrumentMemOperandEpilogue(LocalFrameReg, RegCtx, Ctx, Out);
 }
 
 void X86AddressSanitizer::InstrumentMOVS(const MCInst &Inst,
                                          OperandVector &Operands,
                                          MCContext &Ctx, const MCInstrInfo &MII,
                                          MCStreamer &Out) {
   // Access size in bytes.
   unsigned AccessSize = 0;
RegisterContext RegCtx(X86::RDI /* AddressReg */, X86::RAX /* ShadowReg */,
                              ? X86::RCX
                              : X86::NoRegister /* ScratchReg */);
 
   for (unsigned Ix = 0; Ix < Operands.size(); ++Ix) {
     assert(Operands[Ix]);
     MCParsedAsmOperand &Op = *Operands[Ix];
     if (Op.isMem()) {
       X86Operand &MemOp = static_cast<X86Operand &>(Op);
-      InstrumentMemOperandPrologue(RegCtx, Ctx, Out);
+      std::vector<unsigned> BusyRegs = { RegCtx.AddressReg, RegCtx.ShadowReg,
+                                         RegCtx.ScratchReg };
+      if (MemOp.getMemBaseReg() != X86::NoRegister) {
+        BusyRegs.push_back(
+            getX86SubSuperRegister(MemOp.getMemBaseReg(), MVT::i64));
+      }
+      if (MemOp.getMemIndexReg() != X86::NoRegister) {
+        BusyRegs.push_back(
+            getX86SubSuperRegister(MemOp.getMemIndexReg(), MVT::i64));
+      }
+      unsigned LocalFrameReg = ChooseFrameRegister(BusyRegs);
+      assert(LocalFrameReg != X86::NoRegister);
+      InstrumentMemOperandPrologue(LocalFrameReg, RegCtx, Ctx, Out);
       InstrumentMemOperand(MemOp, AccessSize, IsWrite, RegCtx, Ctx, Out);
-      InstrumentMemOperandEpilogue(RegCtx, Ctx, Out);
+      InstrumentMemOperandEpilogue(LocalFrameReg, RegCtx, Ctx, Out);
     }
   }
 }
 
 void X86AddressSanitizer::ComputeMemOperandAddress(X86Operand &Op,
                                                    MVT::SimpleValueType VT,
                                                    unsigned Reg, MCContext &Ctx,
                                                    MCStreamer &Out) {
void StoreFlags(MCStreamer &Out) {
     OrigSPOffset -= 4;
   }
 
   void RestoreFlags(MCStreamer &Out) {
     EmitInstruction(Out, MCInstBuilder(X86::POPF32));
     OrigSPOffset += 4;
   }
 
-  virtual void InstrumentMemOperandPrologue(const RegisterContext &RegCtx,
+  virtual void InstrumentMemOperandPrologue(unsigned LocalFrameReg,
+                                            const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) override {
+    LocalFrameReg = getX86SubSuperRegister(LocalFrameReg, MVT::i32);
+
     const MCRegisterInfo *MRI = Ctx.getRegisterInfo();
     unsigned FrameReg = GetFrameReg(Ctx, Out);
     if (MRI && FrameReg != X86::NoRegister) {
-      SpillReg(Out, X86::EBP);
+      SpillReg(Out, LocalFrameReg);
       if (FrameReg == X86::ESP) {
-        Out.EmitCFIAdjustCfaOffset(4 /* byte size of the FrameReg */);
-        Out.EmitCFIRelOffset(MRI->getDwarfRegNum(X86::EBP, true /* IsEH */), 0);
+        Out.EmitCFIAdjustCfaOffset(4 /* byte size of the LocalFrameReg */);
+        Out.EmitCFIRelOffset(
+            MRI->getDwarfRegNum(LocalFrameReg, true /* IsEH */), 0);
       }
       EmitInstruction(
-          Out, MCInstBuilder(X86::MOV32rr).addReg(X86::EBP).addReg(FrameReg));
+          Out,
+          MCInstBuilder(X86::MOV32rr).addReg(LocalFrameReg).addReg(FrameReg));
       Out.EmitCFIRememberState();
-      Out.EmitCFIDefCfaRegister(MRI->getDwarfRegNum(X86::EBP, true /* IsEH */));
+      Out.EmitCFIDefCfaRegister(
+          MRI->getDwarfRegNum(LocalFrameReg, true /* IsEH */));
     }
 
     SpillReg(Out, RegCtx.addressReg(MVT::i32));
     SpillReg(Out, RegCtx.shadowReg(MVT::i32));
     if (RegCtx.ScratchReg != X86::NoRegister)
       SpillReg(Out, RegCtx.scratchReg(MVT::i32));
     StoreFlags(Out);
   }
 
-  virtual void InstrumentMemOperandEpilogue(const RegisterContext &RegCtx,
+  virtual void InstrumentMemOperandEpilogue(unsigned LocalFrameReg,
+                                            const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) override {
+    LocalFrameReg = getX86SubSuperRegister(LocalFrameReg, MVT::i32);
+
     RestoreFlags(Out);
     if (RegCtx.ScratchReg != X86::NoRegister)
       RestoreReg(Out, RegCtx.scratchReg(MVT::i32));
     RestoreReg(Out, RegCtx.shadowReg(MVT::i32));
     RestoreReg(Out, RegCtx.addressReg(MVT::i32));
 
     unsigned FrameReg = GetFrameReg(Ctx, Out);
     if (Ctx.getRegisterInfo() && FrameReg != X86::NoRegister) {
-      RestoreReg(Out, X86::EBP);
+      RestoreReg(Out, LocalFrameReg);
       Out.EmitCFIRestoreState();
       if (FrameReg == X86::ESP)
-        Out.EmitCFIAdjustCfaOffset(-4 /* byte size of the FrameReg */);
+        Out.EmitCFIAdjustCfaOffset(-4 /* byte size of the LocalFrameReg */);
     }
   }
 
   virtual void InstrumentMemOperandSmall(X86Operand &Op, unsigned AccessSize,
                                          bool IsWrite,
                                          const RegisterContext &RegCtx,
                                          MCContext &Ctx,
                                          MCStreamer &Out) override;
void StoreFlags(MCStreamer &Out) {
     OrigSPOffset -= 8;
   }
 
   void RestoreFlags(MCStreamer &Out) {
     EmitInstruction(Out, MCInstBuilder(X86::POPF64));
     OrigSPOffset += 8;
   }
 
-  virtual void InstrumentMemOperandPrologue(const RegisterContext &RegCtx,
+  virtual void InstrumentMemOperandPrologue(unsigned LocalFrameReg,
+                                            const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) override {
+    LocalFrameReg = getX86SubSuperRegister(LocalFrameReg, MVT::i64);
+
     const MCRegisterInfo *MRI = Ctx.getRegisterInfo();
     unsigned FrameReg = GetFrameReg(Ctx, Out);
     if (MRI && FrameReg != X86::NoRegister) {
       SpillReg(Out, X86::RBP);
       if (FrameReg == X86::RSP) {
-        Out.EmitCFIAdjustCfaOffset(8 /* byte size of the FrameReg */);
-        Out.EmitCFIRelOffset(MRI->getDwarfRegNum(X86::RBP, true /* IsEH */), 0);
+        Out.EmitCFIAdjustCfaOffset(8 /* byte size of the LocalFrameReg */);
+        Out.EmitCFIRelOffset(
+            MRI->getDwarfRegNum(LocalFrameReg, true /* IsEH */), 0);
       }
       EmitInstruction(
-          Out, MCInstBuilder(X86::MOV64rr).addReg(X86::RBP).addReg(FrameReg));
+          Out,
+          MCInstBuilder(X86::MOV64rr).addReg(LocalFrameReg).addReg(FrameReg));
       Out.EmitCFIRememberState();
-      Out.EmitCFIDefCfaRegister(MRI->getDwarfRegNum(X86::RBP, true /* IsEH */));
+      Out.EmitCFIDefCfaRegister(
+          MRI->getDwarfRegNum(LocalFrameReg, true /* IsEH */));
     }
 
     EmitAdjustRSP(Ctx, Out, -128);
     SpillReg(Out, RegCtx.shadowReg(MVT::i64));
     SpillReg(Out, RegCtx.addressReg(MVT::i64));
     if (RegCtx.ScratchReg != X86::NoRegister)
       SpillReg(Out, RegCtx.scratchReg(MVT::i64));
     StoreFlags(Out);
   }
 
-  virtual void InstrumentMemOperandEpilogue(const RegisterContext &RegCtx,
+  virtual void InstrumentMemOperandEpilogue(unsigned LocalFrameReg,
+                                            const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) override {
+    LocalFrameReg = getX86SubSuperRegister(LocalFrameReg, MVT::i64);
+
     RestoreFlags(Out);
     if (RegCtx.ScratchReg != X86::NoRegister)
       RestoreReg(Out, RegCtx.scratchReg(MVT::i64));
     RestoreReg(Out, RegCtx.addressReg(MVT::i64));
     RestoreReg(Out, RegCtx.shadowReg(MVT::i64));
     EmitAdjustRSP(Ctx, Out, 128);
 
     unsigned FrameReg = GetFrameReg(Ctx, Out);
     if (Ctx.getRegisterInfo() && FrameReg != X86::NoRegister) {
-      RestoreReg(Out, X86::RBP);
+      RestoreReg(Out, LocalFrameReg);
       Out.EmitCFIRestoreState();
       if (FrameReg == X86::RSP)
-        Out.EmitCFIAdjustCfaOffset(-8 /* byte size of the FrameReg */);
+        Out.EmitCFIAdjustCfaOffset(-8 /* byte size of the LocalFrameReg */);
     }
   }
 
   virtual void InstrumentMemOperandSmall(X86Operand &Op, unsigned AccessSize,
                                          bool IsWrite,
                                          const RegisterContext &RegCtx,
                                          MCContext &Ctx,
                                          MCStreamer &Out) override;

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s

 # The test verifies that correct DWARF directives are emitted when
 # assembly files are instrumented.
 
 # RUN: llvm-mc %s -triple=i386-unknown-linux-gnu -asm-instrumentation=address -asan-instrument-assembly | FileCheck %s
 
 # CHECK-LABEL: swap_cfa_rbp
-# CHECK: pushl %ebp
+# CHECK: pushl %ebx
 # CHECK-NOT: .cfi_adjust_cfa_offset 8
-# CHECK: movl %ebp, %ebp
+# CHECK: movl %ebp, %ebx
 # CHECK: .cfi_remember_state
-# CHECK: .cfi_def_cfa_register %ebp
-# CHECK: popl %ebp
+# CHECK: .cfi_def_cfa_register %ebx
+# CHECK: popl %ebx
 # CHECK: .cfi_restore_state
 # CHECK-NOT: .cfi_adjust_cfa_offset -8
 # CHECK: retl
 
   .text
   .globl  swap_cfa_rbp
   .type swap_cfa_rbp,@function
 swap_cfa_rbp:                                   # @swap_cfa_rbp
   .cfi_startproc
   pushl %ebp
   .cfi_def_cfa_offset 8
   .cfi_offset %ebp, -8
   movl  %esp, %ebp
   .cfi_def_cfa_register %ebp
   movl  8(%ebp), %eax
   movl  12(%ebp), %ecx
   movl  (%ecx), %ecx
   movl  %ecx, (%eax)
   popl  %ebp
   retl
   .cfi_endproc
 
 # CHECK-LABEL: swap_cfa_rsp
-# CHECK: pushl %ebp
+# CHECK: pushl %ebx
 # CHECK: .cfi_adjust_cfa_offset 4
-# CHECK: movl %esp, %ebp
+# CHECK: movl %esp, %ebx
 # CHECK: .cfi_remember_state
-# CHECK: .cfi_def_cfa_register %ebp
-# CHECK: popl %ebp
+# CHECK: .cfi_def_cfa_register %ebx
+# CHECK: popl %ebx
 # CHECK: .cfi_restore_state
 # CHECK: retl
 
   .globl  swap_cfa_rsp
   .type swap_cfa_rsp,@function
 swap_cfa_rsp:                                   # @swap_cfa_rsp
   .cfi_startproc
   pushl %ebp