[XRay][X86] Use a valid instruction for the synthetic reference.

[dberris]

Summary

Before this change, we unconditionally write out a synthetic reference to the
XRay function index label in the text section. This makes it so that we're
writing valid instructions in the text section before the actual
relocation/reference.

With this change we're aiming to help the CPU's decoder to decode valid
instructions instead of potentially stalling because of almost quite literally
garbage data in the text section. To do this, we're introducing a protected
virtual function in the AsmPrinter class, which is specifically an extension
point for emitting the XRay synthetic reference.

We're using the INT3 and 7-byte nops for padding before emitting the actual
relocation/reference.

[dberris]

Hi Craig, Reid, and Evgenii,

Do you have alternatives to movabsq for what I'm trying to achieve here?

Thanks in advance!

[craig.topper]

Does xray only work on 64-bit x86?

R10D isn't a valid register for MOV64ri. It should be R10. The D is the 32-bit size.

I can't think of a better instruction if you need all the bits. movabsq is the only instruciton that takes a 64-bit immediate. The only other option might be one of the loads into al/ax/eax/rax from a 64-bit absolute address.

[dberris]

In D35928#822397, @craig.topper wrote:

Does xray only work on 64-bit x86?

Yes, we don't have an implementation for 32-bit x86.

If we end up doing so, we will need to update the implementation to check and use the right instructions for 32-bit. Not today though. :)

R10D isn't a valid register for MOV64ri. It should be R10. The D is the 32-bit size.

Whoops -- good catch, thanks!

I can't think of a better instruction if you need all the bits. movabsq is the only instruciton that takes a 64-bit immediate. The only other option might be one of the loads into al/ax/eax/rax from a 64-bit absolute address.

I see. I think writing to the scratch register from outside of the function bodies might be more "benign" if it ever gets executed (compared to a read into one of the other registers). Although I'm not a security expert, so maybe the tradeoff here isn't as clear-cut. :/

