[X86] Support the "ms-hotpatch" attribute.

[cdavis5x]

Summary

Based on two patches by Michael Mueller.

This is a target attribute that causes a function marked with it to be
emitted as "hotpatchable". This particular mechanism was originally
devised by Microsoft for patching their binaries (which they are
constantly updating to stay ahead of crackers, script kiddies, and other
ne'er-do-wells on the Internet), but is now commonly abused by Windows
programs to hook API functions.

This mechanism is target-specific. For x86, a two-byte no-op instruction
is emitted at the function's entry point; the entry point must be
immediately preceded by 64 (32-bit) or 128 (64-bit) bytes of padding.
This padding is where the patch code is written. The two byte no-op is
then overwritten with a short jump into this code. The no-op is usually
a movl %edi, %edi instruction; this is used as a magic value
indicating that this is a hotpatchable function.

[rnk]

This should use Sanjoy's "patchable-function" attribute:
http://reviews.llvm.org/D19046

lib/Target/X86/X86AsmPrinter.cpp

76

You should consider overriding EmitConstantPool instead, so the fill directive comes before the alignment directive:

.text
.fill 5, 1, 0xcc
.align 2
.global foo
foo:
  movl %edi, %edi
  retq

Are the hotpatching tools that we care about checking for 0xcc bytes, or will they overwrite alignment nops?

[cdavis5x]

• Use Sanjoy's "patchable-function" attribute.
• Override EmitConstantPool() instead of EmitFunctionEntryLabel().
• Clarify why we're emitting 0xcc bytes (int3 instructions) instead of NOPs.

[cdavis5x]

lib/Target/X86/X86AsmPrinter.cpp

76

Are the hotpatching tools that we care about checking for 0xcc bytes, or will they overwrite alignment nops?

It's not necessarily that tools care, it's that I think I'd prefer loudly crashing to getting stuck in an infinite loop if the patch forgets to jump back into the function. (I've added a comment clarifying this.)

[sanjoy]

docs/LangRef.rst

1435 ↗

