[asan-asm-instrumentation] CFI directives are generated for .S files.

[ygorshenin]

Summary

CFI directives are generated for .S files.

[ygorshenin]

asm_cfi.s file is added to git index.

[eugenis]

Please don't forget about a compiler-rt test (either in this or in a separate commit).

lib/MC/MCStreamer.cpp

279

What about MCStreamer::EmitCFIDefCfa?

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

579

This is shadowing X86AsmInstrumentation::FrameReg. Confusing.
Perhaps rename the other one to smth like "InitialFrameReg"?

850

Please check (and test) that all this stuff works with -fno-unwind-tables.

868

Is it even possible? Can we CHECK instead?
Is it the same situation as "No active dwarf frame" above?

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s

2

Please add at least a one-line comment on what this test is about.

[ygorshenin]

Fixes.

[ygorshenin]

PTAL

lib/MC/MCStreamer.cpp

279

Done.

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

579

That's why I moved FrameReg to the private part of the X86AsmInstrumentation and there're no shadowing. But if you still insisting I'll rename X86AsmInstrumentation::FrameReg to something like InitialFrameReg.

850

Sure, but could you please explain what -fno-unwind-tables does?

868

MCDwarfFrameInfo::CfaRegister is set when something like .cfi_def_cfa_register was emitted. Otherwise it won't be set (even between .cfi_startproc and .cfi_endproc).

test/Instrumentation/AddressSanitizer/X86/asm_cfi.s

2

Done.

[ygorshenin]

Fixes.

[eugenis]

D5547 is good, but I'd also like a line in asm_cfi.ll test that checks that we don't add any CFI instructions if there is no .cfi_startproc (i.e. unwind tables are off).

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

579

Yes, please rename.

868

And this makes MCDwarfFrameInfo::CfaRegister unreliable, which is not so good.
There are target-dependent initial CFI instructions, see MCAsmInfo::addInitialFrameState. Could you make CfaRegister take them into account?

[ygorshenin]

Fixes.

[ygorshenin]

PTAL

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

579

Done.

850

Done in http://reviews.llvm.org/D5547.

868

Done.

[eugenis]

LGTM, but I'd like someone else to vet the changes to MCStreamer / MCDwarf.

lib/MC/MCStreamer.cpp

223

I don't really like that we do this extra work (a) for each function and (b) even when there is no instrumentation and CurrentCfaRegister is unused. But all alternatives seem much more complex, and this loop typically has just one or two iterations, so maybe it's not worth optimizing.

[ygorshenin]

Rafael, could you please take a look?

[• rafael]

LGTM with a nit.

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

861

The X86::NoRegister in here is the 0 in CurrentCfaRegister(0), no? I would probably be cleaner no just use 0 everywhere.

This could also be an assert now, no? The initial instructions should always be setting the initial frame register.

[ygorshenin]

Fixes.

[ygorshenin]

Many thanks, Eugene and Rafael!

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

861

Done.

[ygorshenin]

Closed by commit rL219199 (authored by @ygorshenin).

include/llvm/MC/MCDwarf.h

