From f9be7a67d33d9bdec342401a8ed84885672e9b67 Mon Sep 17 00:00:00 2001 From: Johan Lorensson Date: Mon, 26 Aug 2019 16:46:43 +0200 Subject: [PATCH] Fix special case tramp assert in x86/amd64. (#16476) https://github.com/mono/mono/pull/16408 temporary fixed a tramp issue on x86 (also exist on amd64). This commit fixes the underlying issue and re-enable the use of noreturn outside of netcore. The problem hit by the use of OP_NOT_REACHED on x86/amd64 is due to a specific case when the address to patch happens to fall at the start of a different managed method. Since the use of OP_NOT_REACHED can end a method with the call instruction on x86/amd64 in combination with how patch location (using the return address from stack) is resolved, the implementation in mono_arch_patch_callsite didn't take this case into account, reading incorrect patch data, triggering assert oh x86. So, based on timing we could end up with the following code: 025715B3 call 02570D98 025715B8 push ebp since the patch target is 025715B8, but also the start of a completely different method, method_start == origin_code. The fix is to detect this case on x86/amd64 (done on each arch since the way origin_code is detected is arch specific) and then use a method_start of NULL, that is already a supported scenario. --- mono/mini/method-to-ir.c | 2 -- mono/mini/tramp-amd64.c | 7 ++++++- mono/mini/tramp-x86.c | 12 +++++++++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/mono/mini/method-to-ir.c b/mono/mini/method-to-ir.c index f89b2dff563..c234c0d31ec 100644 --- a/mono/mini/method-to-ir.c +++ b/mono/mini/method-to-ir.c @@ -7188,12 +7188,10 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b virtual_ = FALSE; } -#ifdef ENABLE_NETCORE if (cmethod && method_does_not_return (cmethod)) { cfg->cbb->out_of_line = TRUE; noreturn = TRUE; } -#endif cdata.method = method; cdata.inst_tailcall = inst_tailcall; diff --git a/mono/mini/tramp-amd64.c b/mono/mini/tramp-amd64.c index f757c556649..b64226b0bf8 100644 --- a/mono/mini/tramp-amd64.c +++ b/mono/mini/tramp-amd64.c @@ -136,7 +136,12 @@ mono_arch_patch_callsite (guint8 *method_start, guint8 *orig_code, guint8 *addr) { guint8 *code; guint8 buf [16]; - gboolean can_write = mono_breakpoint_clean_code (method_start, orig_code, 14, buf, sizeof (buf)); + + // Since method_start is retrieved from function return address (below current call/jmp to patch) there is a case when + // last instruction of a function is the call (due to OP_NOT_REACHED) instruction and then directly followed by a + // different method. In that case current orig_code points into next method and method_start will also point into + // next method, not the method including the call to patch. For this specific case, fallback to using a method_start of NULL. + gboolean can_write = mono_breakpoint_clean_code (method_start != orig_code ? method_start : NULL, orig_code, 14, buf, sizeof (buf)); code = buf + 14; diff --git a/mono/mini/tramp-x86.c b/mono/mini/tramp-x86.c index 76808b8f13d..68d01a5362b 100644 --- a/mono/mini/tramp-x86.c +++ b/mono/mini/tramp-x86.c @@ -92,7 +92,12 @@ mono_arch_patch_callsite (guint8 *method_start, guint8 *orig_code, guint8 *addr) { guint8 *code; guint8 buf [8]; - gboolean can_write = mono_breakpoint_clean_code (method_start, orig_code, 8, buf, sizeof (buf)); + + // Since method_start is retrieved from function return address (below current call/jmp to patch) there is a case when + // last instruction of a function is the call (due to OP_NOT_REACHED) instruction and then directly followed by a + // different method. In that case current orig_code points into next method and method_start will also point into + // next method, not the method including the call to patch. For this specific case, fallback to using a method_start of NULL. + gboolean can_write = mono_breakpoint_clean_code (method_start != orig_code ? method_start : NULL, orig_code, 8, buf, sizeof (buf)); code = buf + 8; @@ -117,8 +122,9 @@ mono_arch_patch_callsite (guint8 *method_start, guint8 *orig_code, guint8 *addr) if (can_write) mono_atomic_xchg_i32 ((gint32*)(orig_code + 2), (guint)addr - ((guint)orig_code + 1) - 5); } else { - printf ("Invalid trampoline sequence: %x %x %x %x %x %x %x\n", code [0], code [1], code [2], code [3], - code [4], code [5], code [6]); + printf ("Invalid trampoline sequence: %x %x %x %x %x %x n", code [0], code [1], code [2], code [3], + code [4], code [5]); + g_assert_not_reached (); } } -- 2.11.4.GIT