diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp --- a/clang/lib/CodeGen/CodeGenFunction.cpp +++ b/clang/lib/CodeGen/CodeGenFunction.cpp @@ -885,7 +885,9 @@ // 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. diff --git a/llvm/include/llvm/Support/TargetOpcodes.def b/llvm/include/llvm/Support/TargetOpcodes.def --- a/llvm/include/llvm/Support/TargetOpcodes.def +++ b/llvm/include/llvm/Support/TargetOpcodes.def @@ -177,6 +177,8 @@ /// 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 diff --git a/llvm/lib/CodeGen/PatchableFunction.cpp b/llvm/lib/CodeGen/PatchableFunction.cpp --- a/llvm/lib/CodeGen/PatchableFunction.cpp +++ b/llvm/lib/CodeGen/PatchableFunction.cpp @@ -37,23 +37,6 @@ }; } -/// 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(); @@ -74,11 +57,28 @@ #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) diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp --- a/llvm/lib/Target/X86/X86MCInstLower.cpp +++ b/llvm/lib/Target/X86/X86MCInstLower.cpp @@ -1434,6 +1434,9 @@ 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); @@ -1442,9 +1445,11 @@ MCI.addOperand(*MaybeOperand); SmallString<256> Code; - SmallVector Fixups; - raw_svector_ostream VecOS(Code); - CodeEmitter->encodeInstruction(MCI, VecOS, Fixups, getSubtargetInfo()); + if (!EmptyInst) { + SmallVector Fixups; + raw_svector_ostream VecOS(Code); + CodeEmitter->encodeInstruction(MCI, VecOS, Fixups, getSubtargetInfo()); + } if (Code.size() < MinSize) { if (MinSize == 2 && Subtarget->is32Bit() && @@ -1470,8 +1475,8 @@ (void)NopSize; } } - - OutStreamer->emitInstruction(MCI, getSubtargetInfo()); + if (!EmptyInst) + OutStreamer->emitInstruction(MCI, getSubtargetInfo()); } // Lower a stackmap of the form: diff --git a/llvm/test/CodeGen/X86/patchable-prologue-debuginfo.ll b/llvm/test/CodeGen/X86/patchable-prologue-debuginfo.ll new file mode 100644 --- /dev/null +++ b/llvm/test/CodeGen/X86/patchable-prologue-debuginfo.ll @@ -0,0 +1,56 @@ +; 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 (git@gitlab-ncsa.ubisoft.org: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 (git@gitlab-ncsa.ubisoft.org: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) diff --git a/llvm/test/CodeGen/X86/patchable-prologue.ll b/llvm/test/CodeGen/X86/patchable-prologue.ll --- a/llvm/test/CodeGen/X86/patchable-prologue.ll +++ b/llvm/test/CodeGen/X86/patchable-prologue.ll @@ -5,10 +5,6 @@ ; 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) @@ -135,3 +131,63 @@ %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 +}