[clang] 3f3438a - [CodeGen][X86] Crash fixes for "patchable-function" pass
Sylvain Audi via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 30 04:30:19 PST 2022
Author: Sylvain Audi
Date: 2022-11-30T07:29:54-05:00
New Revision: 3f3438a596d4883ecd934fc725a9d5f620082c3b
URL: https://github.com/llvm/llvm-project/commit/3f3438a596d4883ecd934fc725a9d5f620082c3b
DIFF: https://github.com/llvm/llvm-project/commit/3f3438a596d4883ecd934fc725a9d5f620082c3b.diff
LOG: [CodeGen][X86] Crash fixes for "patchable-function" pass
This patch fixes crashes related with how PatchableFunction selects the instruction to make patchable:
- Ensure PatchableFunction skips all instructions that don't generate actual machine instructions.
- Handle the case where the first MachineBasicBlock is empty
- Removed support for 16 bit x86 architectures.
Note: another issue remains related with PatchableFunction, in the lowering part.
See https://github.com/llvm/llvm-project/issues/59039
Differential Revision: https://reviews.llvm.org/D137642
Added:
llvm/test/CodeGen/X86/patchable-prologue-debuginfo.ll
Modified:
clang/lib/CodeGen/CodeGenFunction.cpp
llvm/include/llvm/Support/TargetOpcodes.def
llvm/lib/CodeGen/PatchableFunction.cpp
llvm/lib/Target/X86/X86MCInstLower.cpp
llvm/test/CodeGen/X86/patchable-prologue.ll
Removed:
################################################################################
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 9a1ae23dcbd97..3686c945ab594 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -885,7 +885,9 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
// 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
index 52da5659e8058..06811a962c947 100644
--- a/llvm/include/llvm/Support/TargetOpcodes.def
+++ b/llvm/include/llvm/Support/TargetOpcodes.def
@@ -177,6 +177,8 @@ HANDLE_TARGET_OPCODE(FAULTING_OP)
/// 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
index 0f9da0637cedc..9449f143366f0 100644
--- a/llvm/lib/CodeGen/PatchableFunction.cpp
+++ b/llvm/lib/CodeGen/PatchableFunction.cpp
@@ -37,23 +37,6 @@ struct PatchableFunction : public MachineFunctionPass {
};
}
-/// 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 @@ bool PatchableFunction::runOnMachineFunction(MachineFunction &MF) {
#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
index e1d03d6010e03..feac636d96aa3 100644
--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1434,6 +1434,9 @@ void X86AsmPrinter::LowerPATCHABLE_OP(const MachineInstr &MI,
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 @@ void X86AsmPrinter::LowerPATCHABLE_OP(const MachineInstr &MI,
MCI.addOperand(*MaybeOperand);
SmallString<256> Code;
- SmallVector<MCFixup, 4> Fixups;
- raw_svector_ostream VecOS(Code);
- CodeEmitter->encodeInstruction(MCI, VecOS, Fixups, getSubtargetInfo());
+ 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() &&
@@ -1470,8 +1475,8 @@ void X86AsmPrinter::LowerPATCHABLE_OP(const MachineInstr &MI,
(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
index 0000000000000..e713418c8d8ed
--- /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 at 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 at 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
index f8a5782bc50ae..f1a39777a40bc 100644
--- 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 @@ 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
+}
More information about the cfe-commits
mailing list