StringRef getValues() const {
     assert(Operation == OpEscape);
     return StringRef(&Values[0], Values.size());
   }
 };
 
 struct MCDwarfFrameInfo {
   MCDwarfFrameInfo()
       : Begin(nullptr), End(nullptr), Personality(nullptr), Lsda(nullptr),
-        Instructions(), PersonalityEncoding(), LsdaEncoding(0),
+        Instructions(), CfaRegister(0), PersonalityEncoding(), LsdaEncoding(0),
         CompactUnwindEncoding(0), IsSignalFrame(false), IsSimple(false) {}
   MCSymbol *Begin;
   MCSymbol *End;
   const MCSymbol *Personality;
   const MCSymbol *Lsda;
   std::vector<MCCFIInstruction> Instructions;
+  unsigned CfaRegister;
   unsigned PersonalityEncoding;
   unsigned LsdaEncoding;
   uint32_t CompactUnwindEncoding;
   bool IsSignalFrame;
   bool IsSimple;
 };
 
 class MCDwarfFrameEmitter {

lib/MC/MCStreamer.cpp

void MCStreamer::EmitCFIStartProc(bool IsSimple) {
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   if (CurFrame && !CurFrame->End)
     report_fatal_error("Starting a frame before finishing the previous one!");
 
   MCDwarfFrameInfo Frame;
   Frame.IsSimple = IsSimple;
   EmitCFIStartProcImpl(Frame);
 
   DwarfFrameInfos.push_back(Frame);
 }
 
 void MCStreamer::EmitCFIStartProcImpl(MCDwarfFrameInfo &Frame) {
 }
 
 void MCStreamer::EmitCFIEndProc() {
   EnsureValidDwarfFrame();
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();

 }
 
 void MCStreamer::EmitCFIDefCfa(int64_t Register, int64_t Offset) {
   MCSymbol *Label = EmitCFICommon();
   MCCFIInstruction Instruction =
     MCCFIInstruction::createDefCfa(Label, Register, Offset);
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   CurFrame->Instructions.push_back(Instruction);
+  CurFrame->CfaRegister = static_cast<unsigned>(Register);
 }
 
 void MCStreamer::EmitCFIDefCfaOffset(int64_t Offset) {
   MCSymbol *Label = EmitCFICommon();
   MCCFIInstruction Instruction =
     MCCFIInstruction::createDefCfaOffset(Label, Offset);
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   CurFrame->Instructions.push_back(Instruction);
 }
 
 void MCStreamer::EmitCFIAdjustCfaOffset(int64_t Adjustment) {
   MCSymbol *Label = EmitCFICommon();
   MCCFIInstruction Instruction =
     MCCFIInstruction::createAdjustCfaOffset(Label, Adjustment);
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   CurFrame->Instructions.push_back(Instruction);
 }
 
 void MCStreamer::EmitCFIDefCfaRegister(int64_t Register) {
   MCSymbol *Label = EmitCFICommon();
   MCCFIInstruction Instruction =
     MCCFIInstruction::createDefCfaRegister(Label, Register);
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   CurFrame->Instructions.push_back(Instruction);
+  CurFrame->CfaRegister = static_cast<unsigned>(Register);
 }
 
 void MCStreamer::EmitCFIOffset(int64_t Register, int64_t Offset) {
   MCSymbol *Label = EmitCFICommon();
   MCCFIInstruction Instruction =
     MCCFIInstruction::createOffset(Label, Register, Offset);
   MCDwarfFrameInfo *CurFrame = getCurrentDwarfFrameInfo();
   CurFrame->Instructions.push_back(Instruction);

lib/Target/X86/AsmParser/X86AsmInstrumentation.h

 X86AsmInstrumentation *
 CreateX86AsmInstrumentation(const MCTargetOptions &MCOptions,
                             const MCContext &Ctx, const MCSubtargetInfo &STI);
 
 class X86AsmInstrumentation {
 public:
   virtual ~X86AsmInstrumentation();
 
+  // Sets frame register corresponding to a current frame.
   void SetFrameRegister(unsigned RegNo) {
     FrameReg = RegNo;
   }
 
   // Tries to instrument and emit instruction.
   virtual void InstrumentAndEmitInstruction(
       const MCInst &Inst,
       SmallVectorImpl<std::unique_ptr<MCParsedAsmOperand> > &Operands,
       MCContext &Ctx, const MCInstrInfo &MII, MCStreamer &Out);
 
 protected:
   friend X86AsmInstrumentation *
   CreateX86AsmInstrumentation(const MCTargetOptions &MCOptions,
                               const MCContext &Ctx, const MCSubtargetInfo &STI);
 
   X86AsmInstrumentation(const MCSubtargetInfo &STI);
 
+  unsigned GetFrameRegGeneric(const MCContext &Ctx, MCStreamer &Out);
+
   void EmitInstruction(MCStreamer &Out, const MCInst &Inst);
 
   const MCSubtargetInfo &STI;
 
   unsigned FrameReg;
 };
 
 } // End llvm namespace
 
 #endif

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp

 public:
   static const long kShadowOffset = 0x20000000;
 
   X86AddressSanitizer32(const MCSubtargetInfo &STI)
       : X86AddressSanitizer(STI) {}
 
   virtual ~X86AddressSanitizer32() {}
 
+  unsigned GetFrameReg(const MCContext &Ctx, MCStreamer &Out) {
+    return getX86SubSuperRegister(GetFrameRegGeneric(Ctx, Out), MVT::i32);
+  }
+
   virtual void StoreFlags(MCStreamer &Out) override {
     EmitInstruction(Out, MCInstBuilder(X86::PUSHF32));
   }
 
   virtual void RestoreFlags(MCStreamer &Out) override {
     EmitInstruction(Out, MCInstBuilder(X86::POPF32));
   }
 
   virtual void InstrumentMemOperandPrologue(const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) override {
-    const MCRegisterInfo* MRI = Ctx.getRegisterInfo();
+    const MCRegisterInfo *MRI = Ctx.getRegisterInfo();
+    unsigned FrameReg = GetFrameReg(Ctx, Out);
     if (MRI && FrameReg != X86::NoRegister) {
       EmitInstruction(
           Out, MCInstBuilder(X86::PUSH32r).addReg(X86::EBP));
       if (FrameReg == X86::ESP) {
         Out.EmitCFIAdjustCfaOffset(4 /* byte size of the FrameReg */);
         Out.EmitCFIRelOffset(
             MRI->getDwarfRegNum(X86::EBP, true /* IsEH */), 0);
       }
if (RegCtx.ScratchReg != X86::NoRegister) {
       EmitInstruction(
           Out, MCInstBuilder(X86::POP32r).addReg(RegCtx.scratchReg(MVT::i32)));
     }
     EmitInstruction(
         Out, MCInstBuilder(X86::POP32r).addReg(RegCtx.shadowReg(MVT::i32)));
     EmitInstruction(
         Out, MCInstBuilder(X86::POP32r).addReg(RegCtx.addressReg(MVT::i32)));
 
+    unsigned FrameReg = GetFrameReg(Ctx, Out);
     if (Ctx.getRegisterInfo() && FrameReg != X86::NoRegister) {
       EmitInstruction(
           Out, MCInstBuilder(X86::POP32r).addReg(X86::EBP));
       Out.EmitCFIRestoreState();
       if (FrameReg == X86::ESP)
         Out.EmitCFIAdjustCfaOffset(-4 /* byte size of the FrameReg */);
     }
   }

 public:
   static const long kShadowOffset = 0x7fff8000;
 
   X86AddressSanitizer64(const MCSubtargetInfo &STI)
       : X86AddressSanitizer(STI) {}
 
   virtual ~X86AddressSanitizer64() {}
 
+  unsigned GetFrameReg(const MCContext &Ctx, MCStreamer &Out) {
+    return getX86SubSuperRegister(GetFrameRegGeneric(Ctx, Out), MVT::i64);
+  }
+
   virtual void StoreFlags(MCStreamer &Out) override {
     EmitInstruction(Out, MCInstBuilder(X86::PUSHF64));
   }
 
   virtual void RestoreFlags(MCStreamer &Out) override {
     EmitInstruction(Out, MCInstBuilder(X86::POPF64));
   }
 
   virtual void InstrumentMemOperandPrologue(const RegisterContext &RegCtx,
                                             MCContext &Ctx,
                                             MCStreamer &Out) override {
-    const MCRegisterInfo *RegisterInfo = Ctx.getRegisterInfo();
-    if (RegisterInfo && FrameReg != X86::NoRegister) {
+    const MCRegisterInfo *MRI = Ctx.getRegisterInfo();
+    unsigned FrameReg = GetFrameReg(Ctx, Out);
+    if (MRI && FrameReg != X86::NoRegister) {
       EmitInstruction(Out, MCInstBuilder(X86::PUSH64r).addReg(X86::RBP));
       if (FrameReg == X86::RSP) {
         Out.EmitCFIAdjustCfaOffset(8 /* byte size of the FrameReg */);
         Out.EmitCFIRelOffset(
-            RegisterInfo->getDwarfRegNum(X86::RBP, true /* IsEH */), 0);
+            MRI->getDwarfRegNum(X86::RBP, true /* IsEH */), 0);
       }
       EmitInstruction(
           Out, MCInstBuilder(X86::MOV64rr).addReg(X86::RBP).addReg(FrameReg));
       Out.EmitCFIRememberState();
       Out.EmitCFIDefCfaRegister(
-          RegisterInfo->getDwarfRegNum(X86::RBP, true /* IsEH */));
+          MRI->getDwarfRegNum(X86::RBP, true /* IsEH */));
     }
 
     EmitAdjustRSP(Ctx, Out, -128);
     EmitInstruction(
         Out, MCInstBuilder(X86::PUSH64r).addReg(RegCtx.shadowReg(MVT::i64)));
     EmitInstruction(
         Out, MCInstBuilder(X86::PUSH64r).addReg(RegCtx.addressReg(MVT::i64)));
     if (RegCtx.ScratchReg != X86::NoRegister) {
if (RegCtx.ScratchReg != X86::NoRegister) {
           Out, MCInstBuilder(X86::POP64r).addReg(RegCtx.scratchReg(MVT::i64)));
     }
     EmitInstruction(
         Out, MCInstBuilder(X86::POP64r).addReg(RegCtx.addressReg(MVT::i64)));
     EmitInstruction(
         Out, MCInstBuilder(X86::POP64r).addReg(RegCtx.shadowReg(MVT::i64)));
     EmitAdjustRSP(Ctx, Out, 128);
 
+    unsigned FrameReg = GetFrameReg(Ctx, Out);
     if (Ctx.getRegisterInfo() && FrameReg != X86::NoRegister) {
       EmitInstruction(
           Out, MCInstBuilder(X86::POP64r).addReg(X86::RBP));
       Out.EmitCFIRestoreState();
       if (FrameReg == X86::RSP)
         Out.EmitCFIAdjustCfaOffset(-8 /* byte size of the FrameReg */);
     }
   }
void X86AsmInstrumentation::InstrumentAndEmitInstruction(
   EmitInstruction(Out, Inst);
 }
 
 void X86AsmInstrumentation::EmitInstruction(MCStreamer &Out,
                                             const MCInst &Inst) {
   Out.EmitInstruction(Inst, STI);
 }
 
+unsigned X86AsmInstrumentation::GetFrameRegGeneric(const MCContext &Ctx,
+                                                   MCStreamer &Out) {
+  if (!Out.getNumFrameInfos()) // No active dwarf frame
+    return X86::NoRegister;
+  const MCDwarfFrameInfo &Frame = Out.getDwarfFrameInfos().back();
+  if (Frame.End) // Active dwarf frame is closed
+    return X86::NoRegister;
+  const MCRegisterInfo *MRI = Ctx.getRegisterInfo();
+  if (!MRI) // No register info
+    return X86::NoRegister;
+
+  if (FrameReg != X86::NoRegister) {
+    // FrameReg is set, we're instrumenting a MachineFunction.
+    return FrameReg;
+  }
+
+  // FrameReg isn't set and we're instrumenting an inline assembly.
+  // If CFA register is not specified explicitly, let's assume that it's SP.
+  if (!Frame.CfaRegister)
+    return X86::RSP;
+  return MRI->getLLVMRegNum(Frame.CfaRegister, true /* IsEH */);
+}
+
 X86AsmInstrumentation *
 CreateX86AsmInstrumentation(const MCTargetOptions &MCOptions,
                             const MCContext &Ctx, const MCSubtargetInfo &STI) {
   Triple T(STI.getTargetTriple());
   const bool hasCompilerRTSupport = T.isOSLinux();
   if (ClAsanInstrumentAssembly && hasCompilerRTSupport &&
       MCOptions.SanitizeAddress) {
     if ((STI.getFeatureBits() & X86::Mode32Bit) != 0)
       return new X86AddressSanitizer32(STI);
     if ((STI.getFeatureBits() & X86::Mode64Bit) != 0)
       return new X86AddressSanitizer64(STI);
   }
   return new X86AsmInstrumentation(STI);
 }
 
 } // End llvm namespace

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-NOT: .cfi_adjust_cfa_offset 8
+# CHECK: movl %ebp, %ebp
+# CHECK: .cfi_remember_state
+# CHECK: .cfi_def_cfa_register %ebp
+# CHECK: popl %ebp
+# 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: .cfi_adjust_cfa_offset 4
+# CHECK: movl %esp, %ebp
+# CHECK: .cfi_remember_state
+# CHECK: .cfi_def_cfa_register %ebp
+# CHECK: popl %ebp
+# 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
+  .cfi_offset %ebp, 0
+  movl  %esp, %ebp
+  movl  8(%ebp), %eax
+  movl  12(%ebp), %ecx
+  movl  (%ecx), %ecx
+  movl  %ecx, (%eax)
+  popl  %ebp
+  retl
+  .cfi_endproc

