From e32751850508005a4ee13e4375797f104430a3d7 Mon Sep 17 00:00:00 2001 From: Jay Krell Date: Fri, 30 Aug 2019 14:45:55 -0700 Subject: [PATCH] Cleanup x86_patch and amd64_patch. (mono/mono#16474) * Cleanup x86_patch and amd64_patch. amd64_patch has two incorrect looking probably unused cases. g_assert(0) and fix them. call and jmp indirect (one case) move reg RIP relative x86_patch is large-ish inline macro. Presumably for sharing with amd64, not inlining. Restructure as an inline function, called once. Try to make the code a little clearer also, with same functionality and efficiency (modulo the optimized perf of "if i == 4" vs. "if i") i.e. speak of instruction_size and offset_size, not pos and boolean size. Add to x86_patch the ability to patch jmp/call indirect, as amd64_patch has, but which was incorrect. This would seem useful for a later change (using "thunks" to patch nearby data instead of patching aligned instructions, indirect call/jmp vs. direct). amd64_patch largely delegates to x86_patch. One case asserts and delegates. Move the assert to the x86 side, where an optimizing compiler should know to remove it for x86 anyway. Change some assert to g_assert. Remove some unnecessary casts. Fix an assert (https://github.com/mono/mono/pull/16471). * Change g_assert back to assert except where making larger changes and less expansion. Commit migrated from https://github.com/mono/mono/commit/f735571f27738e3354f6d92650a0517d0482039d --- src/mono/mono/arch/x86/x86-codegen.h | 82 ++++++++++++++++++++++++++---------- src/mono/mono/mini/mini-amd64.c | 25 ++++++----- src/mono/mono/mini/mini-x86.c | 4 +- src/mono/mono/mini/mini-x86.h | 2 +- 4 files changed, 77 insertions(+), 36 deletions(-) diff --git a/src/mono/mono/arch/x86/x86-codegen.h b/src/mono/mono/arch/x86/x86-codegen.h index d402f79..9fd6523 100644 --- a/src/mono/mono/arch/x86/x86-codegen.h +++ b/src/mono/mono/arch/x86/x86-codegen.h @@ -359,29 +359,65 @@ typedef union { * the instruction is inspected for validity and the correct displacement * is inserted. */ -#define x86_patch(ins,target) \ - do { \ - unsigned char* pos = (ins) + 1; \ - int disp, size = 0; \ - switch (*(unsigned char*)(ins)) { \ - case 0xe8: case 0xe9: ++size; break; /* call, jump32 */ \ - case 0x0f: if (!(*pos >= 0x80 && *pos <= 0x8f)) assert (0); \ - ++size; ++pos; break; /* prefix for conditional jumps with 32-bit disp */ \ - case 0xe0: case 0xe1: case 0xe2: /* loop */ \ - case 0xeb: /* jump8 */ \ - /* conditional jump opcodes */ \ - case 0x70: case 0x71: case 0x72: case 0x73: \ - case 0x74: case 0x75: case 0x76: case 0x77: \ - case 0x78: case 0x79: case 0x7a: case 0x7b: \ - case 0x7c: case 0x7d: case 0x7e: case 0x7f: \ - break; \ - default: assert (0); \ - } \ - disp = (target) - pos; \ - if (size) x86_imm_emit32 (pos, disp - 4); \ - else if (x86_is_imm8 (disp - 1)) x86_imm_emit8 (pos, disp - 1); \ - else assert (0); \ - } while (0) +void +mono_x86_patch (guchar* code, gpointer target); + +// This is inline only to share with amd64. +// It is only for use by mono_x86_patch and mono_amd64_patch. +static inline void +mono_x86_patch_inline (guchar* code, gpointer target) +{ + int instruction_size = 2; // 2 or 5 or 6, code handles anything + int offset_size = 1; // 1 or 4 + + switch (*code) { + case 0xe8: case 0xe9: // call, jump32 + instruction_size = 5; + offset_size = 4; + break; + + case 0x0f: // 6byte conditional branches with 32bit offsets. + g_assert (code [1] >= 0x80 && code [1] <= 0x8F); + instruction_size = 6; + offset_size = 4; + break; + + case 0xe0: case 0xe1: case 0xe2: /* loop */ + case 0xeb: /* jump8 */ + /* conditional jump opcodes */ + case 0x70: case 0x71: case 0x72: case 0x73: + case 0x74: case 0x75: case 0x76: case 0x77: + case 0x78: case 0x79: case 0x7a: case 0x7b: + case 0x7c: case 0x7d: case 0x7e: case 0x7f: + break; + + case 0xFF: + // call or jmp indirect; patch the data, not the code. + // amd64 handles this elsewhere (RIP relative). + g_assert (code [1] == 0x15 || code [1] == 0x25); + g_assert (0); // For possible use later. + *(void**)(code + 2) = target; + return; + + default: + g_assert (0); + } + + // Offsets are relative to the end of the instruction. + ptrdiff_t const offset = (guchar*)target - (code + instruction_size); + + if (offset_size == 4) { + g_assert (offset == (gint32)offset); // for amd64 + code += instruction_size - 4; // Next line demands lvalue, not temp. + x86_imm_emit32 (code, offset); + } else { + g_assert (offset == (gint8)offset); + code += instruction_size - 1; // Next line demands lvalue, not temp. + x86_imm_emit8 (code, offset); + } +} + +#define x86_patch(ins, target) (mono_x86_patch ((ins), (target))) #define x86_breakpoint(inst) x86_byte (inst, 0xcc) diff --git a/src/mono/mono/mini/mini-amd64.c b/src/mono/mono/mini/mini-amd64.c index 5185a00..223e04b 100644 --- a/src/mono/mono/mini/mini-amd64.c +++ b/src/mono/mono/mini/mini-amd64.c @@ -173,6 +173,12 @@ amd64_use_imm32 (gint64 val) return amd64_is_imm32 (val); } +void +mono_x86_patch (unsigned char* code, gpointer target) +{ + mono_x86_patch_inline (code, target); +} + static void amd64_patch (unsigned char* code, gpointer target) { @@ -195,23 +201,22 @@ amd64_patch (unsigned char* code, gpointer target) } else if ((code [0] == 0x8b) && rex && x86_modrm_mod (code [1]) == 0 && x86_modrm_rm (code [1]) == 5) { /* mov 0(%rip), %dreg */ - *(guint32*)(code + 2) = (guint32)(guint64)target - 7; + g_assert (!1); // Historical code was incorrect. + ptrdiff_t const offset = (guchar*)target - (code + 6); + g_assert (offset == (gint32)offset); + *(gint32*)(code + 2) = (gint32)offset; } else if (code [0] == 0xff && (code [1] == 0x15 || code [1] == 0x25)) { /* call or jmp *(%rip) */ - *(guint32*)(code + 2) = ((guint32)(guint64)target) - 7; - } - else if (code [0] == 0xe8 || code [0] == 0xe9) { - /* call or jmp */ - gint64 disp = (guint8*)target - (guint8*)code; - g_assert (amd64_is_imm32 (disp)); - x86_patch (code, (unsigned char*)target); + // Patch the data, not the code. + g_assert (!2); // For possible use later. + *(void**)(code + 6 + *(gint32*)(code + 2)) = target; } else - x86_patch (code, (unsigned char*)target); + x86_patch (code, target); } -void +void mono_amd64_patch (unsigned char* code, gpointer target) { amd64_patch (code, target); diff --git a/src/mono/mono/mini/mini-x86.c b/src/mono/mono/mini/mini-x86.c index 38dcb38..41f2ab3 100644 --- a/src/mono/mono/mini/mini-x86.c +++ b/src/mono/mono/mini/mini-x86.c @@ -137,10 +137,10 @@ mono_arch_xregname (int reg) } } -void +void mono_x86_patch (unsigned char* code, gpointer target) { - x86_patch (code, (unsigned char*)target); + mono_x86_patch_inline (code, target); } #define FLOAT_PARAM_REGS 0 diff --git a/src/mono/mono/mini/mini-x86.h b/src/mono/mono/mini/mini-x86.h index af976b0..55f6850 100644 --- a/src/mono/mono/mini/mini-x86.h +++ b/src/mono/mono/mini/mini-x86.h @@ -344,7 +344,7 @@ void mono_x86_throw_corlib_exception (host_mgreg_t *regs, guint32 ex_token_index, host_mgreg_t eip, gint32 pc_offset); -void +void mono_x86_patch (unsigned char* code, gpointer target); gpointer -- 2.7.4