[X86][CodeGen] Fix crash in hotpatch

[saudi]

Summary

This patch fixes crashes (asserts) encountered in some cases while activating hotpatch using -fms-hotpatch, or clang-cl /HOTPATCH.

PatchableFunction pass emits a TargetOpcode::PATCHABLE_OP at the first LLVM-IR instruction that generates machine code. The search for that instruction was done with a local helper that hasn't been updated along with the addition of new LLVM-IR instructions.

Example crash: https://godbolt.org/z/8M9vGajs9
In this example, DBG_INSTR_REF is selected for TargetOpcode::PATCHABLE_OP; lowering that instruction asserts as the metadata-typed operands are unexpected.

Also, the search for the first instruction was assuming that one would be found, asserting otherwise.
Such case may happen in the case of an empty function that is unreachable ( in C/C++, a simple example is int f() {} )

[saudi]

Update: cleanup lambda (remove capture, fix const correctness)

[saudi]

I found new related crashes, I will investigate.

[rnk]

I was going to approve it as is, since it looks like a correct simplification.

Relatedly, @dblaikie was considering establishing the invariant that all empty functions have at least one trap instruction, although for this code the entry BB could conceivably be empty, and you should handle that.

[saudi]

In D137642#3934867, @rnk wrote:

I was going to approve it as is, since it looks like a correct simplification.

Relatedly, @dblaikie was considering establishing the invariant that all empty functions have at least one trap instruction, although for this code the entry BB could conceivably be empty, and you should handle that.

I will update this patch to better handle the case of an empty entry BB.

Also, I encountered another issue with -fms-hotpatch, which looks more involved: currently, lowering PATCHABLE_OP may not lower the wrapped instruction properly.
I created an issue for it: https://github.com/llvm/llvm-project/issues/59039
I could keep that for another patch, but as it is pretty related it may be worth combining into a patch that completely fixes the feature.
WDYT?

[saudi]

Added support for empty MBB: ensures 2-byte nop is inserted, to make the function patchable.
This should also support the requirement for the functions not to jump to the first instruction.

The 2-byte nop instruction insertion is done by creating a PATCHABLE_OP instance with a PATCHABLE_OP opcode as wrapped instruction.

Also, I removed the support for 16 bit, which would assert when trying to emit nops (happens with the new test cases)
It could be handled instead if necessary; I removed it because I didn't think hotpatching would be ever used on such a platform.

[saudi]

Small update: improve comments, remove empty line modification

[saudi]

Ping!

[rnk]

lgtm

clang/lib/CodeGen/CodeGenFunction.cpp

if (Count && Offset <= Count) {
     if (Offset)
       Fn->addFnAttr("patchable-function-prefix", std::to_string(Offset));
   }
   // Instruct that functions for COFF/CodeView targets should start with a
   // patchable instruction, but only on x86/x64. Don't forward this to ARM/ARM64
   // backends as they don't need it -- instructions on these architectures are
   // always atomically patchable at runtime.
   if (CGM.getCodeGenOpts().HotPatch &&
-      getContext().getTargetInfo().getTriple().isX86())
+      getContext().getTargetInfo().getTriple().isX86() &&
+      getContext().getTargetInfo().getTriple().getEnvironment() !=
+          llvm::Triple::CODE16)
     Fn->addFnAttr("patchable-function", "prologue-short-redirect");
 
   // Add no-jump-tables value.
   if (CGM.getCodeGenOpts().NoUseJumpTables)
     Fn->addFnAttr("no-jump-tables", "true");
 
   // Add no-inline-line-tables value.
   if (CGM.getCodeGenOpts().NoInlineLineTables)

llvm/include/llvm/Support/TargetOpcodes.def

 
 /// Wraps a machine instruction to add patchability constraints.  An
 /// instruction wrapped in PATCHABLE_OP has to either have a minimum
 /// size or be preceded with a nop of that size.  The first operand is
 /// an immediate denoting the minimum size of the instruction, the
 /// second operand is an immediate denoting the opcode of the original
 /// instruction.  The rest of the operands are the operands of the
 /// original instruction.
+/// PATCHABLE_OP can be used as second operand to only insert a nop of
+/// required size.
 HANDLE_TARGET_OPCODE(PATCHABLE_OP)
 
 /// This is a marker instruction which gets translated into a nop sled, useful
 /// for inserting instrumentation instructions at runtime.
 HANDLE_TARGET_OPCODE(PATCHABLE_FUNCTION_ENTER)
 
 /// Wraps a return instruction and its operands to enable adding nop sleds
 /// either before or after the return. The nop sleds are useful for inserting