test/Instrumentation/AddressSanitizer/X86/asm_mov.s

 # RUN: llvm-mc %s -triple=x86_64-unknown-linux-gnu -mcpu=corei7 -mattr=+sse2 -asm-instrumentation=address -asan-instrument-assembly | FileCheck %s
 
   .text
   .globl  mov1b
   .align  16, 0x90
   .type mov1b,@function
 # CHECK-LABEL: mov1b:
 #
 # CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_load1@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movb (%rsi), %al
+# CHECK: movb (%rsi), %al
 #
-# CHECK-NEXT: leaq -128(%rsp), %rsp
+# CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_store1@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movb %al, (%rdi)
+# CHECK: movb %al, (%rdi)
 mov1b:                                  # @mov1b
   .cfi_startproc
 # BB#0:
   #APP
   movb  (%rsi), %al
   movb  %al, (%rdi)
 
   #NO_APP
   retq
 .Ltmp0:
   .size mov1b, .Ltmp0-mov1b
   .cfi_endproc
 
   .globl  mov16b
   .align  16, 0x90
   .type mov16b,@function
 # CHECK-LABEL: mov16b:
 #
 # CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_load16@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movaps (%rsi), %xmm0
+# CHECK: movaps (%rsi), %xmm0
 #
-# CHECK-NEXT: leaq -128(%rsp), %rsp
+# CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_store16@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movaps %xmm0, (%rdi)
+# CHECK: movaps %xmm0, (%rdi)
 mov16b:                                 # @mov16b
   .cfi_startproc
 # BB#0:
   #APP
   movaps  (%rsi), %xmm0
   movaps  %xmm0, (%rdi)
 
   #NO_APP
   retq
 .Ltmp1:
   .size mov16b, .Ltmp1-mov16b
   .cfi_endproc
 
 
   .ident  "clang version 3.5 "
   .section  ".note.GNU-stack","",@progbits