Will the x86 pipelines be happier with a store to a register than a load of an address? Or put another way, is the cost of movabsq <immediate>,%r10 higher if it gets speculatively executed compared to say a load instruction? Or should I not even worry about this because the odds of the instruction being decoded/executed is very small (since it's aligned to 16 byte boundaries away from the end of the function)?

[dberris]

• fixup: use %r10 not %r10d

[dberris]

(reword)

[dberris]

• Add some nops after the synthesized instruction

[chandlerc]

I'd like to revisit the goal here:

Is this a performance fix by avoiding an unknown instruction that gets decoded in some cases? Or is there a correctness issue? (I think you mentioned something about linking having trouble w/ this?)

In D35928#822817, @dberris wrote:

I can't think of a better instruction if you need all the bits. movabsq is the only instruciton that takes a 64-bit immediate. The only other option might be one of the loads into al/ax/eax/rax from a 64-bit absolute address.

I see. I think writing to the scratch register from outside of the function bodies might be more "benign" if it ever gets executed (compared to a read into one of the other registers). Although I'm not a security expert, so maybe the tradeoff here isn't as clear-cut. :/

Will the x86 pipelines be happier with a store to a register than a load of an address? Or put another way, is the cost of movabsq <immediate>,%r10 higher if it gets speculatively executed compared to say a load instruction? Or should I not even worry about this because the odds of the instruction being decoded/executed is very small (since it's aligned to 16 byte boundaries away from the end of the function)?

Emitting a load sounds really bad here. Honestly, even movabsq seems a bit bad if you're worried about performance.

To help with performance I would pad this out with nops that we know decode *fast* (as in, not into micro-ops). If you want to be defensive about ROP gadgets, I'd emit a seld of int3 instructions followed by whatever you want.

The interesting bit (as indicated above) is whether you can just guard the raw data with a nop/int3 sled, or if you actually need a valid instruction. If you *do* need a valid instruction, I'd like to understand why. We have a reasonable number of places that slap raw data above a function section...

[dberris]

Use different instructions for padding.

[dberris]

(reword)

[dberris]

In D35928#826967, @chandlerc wrote:

I'd like to revisit the goal here:

Is this a performance fix by avoiding an unknown instruction that gets decoded in some cases? Or is there a correctness issue? (I think you mentioned something about linking having trouble w/ this?)

This is mainly for performance reasons, one that's speculative too. It's an attempt to align whatever is coming next after the synthetic reference to 16-bytes, so I was looking for a way to do this with "valid" instructions.

In D35928#822817, @dberris wrote:

I can't think of a better instruction if you need all the bits. movabsq is the only instruciton that takes a 64-bit immediate. The only other option might be one of the loads into al/ax/eax/rax from a 64-bit absolute address.

I see. I think writing to the scratch register from outside of the function bodies might be more "benign" if it ever gets executed (compared to a read into one of the other registers). Although I'm not a security expert, so maybe the tradeoff here isn't as clear-cut. :/

Will the x86 pipelines be happier with a store to a register than a load of an address? Or put another way, is the cost of movabsq <immediate>,%r10 higher if it gets speculatively executed compared to say a load instruction? Or should I not even worry about this because the odds of the instruction being decoded/executed is very small (since it's aligned to 16 byte boundaries away from the end of the function)?

Emitting a load sounds really bad here. Honestly, even movabsq seems a bit bad if you're worried about performance.

To help with performance I would pad this out with nops that we know decode *fast* (as in, not into micro-ops). If you want to be defensive about ROP gadgets, I'd emit a seld of int3 instructions followed by whatever you want.

The interesting bit (as indicated above) is whether you can just guard the raw data with a nop/int3 sled, or if you actually need a valid instruction. If you *do* need a valid instruction, I'd like to understand why. We have a reasonable number of places that slap raw data above a function section...

I settled on an INT3 then a 7-byte sled, then the reference. This is so that we can mitigate as much as possible the off-chance that the synthetic reference would be decoded and speculatively executed.

Thanks, Chandler!

PTAL

[dberris]

This is no longer necessary, now that we've found a way to keep the sections alive without the use of a synthetic reference.

include/llvm/CodeGen/AsmPrinter.h

private:
 
   /// If the target supports dwarf debug info, this pointer is non-null.
   DwarfDebug *DD = nullptr;
 
   /// If the current module uses dwarf CFI annotations strictly for debugging.
   bool isCFIMoveForDebugging = false;
 
 protected:
+  /// Extension point for XRay to customize the generation of a synthetic
+  /// reference past the function body.
+  virtual void emitXRaySyntheticRef(MCSymbol* Symbol);
+
   explicit AsmPrinter(TargetMachine &TM, std::unique_ptr<MCStreamer> Streamer);
 
 public:
   ~AsmPrinter() override;
 
   DwarfDebug *getDwarfDebug() { return DD; }
   DwarfDebug *getDwarfDebug() const { return DD; }
 

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::XRayFunctionEntry::emit(int Bytes, MCStreamer *Out,
   Out->EmitSymbolValue(CurrentFnSym, Bytes);
   auto Kind8 = static_cast<uint8_t>(Kind);
   Out->EmitBytes(StringRef(reinterpret_cast<const char *>(&Kind8), 1));
   Out->EmitBytes(
       StringRef(reinterpret_cast<const char *>(&AlwaysInstrument), 1));
   Out->EmitZeros(2 * Bytes - 2);  // Pad the previous two entries
 }
 
+void AsmPrinter::emitXRaySyntheticRef(MCSymbol* Symbol) {
+  auto WordSizeBytes = MAI->getCodePointerSize();
+  OutStreamer->EmitCodeAlignment(16);
+  OutStreamer->EmitSymbolValue(Symbol, WordSizeBytes, false);
+}
+
 void AsmPrinter::emitXRayTable() {
   if (Sleds.empty())
     return;
 
   auto PrevSection = OutStreamer->getCurrentSectionOnly();
   auto Fn = MF->getFunction();
   MCSection *InstMap = nullptr;
   MCSection *FnSledIndex = nullptr;
void AsmPrinter::emitXRayTable() {
 
   // Before we switch over, we force a reference to a label inside the
   // xray_fn_idx sections. This makes sure that the xray_fn_idx section is kept
   // live by the linker if the function is not garbage-collected. Since this
   // function is always called just before the function's end, we assume that
   // this is happening after the last return instruction.
   auto WordSizeBytes = MAI->getCodePointerSize();
   MCSymbol *IdxRef = OutContext.createTempSymbol("xray_fn_idx_synth_", true);
-  OutStreamer->EmitCodeAlignment(16);
-  OutStreamer->EmitSymbolValue(IdxRef, WordSizeBytes, false);
+  emitXRaySyntheticRef(IdxRef);
 
   // Now we switch to the instrumentation map section. Because this is done
   // per-function, we are able to create an index entry that will represent the
   // range of sleds associated with a function.
   MCSymbol *SledsStart = OutContext.createTempSymbol("xray_sleds_start", true);
   OutStreamer->SwitchSection(InstMap);
   OutStreamer->EmitLabel(SledsStart);
   for (const auto &Sled : Sleds)

lib/Target/X86/X86AsmPrinter.h

public:
 
   bool doInitialization(Module &M) override {
     SMShadowTracker.reset(0);
     SM.reset();
     return AsmPrinter::doInitialization(M);
   }
 
   bool runOnMachineFunction(MachineFunction &F) override;
+
+protected:
+  /// Special implementation for X86 of synthetic reference generation in X86.
+  void emitXRaySyntheticRef(MCSymbol* Symbol) override;
 };
 
 } // end namespace llvm
 
 #endif

lib/Target/X86/X86AsmPrinter.cpp

 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
+#include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCSectionCOFF.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"
 using namespace llvm;

lib/Target/X86/X86MCInstLower.cpp

 };
 
 } // end anonymous namespace
 
 // Emit a minimal sequence of nops spanning NumBytes bytes.
 static void EmitNops(MCStreamer &OS, unsigned NumBytes, bool Is64Bit,
                      const MCSubtargetInfo &STI);
 
+/// emitXRaySyntheticRef - Appropriately size the synthetic reference to use
+/// instructions and nop paddings before the actual relocation/symbol.
+void X86AsmPrinter::emitXRaySyntheticRef(MCSymbol* Symbol) {
+  auto WordSizeBytes = MAI->getCodePointerSize();
+
+  // Use int3 and then a 7-byte nop aligned to 16-bytes. This allows us to use
+  // 16 bytes for the synthetic reference, so that we are 16-byte aligned.
+  OutStreamer->EmitCodeAlignment(2 * WordSizeBytes);
+  OutStreamer->EmitInstruction(MCInstBuilder(X86::INT3), getSubtargetInfo());
+  EmitNops(*OutStreamer, 7u, getSubtarget().is64Bit(), getSubtargetInfo());
+  OutStreamer->EmitSymbolValue(Symbol, WordSizeBytes);
+}
+
 void X86AsmPrinter::StackMapShadowTracker::count(MCInst &Inst,
                                                  const MCSubtargetInfo &STI,
                                                  MCCodeEmitter *CodeEmitter) {
   if (InShadow) {
     SmallString<256> Code;
     SmallVector<MCFixup, 4> Fixups;
     raw_svector_ostream VecOS(Code);
     CodeEmitter->encodeInstruction(Inst, VecOS, Fixups, STI);

test/CodeGen/X86/xray-attribute-instrumentation.ll

 ; RUN: llc -filetype=asm -o - -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
 ; RUN: llc -filetype=asm -o - -mtriple=x86_64-darwin-unknown    < %s | FileCheck %s
 
 define i32 @foo() nounwind noinline uwtable "function-instrument"="xray-always" {
 ; CHECK:       .p2align 1, 0x90
 ; CHECK-LABEL: Lxray_sled_0:
 ; CHECK-NEXT:  .ascii "\353\t"
 ; CHECK-NEXT:  nopw 512(%rax,%rax)
 ; CHECK-LABEL: Ltmp0:
   ret i32 0
 ; CHECK:       .p2align 1, 0x90
 ; CHECK-LABEL: Lxray_sled_1:
 ; CHECK-NEXT:  retq
 ; CHECK-NEXT:  nopw %cs:512(%rax,%rax)
 }
 ; CHECK:       .p2align 4, 0x90
-; CHECK-NEXT:  .quad {{.*}}xray_fn_idx_synth_0
+; CHECK-NEXT:  int3
+; CHECK-NEXT:  nopl 512(%rax)
+; CHECK-NEXT:  .quad {{.*}}xray_fn_idx_synth_0{{.*}}
 ; CHECK-NEXT:  .section {{.*}}xray_instr_map
 ; CHECK-LABEL: Lxray_sleds_start0:
 ; CHECK:       .quad {{.*}}xray_sled_0
 ; CHECK:       .quad {{.*}}xray_sled_1
 ; CHECK-LABEL: Lxray_sleds_end0:
 ; CHECK:       .section {{.*}}xray_fn_idx
 ; CHECK-LABEL: Lxray_fn_idx_synth_0:
 ; CHECK:       .quad {{.*}}xray_sleds_start0

 NotEqual:
   ret i32 1
 ; CHECK:       .p2align 1, 0x90
 ; CHECK-LABEL: Lxray_sled_4:
 ; CHECK-NEXT:  retq
 ; CHECK-NEXT:  nopw %cs:512(%rax,%rax)
 }
 ; CHECK:       .p2align 4, 0x90
-; CHECK-NEXT:  .quad {{.*}}xray_fn_idx_synth_1
+; CHECK-NEXT:  int3
+; CHECK-NEXT:  nopl 512(%rax)
+; CHECK-NEXT:  .quad {{.*}}xray_fn_idx_synth_1{{.*}}
 ; CHECK-NEXT:  .section {{.*}}xray_instr_map
 ; CHECK-LABEL: Lxray_sleds_start1:
 ; CHECK:       .quad {{.*}}xray_sled_2
 ; CHECK:       .quad {{.*}}xray_sled_3
 ; CHECK:       .quad {{.*}}xray_sled_4
 ; CHECK-LABEL: Lxray_sleds_end1:
 ; CHECK:       .section {{.*}}xray_fn_idx
 ; CHECK-LABEL: Lxray_fn_idx_synth_1:
 ; CHECK:       .quad {{.*}}xray_sleds_start1
 ; CHECK-NEXT:  .quad {{.*}}xray_sleds_end1

test/CodeGen/X86/xray-tail-call-sled.ll

 ; RUN: llc -filetype=asm -o - -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
 ; RUN: llc -filetype=asm -o - -mtriple=x86_64-darwin-unknown    < %s | FileCheck %s
 
 define i32 @callee() nounwind noinline uwtable "function-instrument"="xray-always" {
 ; CHECK:       .p2align 1, 0x90
 ; CHECK-LABEL: Lxray_sled_0:
 ; CHECK-NEXT:  .ascii "\353\t"
 ; CHECK-NEXT:  nopw 512(%rax,%rax)
 ; CHECK-LABEL: Ltmp0:
   ret i32 0
 ; CHECK:       .p2align 1, 0x90
 ; CHECK-LABEL: Lxray_sled_1:
 ; CHECK-NEXT:  retq
 ; CHECK-NEXT:  nopw %cs:512(%rax,%rax)
 }
 ; CHECK:       .p2align 4, 0x90
+; CHECK-NEXT:  int3
+; CHECK-NEXT:  nopl 512(%rax)
 ; CHECK-NEXT:  .quad {{.*}}xray_fn_idx_synth_0{{.*}}
 ; CHECK-NEXT:  .section {{.*}}xray_instr_map
 ; CHECK-LABEL: Lxray_sleds_start0:
 ; CHECK:       .quad {{.*}}xray_sled_0
 ; CHECK:       .quad {{.*}}xray_sled_1
 ; CHECK-LABEL: Lxray_sleds_end0:
 ; CHECK-NEXT:  .section {{.*}}xray_fn_idx
 ; CHECK-LABEL: Lxray_fn_idx_synth_0:

 ; CHECK-NEXT:  .ascii "\353\t"
 ; CHECK-NEXT:  nopw 512(%rax,%rax)
 ; CHECK-LABEL: Ltmp2:
   %retval = tail call i32 @callee()
 ; CHECK:       jmp {{.*}}callee {{.*}}# TAILCALL
   ret i32 %retval
 }
 ; CHECK:       .p2align 4, 0x90
+; CHECK-NEXT:  int3
+; CHECK-NEXT:  nopl 512(%rax)
 ; CHECK-NEXT:  .quad {{.*}}xray_fn_idx_synth_1{{.*}}
 ; CHECK-LABEL: Lxray_sleds_start1:
 ; CHECK:       .quad {{.*}}xray_sled_2
 ; CHECK:       .quad {{.*}}xray_sled_3
 ; CHECK-LABEL: Lxray_sleds_end1:
 ; CHECK:       .section {{.*}}xray_fn_idx
 ; CHECK-LABEL: Lxray_fn_idx_synth_1:
 ; CHECK:       .quad {{.*}}xray_sleds_start1
 ; CHECK:       .quad {{.*}}xray_sleds_end1