llvm/lib/CodeGen/PatchableFunction.cpp

struct PatchableFunction : public MachineFunctionPass {
   bool runOnMachineFunction(MachineFunction &F) override;
    MachineFunctionProperties getRequiredProperties() const override {
     return MachineFunctionProperties().set(
         MachineFunctionProperties::Property::NoVRegs);
   }
 };
 }
 
-/// Returns true if instruction \p MI will not result in actual machine code
-/// instructions.
-static bool doesNotGeneratecode(const MachineInstr &MI) {
-  // TODO: Introduce an MCInstrDesc flag for this
-  switch (MI.getOpcode()) {
-  default: return false;
-  case TargetOpcode::IMPLICIT_DEF:
-  case TargetOpcode::KILL:
-  case TargetOpcode::CFI_INSTRUCTION:
-  case TargetOpcode::EH_LABEL:
-  case TargetOpcode::GC_LABEL:
-  case TargetOpcode::DBG_VALUE:
-  case TargetOpcode::DBG_LABEL:
-    return true;
-  }
-}
-
 bool PatchableFunction::runOnMachineFunction(MachineFunction &MF) {
   if (MF.getFunction().hasFnAttribute("patchable-function-entry")) {
     MachineBasicBlock &FirstMBB = *MF.begin();
     const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
     // The initial .loc covers PATCHABLE_FUNCTION_ENTER.
     BuildMI(FirstMBB, FirstMBB.begin(), DebugLoc(),
             TII->get(TargetOpcode::PATCHABLE_FUNCTION_ENTER));
     return true;
   }
 
   if (!MF.getFunction().hasFnAttribute("patchable-function"))
     return false;
 
 #ifndef NDEBUG
   Attribute PatchAttr = MF.getFunction().getFnAttribute("patchable-function");
   StringRef PatchType = PatchAttr.getValueAsString();
   assert(PatchType == "prologue-short-redirect" && "Only possibility today!");
 #endif
 
   auto &FirstMBB = *MF.begin();
-  MachineBasicBlock::iterator FirstActualI = FirstMBB.begin();
-  for (; doesNotGeneratecode(*FirstActualI); ++FirstActualI)
-    assert(FirstActualI != FirstMBB.end());
-
   auto *TII = MF.getSubtarget().getInstrInfo();
+
+  MachineBasicBlock::iterator FirstActualI = llvm::find_if(
+      FirstMBB, [](const MachineInstr &MI) { return !MI.isMetaInstruction(); });
+
+  if (FirstActualI == FirstMBB.end()) {
+    // As of Microsoft documentation on /hotpatch feature, we must ensure that
+    // "the first instruction of each function is at least two bytes, and no
+    // jump within the function goes to the first instruction"
+
+    // When the first MBB is empty, insert a patchable no-op. This ensures the
+    // first instruction is patchable in two special cases:
+    // - the function is empty (e.g. unreachable)
+    // - the function jumps back to the first instruction, which is in a
+    // successor MBB.
+    BuildMI(&FirstMBB, DebugLoc(), TII->get(TargetOpcode::PATCHABLE_OP))
+        .addImm(2)
+        .addImm(TargetOpcode::PATCHABLE_OP);
+    MF.ensureAlignment(Align(16));
+    return true;
+  }
+
   auto MIB = BuildMI(FirstMBB, FirstActualI, FirstActualI->getDebugLoc(),
                      TII->get(TargetOpcode::PATCHABLE_OP))
                  .addImm(2)
                  .addImm(FirstActualI->getOpcode());
 
   for (auto &MO : FirstActualI->operands())
     MIB.add(MO);
 

llvm/lib/Target/X86/X86MCInstLower.cpp

 void X86AsmPrinter::LowerPATCHABLE_OP(const MachineInstr &MI,
                                       X86MCInstLower &MCIL) {
   // PATCHABLE_OP minsize, opcode, operands
 
   NoAutoPaddingScope NoPadScope(*OutStreamer);
 
   unsigned MinSize = MI.getOperand(0).getImm();
   unsigned Opcode = MI.getOperand(1).getImm();
+  // Opcode PATCHABLE_OP is a special case: there is no instruction to wrap,
+  // simply emit a nop of size MinSize.
+  bool EmptyInst = (Opcode == TargetOpcode::PATCHABLE_OP);
 
   MCInst MCI;
   MCI.setOpcode(Opcode);
   for (auto &MO : drop_begin(MI.operands(), 2))
     if (auto MaybeOperand = MCIL.LowerMachineOperand(&MI, MO))
       MCI.addOperand(*MaybeOperand);
 
   SmallString<256> Code;
+  if (!EmptyInst) {
     SmallVector<MCFixup, 4> Fixups;
     raw_svector_ostream VecOS(Code);
     CodeEmitter->encodeInstruction(MCI, VecOS, Fixups, getSubtargetInfo());
+  }
 
   if (Code.size() < MinSize) {
     if (MinSize == 2 && Subtarget->is32Bit() &&
         Subtarget->isTargetWindowsMSVC() &&
         (Subtarget->getCPU().empty() || Subtarget->getCPU() == "pentium3")) {
       // For compatibility reasons, when targetting MSVC, is is important to
       // generate a 'legacy' NOP in the form of a 8B FF MOV EDI, EDI. Some tools
       // rely specifically on this pattern to be able to patch a function.
if (MinSize == 2 && Subtarget->is32Bit() &&
       // bytes too, so the check on MinSize is important.
       MCI.setOpcode(X86::PUSH64rmr);
     } else {
       unsigned NopSize = emitNop(*OutStreamer, MinSize, Subtarget);
       assert(NopSize == MinSize && "Could not implement MinSize!");
       (void)NopSize;
     }
   }
-
+  if (!EmptyInst)
     OutStreamer->emitInstruction(MCI, getSubtargetInfo());
 }
 
 // Lower a stackmap of the form:
 // <id>, <shadowBytes>, ...
 void X86AsmPrinter::LowerSTACKMAP(const MachineInstr &MI) {
   SMShadowTracker.emitShadowPadding(*OutStreamer, getSubtargetInfo());
 
   auto &Ctx = OutStreamer->getContext();

llvm/test/CodeGen/X86/patchable-prologue-debuginfo.ll

+; RUN: llc -verify-machineinstrs < %s | FileCheck %s --check-prefix=CHECK
+
+; Regression test for function patching asserting in some cases when debug info activated.
+; The code below reproduces this crash.
+
+; Compilation flag:  clang -target x86_64-none-linux-gnu -c -O2 -g -fms-hotpatch patchable-prologue-debuginfo.c
+; int func( int val ) {
+;   int neg = -val;
+;   return neg + 1;
+; }
+
+; CHECK: # -- Begin function func
+
+; ModuleID = 'patchable-prologue-debuginfo.c'
+source_filename = "patchable-prologue-debuginfo.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-none-linux-gnu"
+
+; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone willreturn uwtable
+define dso_local i32 @func(i32 noundef %val) local_unnamed_addr #0 !dbg !9 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %val, metadata !14, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata !DIArgList(i32 0, i32 %val), metadata !15, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_minus, DW_OP_stack_value)), !dbg !16
+  %add = sub i32 1, %val, !dbg !17
+  ret i32 %add, !dbg !18
+}
+
+; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { mustprogress nofree norecurse nosync nounwind readnone willreturn uwtable "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "patchable-function"="prologue-short-redirect" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 15.0.4 ([e-mail]:LLVM/llvm-project.git 17850fb41c5bddcd80a9c2714f7e293f49fa8bb2)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "patchable-prologue-debuginfo.c", directory: "D:\\saudi\\bugrepro-llvm-hotpatch-crash")
+!2 = !{i32 7, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 2}
+!8 = !{!"clang version 15.0.4 ([e-mail]:LLVM/llvm-project.git 17850fb41c5bddcd80a9c2714f7e293f49fa8bb2)"}
+!9 = distinct !DISubprogram(name: "func", scope: !1, file: !1, line: 1, type: !10, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+!10 = !DISubroutineType(types: !11)
+!11 = !{!12, !12}
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = !{!14, !15}
+!14 = !DILocalVariable(name: "val", arg: 1, scope: !9, file: !1, line: 1, type: !12)
+!15 = !DILocalVariable(name: "neg", scope: !9, file: !1, line: 3, type: !12)
+!16 = !DILocation(line: 0, scope: !9)
+!17 = !DILocation(line: 4, column: 16, scope: !9)
+!18 = !DILocation(line: 4, column: 5, scope: !9)

llvm/test/CodeGen/X86/patchable-prologue.ll

 ; RUN: llc -verify-machineinstrs -filetype=obj -o - -mtriple=x86_64-apple-macosx < %s | llvm-objdump --no-print-imm-hex --triple=x86_64-apple-macosx -d - | FileCheck %s
 ; RUN: llc -verify-machineinstrs -mtriple=x86_64-apple-macosx < %s | FileCheck %s --check-prefix=CHECK-ALIGN
 ; RUN: llc -verify-machineinstrs -show-mc-encoding -mtriple=i386 < %s | FileCheck %s --check-prefixes=32,32CFI,XCHG
 ; RUN: llc -verify-machineinstrs -show-mc-encoding -mtriple=i386-windows-msvc < %s | FileCheck %s --check-prefixes=32,MOV
 ; RUN: llc -verify-machineinstrs -show-mc-encoding -mtriple=i386-windows-msvc -mcpu=pentium3 < %s | FileCheck %s --check-prefixes=32,MOV
 ; RUN: llc -verify-machineinstrs -show-mc-encoding -mtriple=i386-windows-msvc -mcpu=pentium4 < %s | FileCheck %s --check-prefixes=32,XCHG
 ; RUN: llc -verify-machineinstrs -show-mc-encoding -mtriple=x86_64-windows-msvc < %s | FileCheck %s --check-prefix=64
-; RUN: llc -verify-machineinstrs -show-mc-encoding -mtriple=i386-unknown-linux-code16 < %s | FileCheck %s --check-prefix=16
-
-; 16-NOT: movl   %edi, %edi
-; 16-NOT: xchgw   %ax, %ax
 
 declare void @callee(ptr)
 
 define void @f0() "patchable-function"="prologue-short-redirect" {
 ; CHECK-LABEL: _f0{{>?}}:
 ; CHECK-NEXT:  66 90  nop
 
 ; CHECK-ALIGN:  .p2align  4, 0x90

 
 bb16:
   br label %bb21
 
 bb21:
   %tmp22 = phi i32 [ %tmp12, %bb ], [ %arg3, %bb16 ]
   ret i32 %tmp22
 }
+
+; This testcase produces an empty function (not even a ret on some targets).
+; This scenario can happen with undefined behavior.
+; Ensure that the "patchable-function" pass supports this case.
+; CHECK-LABEL: _emptyfunc
+; CHECK-NEXT: 0f 0b   ud2
+
+; CHECK-ALIGN:  .p2align  4, 0x90
+; CHECK-ALIGN: _emptyfunc:
+
+; 32: emptyfunc:
+; 32CFI-NEXT: .cfi_startproc
+; 32-NEXT: # %bb.0:
+; XCHG-NEXT: xchgw   %ax, %ax
+; MOV-NEXT: movl   %edi, %edi
+
+; 64: emptyfunc:
+; 64-NEXT: # %bb.0:
+; 64-NEXT: xchgw   %ax, %ax
+
+; From code: int emptyfunc() {}
+define i32 @emptyfunc() "patchable-function"="prologue-short-redirect" {
+  unreachable
+}
+
+
+; Hotpatch feature must ensure no jump within the function goes to the first instruction.
+; From code:
+; void jmp_to_start(char *b) {
+;   do {
+;   } while ((++(*b++)));
+; }
+
+; CHECK-ALIGN:  .p2align  4, 0x90
+; CHECK-ALIGN: _jmp_to_start:
+
+; 32: jmp_to_start:
+; 32CFI-NEXT: .cfi_startproc
+; 32-NEXT: # %bb.0:
+; XCHG-NEXT: xchgw   %ax, %ax
+; MOV-NEXT: movl   %edi, %edi
+
+; 64: jmp_to_start:
+; 64-NEXT: # %bb.0:
+; 64-NEXT: xchgw   %ax, %ax
+
+define dso_local void @jmp_to_start(ptr inreg nocapture noundef %b) "patchable-function"="prologue-short-redirect" {
+entry:
+  br label %do.body
+do.body:                                          ; preds = %do.body, %entry
+  %b.addr.0 = phi ptr [ %b, %entry ], [ %incdec.ptr, %do.body ]
+  %incdec.ptr = getelementptr inbounds i8, ptr %b.addr.0, i64 1
+  %0 = load i8, ptr %b.addr.0, align 1
+  %inc = add i8 %0, 1
+  store i8 %inc, ptr %b.addr.0, align 1
+  %tobool.not = icmp eq i8 %inc, 0
+  br i1 %tobool.not, label %do.end, label %do.body
+do.end:                                           ; preds = %do.body
+  ret void
+}