test/Instrumentation/AddressSanitizer/X86/asm_swap_intel.s

 # RUN: llvm-mc %s -x86-asm-syntax=intel -triple=x86_64-unknown-linux-gnu -asm-instrumentation=address -asan-instrument-assembly | FileCheck %s
 
   .text
   .globl  swap
   .align  16, 0x90
   .type swap,@function
 # CHECK-LABEL: swap:
 #
 # CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_load8@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movq (%rcx), %rax
+# CHECK: movq (%rcx), %rax
 #
-# CHECK-NEXT: leaq -128(%rsp), %rsp
+# CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_load8@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movq (%rdx), %rbx
+# CHECK: movq (%rdx), %rbx
 #
-# CHECK-NEXT: leaq -128(%rsp), %rsp
+# CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_store8@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movq %rbx, (%rcx)
+# CHECK: movq %rbx, (%rcx)
 #
-# CHECK-NEXT: leaq -128(%rsp), %rsp
+# CHECK: leaq -128(%rsp), %rsp
 # CHECK: callq __asan_report_store8@PLT
 # CHECK: leaq 128(%rsp), %rsp
 #
-# CHECK-NEXT: movq %rax, (%rdx)
+# CHECK: movq %rax, (%rdx)
 swap:                                   # @swap
   .cfi_startproc
 # BB#0:
   push  rbx
 .Ltmp0:
   .cfi_def_cfa_offset 16
 .Ltmp1:
   .cfi_offset rbx, -16