(On Diff #57387)

So on x86-64, the 2 byte instruction can be anything? May be helpful to be explicit about that.

lib/CodeGen/PatchableFunction.cpp

43 ↗

(On Diff #57387)

I presume you're handling the x86-64 case here? If so, how about handling both the 32bit and 64bit case here? On 32 bits, you can just emit a mov %edi, %edi instead of a PATCHABLE_OP? That way related bits stay together.

[rnk]

lib/CodeGen/PatchableFunction.cpp

43 ↗

(On Diff #57387)

+1

49 ↗

(On Diff #57387)

GCC will give you a -Wparentheses warning on this.

lib/Target/X86/X86ISelLowering.cpp

2500–2503

Why do we need this change? The patchable nop comes before the prologue. Surely this isn't use for some kind of on-stack-replacement.

test/CodeGen/X86/ms-hotpatch-attr.ll

5

Can you add a check for "align"? Otherwise there's no difference between the constant pool entry and the function label override.

[cdavis5x]

lib/CodeGen/PatchableFunction.cpp

43 ↗

(On Diff #57387)

I'm not so sure about this. This looks like it's ultimately intended to be a target-independent pass. Putting target-specific code in here feels... wrong. If you really want me to do that anyway, I'll do it, but I'm just saying, I don't think that's such a good idea.

[cdavis5x]

• Clarify that x86-64 doesn't use a special magic no-op.
• Don't emit patch space with MSVC. The linker adds it for us there.
• Don't force 64-bit functions to use a stack frame. That's only needed for 32-bit.
• Test that, when we do emit the patch space, we emit it before the alignment directive.

[cdavis5x]

docs/LangRef.rst

1435 ↗

(On Diff #57387)

Theoretically, but GCC seems to emit a lea (%rsp), %rsp on x86-64 (which is 8 bytes long). I don't know if we need to do this.

@rnk: What does MSVC do in that situation?

lib/Target/X86/X86ISelLowering.cpp

2500–2503

GCC seems to emit a standard push %ebp; mov %esp, %ebp prologue along with the patchable no-op. It doesn't do this for 64-bit.

[sanjoy]

Sorry for the delayed response, I thought I pressed submit, when I hadn't.

lib/CodeGen/PatchableFunction.cpp

43 ↗

(On Diff #57850)

You'd have to add a TTI hook or re-purposes an existing one that generates the code for you (I agree that hard-coding X86 instructions here is a bad idea).

[sanjoy]

Marked as "needing changes" to get this off my pending queue.

[dberris]

docs/LangRef.rst

1446 ↗

(On Diff #57850)

At some point we probably have to reconcile how XRay will work with ms-hotpatch and prologue-short-redirect.

Is it absolutely required that the first instructions be specific ones, or would something like what XRay has for functions work too (like a short relative jump then a few more bytes of nops)?

[cdavis5x]

Factor emission of the patchable instruction into a TTI hook.

[cdavis5x]

docs/LangRef.rst

1463 ↗

(On Diff #62407)

tl;dr: You'll have problems on i386, but not anywhere else.

On 32-bit x86, a function marked ms-hotpatch must start with the bytes 8b ff (a mov edi, edi instruction). (Strictly speaking, it doesn't have to, but programs are abusing this to alter functionality of Windows API calls, and they use this as a sort of magic number to know if a function can be hotpatched or not, so unfortunately, it's too late to change this.) Later on, this gets replaced by a two-byte jmp short into the patch space (which is only guaranteed to be at least 5 bytes, though GCC--and therefore, this patch--gives you 64 bytes) which precedes this function. I believe the mov edi, edi must immediately be followed by a standard prologue (i.e. push ebp; mov ebp, esp), but I'm not sure about that. Based on all that, and on what you've said, I fear you may have problems getting XRay to play well with ms-hotpatch-style functions on i386. But it looks like you don't support anything other than x86-64 yet.

On any other architecture (including x64), no, it is not required (AFAICT) to be any specific instruction.

[cdavis5x]

Ping.

[rnk]

lgtm

[cdavis5x]

@sanjoy: Ping.

[sanjoy]

lgtm

[cdavis5x]

Unfortunately, this patch needs revision due to a merge conflict. :\

[cdavis5x]

Merge with a more recent revision.

[cdavis5x]

I think I've figured out why the build broke when I landed this. (Sorry that took so long.)

When I added the emitPatchableOp() method to TargetTransformInfo, I added a dependency of Analysis on CodeGen, as well. But, I didn't record that in the build files. I didn't see any problems because I normally build with LLVM_LINK_LLVM_DYLIB=ON, so all of LLVM gets linked into a giant dylib, and everything that uses LLVM gets linked to this giant dylib. The buildbots, on the other hand, don't do this. Since the build system didn't know that Analysis now depended on CodeGen, BOOM.

I could've just added the dependency and been done with it, but this got me thinking. CodeGen already depends on Analysis. Having Analysis depend on CodeGen would produce a circular dependency. I don't think that's a good thing. I also became convinced that adding emitPatchableOp() to TargetTransformInfo constituted a layering violation. So, I moved it somewhere else--into TargetInstrInfo instead. The base TargetInstrInfo is already part of CodeGen, so no new dependencies are added by this change.

I don't want to break the build again like that. I also made an (IMO) not insignificant change to this patch. So, I'd like you all to review this patch again. (Sorry, but this is the last time, I promise!) I've verified that this will not break the build again.

[cdavis5x]

Fix the build error we saw in trunk.

[aganea]

Hello! It seems this patch and another related patch were reverted and @cdavis5x has been inactive since.
Any objection if I tried to reland this? Is there a policy for abandonned patches?

[MaskRay]

In D19908#2051605, @aganea wrote:

Hello! It seems this patch and another related patch were reverted and @cdavis5x has been inactive since.
Any objection if I tried to reland this? Is there a policy for abandonned patches?

I don't know the policy is written down anywhere. You may create a new patch and state that it is based on @cdavis5x.
I took a quick look at the test llvm/test/CodeGen/X86/ms-hotpatch-attr.ll. It can be improved a bit.

According to https://reviews.llvm.org/D72215#1815586 , @reames is a user of the existing "patchable-function" and you may need to add him as a reviewer.
("patchable-function-entry" and ("xray-instruction-threashold"|"function-instrument"="xray-always") are similar but different instrumentations.)

[cdavis5x]

In D19908#2051605, @aganea wrote:

Hello! It seems this patch and another related patch were reverted and @cdavis5x has been inactive since.
Any objection if I tried to reland this? Is there a policy for abandonned patches?

I've actually been meaning to get back to this for a while now, but I've been busy with other stuff lately. If you want to drive this forward, that's fine by me.

[aganea]

This patch didn't apply cleanly on latest, for the reasons mentionned by @cdavis5x here: https://reviews.llvm.org/D19908#518374
I ended up reimplementing the end-feature of this patch in D81301.
@cdavis5x Could you please take a look at let me know if that patch covers what you intended to do?

llvm/lib/Target/X86/X86AsmPrinter.cpp

97 ↗

(On Diff #223567)

I'm not sure why this was done here. I vaguely recall that older versions of MSVC (VS2012 or before) were generating this kind of fill, but this was then replaced by /FUNCTIONPADMIN in the linker (it is supported by LLD).

lib/Target/X86/X86AsmPrinter.h

public:
   void EmitEndOfAsmFile(Module &M) override;
 
   void EmitInstruction(const MachineInstr *MI) override;
 
   void EmitBasicBlockEnd(const MachineBasicBlock &MBB) override {
     SMShadowTracker.emitShadowPadding(*OutStreamer, getSubtargetInfo());
   }
 
+  void EmitFunctionEntryLabel() override;
+
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        unsigned AsmVariant, const char *ExtraCode,
                        raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              unsigned AsmVariant, const char *ExtraCode,
                              raw_ostream &OS) override;
 
   /// \brief Return the symbol for the specified constant pool entry.

lib/Target/X86/X86AsmPrinter.cpp

bool X86AsmPrinter::runOnMachineFunction(MachineFunction &MF) {
 
   // Emit the rest of the function body.
   EmitFunctionBody();
 
   // We didn't modify anything.
   return false;
 }
 
+void X86AsmPrinter::EmitFunctionEntryLabel() {
+  // If a hotpatch function, we need to ensure 64 (32-bit) or 128 (64-bit)
+  // bytes of padding precede the label.  This is the scratch space used
+  // by the hotpatching mechanism to insert a jump to the patch code.  The
+  // movl %edi, %edi instruction emitted as the very first instruction of
+  // a hotpatch function is usually overwritten with a short jump
+  // instruction when the patch is installed, so it will jump directly into
+  // this space.
+  if (MF->getFunction()->hasFnAttribute("ms-hotpatch")) {
+    // FIXME: Figure out some way to use the X86AsmBackend::writeNopData()
+    // method for this.
+    OutStreamer->EmitFill(Subtarget->is64Bit() ? 128 : 64, 0xcc);
+  }
+
+  AsmPrinter::EmitFunctionEntryLabel();
+}
+
 /// printSymbolOperand - Print a raw symbol reference operand.  This handles
 /// jump tables, constant pools, global address and external symbols, all of
 /// which print to a label with various suffixes for relocation types etc.
 static void printSymbolOperand(X86AsmPrinter &P, const MachineOperand &MO,
                                raw_ostream &O) {
   switch (MO.getType()) {
   default: llvm_unreachable("unknown symbol type!");
   case MachineOperand::MO_ConstantPoolIndex:

lib/Target/X86/X86FrameLowering.cpp

bool FnHasClrFunclet =
       MMI.hasEHFunclets() && Personality == EHPersonality::CoreCLR;
   bool IsClrFunclet = IsFunclet && FnHasClrFunclet;
   bool HasFP = hasFP(MF);
   bool IsWin64CC = STI.isCallingConvWin64(Fn->getCallingConv());
   bool IsWin64Prologue = MF.getTarget().getMCAsmInfo()->usesWindowsCFI();
   bool NeedsWinCFI = IsWin64Prologue && Fn->needsUnwindTableEntry();
   bool NeedsDwarfCFI =
       !IsWin64Prologue && (MMI.hasDebugInfo() || Fn->needsUnwindTableEntry());
+  bool IsMSHotpatch = Fn->hasFnAttribute("ms-hotpatch");
   unsigned FramePtr = TRI->getFrameRegister(MF);
   const unsigned MachineFramePtr =
       STI.isTarget64BitILP32()
           ? getX86SubSuperRegister(FramePtr, 64) : FramePtr;
   unsigned BasePtr = TRI->getBaseRegister();
   
   // Debug location must be unknown since the first debug location is used
   // to determine the end of the prologue.
if (Is64Bit && !Fn->hasFnAttribute(Attribute::NoRedZone) &&
       !MF.shouldSplitStack()) {                 // Regular stack
     uint64_t MinSize = X86FI->getCalleeSavedFrameSize();
     if (HasFP) MinSize += SlotSize;
     X86FI->setUsesRedZone(MinSize > 0 || StackSize > 0);
     StackSize = std::max(MinSize, StackSize > 128 ? StackSize - 128 : 0);
     MFI->setStackSize(StackSize);
   }
 
+  if (IsMSHotpatch && !Is64Bit)
+    // Add a no-op movl %edi, %edi instruction.  A short jump instruction
+    // will be written over it if a patch is installed.  This particular
+    // instruction is often used as a sort of magic number indicating that
+    // this function can be patched.
+    // N.B. This MI *must* be the first instruction, or the hook won't work.
+    BuildMI(MBB, MBBI, DL, TII.get(X86::MOV32rr_REV), X86::EDI)
+      .addReg(X86::EDI)
+      .setMIFlag(MachineInstr::FrameSetup);
+
   // Insert stack pointer adjustment for later moving of return addr.  Only
   // applies to tail call optimized functions where the callee argument stack
   // size is bigger than the callers.
   if (TailCallReturnAddrDelta < 0) {
     BuildStackAdjustment(MBB, MBBI, DL, TailCallReturnAddrDelta,
                          /*InEpilogue=*/false)
         .setMIFlag(MachineInstr::FrameSetup);
   }
if (NeedsWinCFI) {
       BuildMI(MBB, MBBI, DL, TII.get(X86::SEH_PushReg))
           .addImm(FramePtr)
           .setMIFlag(MachineInstr::FrameSetup);
     }
 
     if (!IsWin64Prologue && !IsFunclet) {
       // Update EBP with the new base value.
       BuildMI(MBB, MBBI, DL,
-              TII.get(Uses64BitFramePtr ? X86::MOV64rr : X86::MOV32rr),
+              TII.get(IsMSHotpatch ?
+                      (Uses64BitFramePtr ? X86::MOV64rr_REV : X86::MOV32rr_REV):
+                      (Uses64BitFramePtr ? X86::MOV64rr : X86::MOV32rr)),
               FramePtr)
           .addReg(StackPtr)
           .setMIFlag(MachineInstr::FrameSetup);
 
       if (NeedsDwarfCFI) {
         // Mark effective beginning of when frame pointer becomes valid.
         // Define the current CFA to use the EBP/RBP register.
         unsigned DwarfFramePtr = TRI->getDwarfRegNum(MachineFramePtr, true);

lib/Target/X86/X86ISelLowering.cpp

SDValue X86TargetLowering::LowerFormalArguments(
     SDValue Chain, CallingConv::ID CallConv, bool isVarArg,
     const SmallVectorImpl<ISD::InputArg> &Ins, SDLoc dl, SelectionDAG &DAG,
     SmallVectorImpl<SDValue> &InVals) const {
   MachineFunction &MF = DAG.getMachineFunction();
   X86MachineFunctionInfo *FuncInfo = MF.getInfo<X86MachineFunctionInfo>();
   const TargetFrameLowering &TFI = *Subtarget.getFrameLowering();
 
   const Function* Fn = MF.getFunction();
-  if (Fn->hasExternalLinkage() &&
+  if ((Fn->hasExternalLinkage() &&
        Subtarget.isTargetCygMing() &&
-      Fn->getName() == "main")
+       Fn->getName() == "main") ||
+      Fn->hasFnAttribute("ms-hotpatch"))
     FuncInfo->setForceFramePointer(true);
 
   MachineFrameInfo *MFI = MF.getFrameInfo();
   bool Is64Bit = Subtarget.is64Bit();
   bool IsWin64 = Subtarget.isCallingConvWin64(CallConv);
 
   assert(!(isVarArg && canGuaranteeTCO(CallConv)) &&
          "Var args not supported with calling convention fastcc, ghc or hipe");
 
   if (CallConv == CallingConv::X86_INTR) {

test/CodeGen/X86/ms-hotpatch-attr.ll

+; RUN: llc < %s -march=x86 -filetype=asm | FileCheck -check-prefix=CHECK-32 %s
+; RUN: llc < %s -march=x86-64 -filetype=asm | FileCheck -check-prefix=CHECK-64 %s
+
+; CHECK-32: .space 64,204
+; CHECK-32-LABEL: foo:
+; CHECK-32: movl %edi, %edi
+; CHECK-32-NEXT: pushl %ebp
+; CHECK-32-NEXT: movl %esp, %ebp
+; CHECK-64: .space 128,204
+; CHECK-64-LABEL: foo:
+; CHECK-64: pushq %rbp
+; CHECK-64-NEXT: movq %rsp, %rbp
+define void @foo() nounwind "ms-hotpatch" {
+entry:
+  ret void
+}