i#4271: Fix signal-syscall race handling on AArch64. (#4453)

Fixes logic to detect the two jmps preceding a syscall on AArch64. These jmps are used to bypass the syscall, which bounds the time before exiting code cache and hence delivering the signal.

Enables the linux.signal_racesys test on AArch64, which reproduces the assert failure in current implementation. This required adding the missing implementation to check for pending signals before entering fcache in append_fcache_enter_prologue.

Makes the signals_pending char as signed explicitly, using the existing sbyte alias. This is required because chars are unsigned by default on ARM. Also, adds support for OP_ldrsb instrs on AArch64 to load this data.

Fixes logic for skip_pc detection on ARM32. The existing logic does not work when skip_pc is pointing to the exit cti_pc. This prevents fragment re-linking and may cause performance issues.

Fixes: #4271, #2043

core/CMakeLists.txt

@@ -264,6 +264,7 @@ set(ARCH_SRCS
   arch/arch.c
   arch/emit_utils_shared.c
   arch/${ARCH_NAME}/emit_utils.c
+  arch/${ARCH_NAME_SHARED}/emit_utils.c
   # TODO i#1684: Link with drdecode rather than compiling all the same files.
   ${DECODER_SRCS}
   arch/interp.c

core/arch/aarch64/emit_utils.c

@@ -381,50 +381,6 @@ insert_fragment_prefix(dcontext_t *dcontext, fragment_t *f)
 /*             THREAD-PRIVATE/SHARED ROUTINE GENERATION                    */
 /***************************************************************************/
 
-/* Load DR's TLS base to dr_reg_stolen via reg_base. */
-static void
-insert_load_dr_tls_base(dcontext_t *dcontext, instrlist_t *ilist, instr_t *where,
-                        reg_id_t reg_base)
-{
-    /* Load TLS base from user-mode thread pointer/ID register:
-     * mrs reg_base, tpidr_el0
-     */
-    PRE(ilist, where,
-        INSTR_CREATE_mrs(dcontext, opnd_create_reg(reg_base),
-                         opnd_create_reg(DR_REG_TPIDR_EL0)));
-    /* ldr dr_reg_stolen, [reg_base, DR_TLS_BASE_OFFSET] */
-    PRE(ilist, where,
-        XINST_CREATE_load(dcontext, opnd_create_reg(dr_reg_stolen),
-                          OPND_CREATE_MEMPTR(reg_base, DR_TLS_BASE_OFFSET)));
-}
-
-/* Having only one thread register (TPIDR_EL0) shared between app and DR,
- * we steal a register for DR's TLS base in the code cache, and store
- * DR's TLS base into an private lib's TLS slot for accessing in C code.
- * On entering the code cache (fcache_enter):
- * - grab gen routine's parameter dcontext and put it into REG_DCXT
- * - load DR's TLS base into dr_reg_stolen from privlib's TLS
- */
-void
-append_fcache_enter_prologue(dcontext_t *dcontext, instrlist_t *ilist, bool absolute)
-{
-    ASSERT_NOT_IMPLEMENTED(!absolute &&
-                           !TEST(SELFPROT_DCONTEXT, dynamo_options.protect_mask));
-    /* Grab gen routine's parameter dcontext and put it into REG_DCXT:
-     * mov x(dxct), x0
-     */
-    APP(ilist,
-        XINST_CREATE_move(dcontext, opnd_create_reg(REG_DCXT),
-                          opnd_create_reg(DR_REG_X0)));
-
-    /* FIXME i#2019: check dcontext->signals_pending.  First we need a way to
-     * create a conditional branch b.le.
-     */
-
-    /* set up stolen reg */
-    insert_load_dr_tls_base(dcontext, ilist, NULL /*append*/, SCRATCH_REG0);
-}
-
 void
 append_call_exit_dr_hook(dcontext_t *dcontext, instrlist_t *ilist, bool absolute,
                          bool shared)

core/arch/aarchxx/emit_utils.c

@@ -0,0 +1,112 @@
+/* **********************************************************
+ * Copyright (c) 2014-2020 Google, Inc.  All rights reserved.
+ * Copyright (c) 2016 ARM Limited. All rights reserved.
+ * **********************************************************/
+
+/*
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * * Redistributions of source code must retain the above copyright notice,
+ *   this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright notice,
+ *   this list of conditions and the following disclaimer in the documentation
+ *   and/or other materials provided with the distribution.
+ *
+ * * Neither the name of ARM Limited nor the names of its contributors may be
+ *   used to endorse or promote products derived from this software without
+ *   specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL ARM LIMITED OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ */
+
+#include "../globals.h"
+#include "instr_create.h"
+#include "instrlist.h"
+#include "instrument.h"
+
+#define APP instrlist_meta_append
+#define PRE instrlist_meta_preinsert
+
+#define OPND_ARG1 opnd_create_reg(DR_REG_R0)
+
+/* Load DR's TLS base to dr_reg_stolen via reg_base */
+static void
+insert_load_dr_tls_base(dcontext_t *dcontext, instrlist_t *ilist, instr_t *where,
+                        reg_id_t reg_base)
+{
+#ifdef AARCH64
+    /* Load TLS base from user-mode thread pointer/ID register:
+     * mrs reg_base, tpidr_el0
+     */
+    PRE(ilist, where,
+        INSTR_CREATE_mrs(dcontext, opnd_create_reg(reg_base),
+                         opnd_create_reg(DR_REG_TPIDR_EL0)));
+#else // ARM
+    /* load TLS base from user-read-only-thread-ID register
+     * mrc p15, 0, reg_base, c13, c0, 3
+     */
+    PRE(ilist, where,
+        INSTR_CREATE_mrc(dcontext, opnd_create_reg(reg_base),
+                         OPND_CREATE_INT(USR_TLS_COPROC_15), OPND_CREATE_INT(0),
+                         opnd_create_reg(DR_REG_CR13), opnd_create_reg(DR_REG_CR0),
+                         OPND_CREATE_INT(USR_TLS_REG_OPCODE)));
+#endif
+    /* ldr dr_reg_stolen, [reg_base, DR_TLS_BASE_OFFSET] */
+    PRE(ilist, where,
+        XINST_CREATE_load(dcontext, opnd_create_reg(dr_reg_stolen),
+                          OPND_CREATE_MEMPTR(reg_base, DR_TLS_BASE_OFFSET)));
+}
+
+/* Having only one thread register (TPIDRURO for ARM, TPIDR_EL0 for AARCH64) shared
+ * between app and DR, we steal a register for DR's TLS base in the code cache,
+ * and store DR's TLS base into an private lib's TLS slot for accessing in C code.
+ * On entering the code cache (fcache_enter):
+ * - grab gen routine's parameter dcontext and put it into REG_DCXT
+ * - load DR's TLS base into dr_reg_stolen from privlib's TLS
+ */
+void
+append_fcache_enter_prologue(dcontext_t *dcontext, instrlist_t *ilist, bool absolute)
+{
+#ifdef UNIX
+    instr_t *no_signals = INSTR_CREATE_label(dcontext);
+    /* save callee-saved reg in case we return for a signal */
+    APP(ilist,
+        XINST_CREATE_move(dcontext, opnd_create_reg(DR_REG_R1),
+                          opnd_create_reg(REG_DCXT)));
+#endif
+    ASSERT_NOT_IMPLEMENTED(!absolute &&
+                           !TEST(SELFPROT_DCONTEXT, dynamo_options.protect_mask));
+    /* grab gen routine's parameter dcontext and put it into REG_DCXT */
+    APP(ilist, XINST_CREATE_move(dcontext, opnd_create_reg(REG_DCXT), OPND_ARG1));
+#ifdef UNIX
+    APP(ilist,
+        INSTR_CREATE_ldrsb(dcontext, opnd_create_reg(DR_REG_R2),
+                           OPND_DC_FIELD(absolute, dcontext, OPSZ_1, SIGPENDING_OFFSET)));
+    APP(ilist,
+        XINST_CREATE_cmp(dcontext, opnd_create_reg(DR_REG_R2), OPND_CREATE_INT8(0)));
+    APP(ilist,
+        XINST_CREATE_jump_cond(dcontext, DR_PRED_LE, opnd_create_instr(no_signals)));
+    /* restore callee-saved reg */
+    APP(ilist,
+        XINST_CREATE_move(dcontext, opnd_create_reg(REG_DCXT),
+                          opnd_create_reg(DR_REG_R1)));
+    APP(ilist,
+        IF_AARCH64_ELSE(INSTR_CREATE_br, INSTR_CREATE_bx)(dcontext,
+                                                          opnd_create_reg(DR_REG_LR)));
+    APP(ilist, no_signals);
+#endif
+    /* set up stolen reg */
+    insert_load_dr_tls_base(dcontext, ilist, NULL /*append*/, SCRATCH_REG0);
+}

core/arch/arm/emit_utils.c

@@ -593,68 +593,6 @@ insert_fragment_prefix(dcontext_t *dcontext, fragment_t *f)
 
 /* helper functions for emit_fcache_enter_common */
 
-#define OPND_ARG1 opnd_create_reg(DR_REG_R0)
-
-/* Load DR's TLS base to dr_reg_stolen via reg_base */
-static void
-insert_load_dr_tls_base(dcontext_t *dcontext, instrlist_t *ilist, instr_t *where,
-                        reg_id_t reg_base)
-{
-    /* load TLS base from user-read-only-thread-ID register
-     * mrc p15, 0, reg_base, c13, c0, 3
-     */
-    PRE(ilist, where,
-        INSTR_CREATE_mrc(dcontext, opnd_create_reg(reg_base),
-                         OPND_CREATE_INT(USR_TLS_COPROC_15), OPND_CREATE_INT(0),
-                         opnd_create_reg(DR_REG_CR13), opnd_create_reg(DR_REG_CR0),
-                         OPND_CREATE_INT(USR_TLS_REG_OPCODE)));
-    /* ldr dr_reg_stolen, [reg_base, DR_TLS_BASE_OFFSET] */
-    PRE(ilist, where,
-        XINST_CREATE_load(dcontext, opnd_create_reg(dr_reg_stolen),
-                          OPND_CREATE_MEMPTR(reg_base, DR_TLS_BASE_OFFSET)));
-}
-
-/* Having only one thread register (TPIDRURO) shared between app and DR,
- * we steal a register for DR's TLS base in the code cache,
- * and store DR's TLS base into an private lib's TLS slot for accessing in C code.
- * On entering the code cache (fcache_enter):
- * - grab gen routine's parameter dcontext and put it into REG_DCXT
- * - load DR's TLS base into dr_reg_stolen from privlib's TLS
- */
-void
-append_fcache_enter_prologue(dcontext_t *dcontext, instrlist_t *ilist, bool absolute)
-{
-#ifdef UNIX
-    instr_t *no_signals = INSTR_CREATE_label(dcontext);
-    /* save callee-saved reg in case we return for a signal */
-    APP(ilist,
-        XINST_CREATE_move(dcontext, opnd_create_reg(DR_REG_R1),
-                          opnd_create_reg(REG_DCXT)));
-#endif
-    ASSERT_NOT_IMPLEMENTED(!absolute &&
-                           !TEST(SELFPROT_DCONTEXT, dynamo_options.protect_mask));
-    /* grab gen routine's parameter dcontext and put it into REG_DCXT */
-    APP(ilist, XINST_CREATE_move(dcontext, opnd_create_reg(REG_DCXT), OPND_ARG1 /*r0*/));
-#ifdef UNIX
-    APP(ilist,
-        INSTR_CREATE_ldrsb(dcontext, opnd_create_reg(DR_REG_R2),
-                           OPND_DC_FIELD(absolute, dcontext, OPSZ_1, SIGPENDING_OFFSET)));
-    APP(ilist,
-        XINST_CREATE_cmp(dcontext, opnd_create_reg(DR_REG_R2), OPND_CREATE_INT8(0)));
-    APP(ilist,
-        INSTR_PRED(INSTR_CREATE_b(dcontext, opnd_create_instr(no_signals)), DR_PRED_LE));
-    /* restore callee-saved reg */
-    APP(ilist,
-        XINST_CREATE_move(dcontext, opnd_create_reg(REG_DCXT),
-                          opnd_create_reg(DR_REG_R1)));
-    APP(ilist, INSTR_CREATE_bx(dcontext, opnd_create_reg(DR_REG_LR)));
-    APP(ilist, no_signals);
-#endif
-
-    /* set up stolen reg */
-    insert_load_dr_tls_base(dcontext, ilist, NULL /*append*/, SCRATCH_REG0);
-}
-
 void
 append_call_exit_dr_hook(dcontext_t *dcontext, instrlist_t *ilist, bool absolute,
                          bool shared)

core/arch/mangle_shared.c

@@ -736,15 +736,21 @@ mangle_syscall_code(dcontext_t *dcontext, fragment_t *f, byte *pc, bool skip)
         prev_pc = pc;
         pc = decode(dcontext, pc, &instr);
         ASSERT(pc != NULL); /* our own code! */
-        if (instr_get_opcode(&instr) ==
-            OP_jmp_short
-                /* For A32 it's not OP_b_short */
-                IF_ARM(||
-                       (instr_get_opcode(&instr) == OP_jmp &&
-                        opnd_get_pc(instr_get_target(&instr)) == pc + ARM_INSTR_SIZE)))
+        if (instr_get_opcode(&instr) == OP_jmp_short) {
+#    ifdef AARCH64
+            /* For A64, both skip_pc and cti_pc are an OP_jmp_short instr. */
+            skip_pc = cti_pc;
+            cti_pc = prev_pc;
+#    else
             skip_pc = prev_pc;
-        else if (instr_get_opcode(&instr) == OP_jmp)
+#    endif
+        } else if (instr_get_opcode(&instr) == OP_jmp) {
+#    ifdef ARM
+            /* For A32, both skip_pc and cti_pc are an OP_jmp instr. */
+            skip_pc = cti_pc;
+#    endif
             cti_pc = prev_pc;
+        }
         if (pc >= stop_pc) {
             LOG(THREAD, LOG_SYSCALLS, 3, "\tno syscalls found\n");
             instr_free(dcontext, &instr);

core/dispatch.c

@@ -503,7 +503,7 @@ dispatch_enter_fcache(dcontext_t *dcontext, fragment_t *targetf)
 #endif
     );
 #ifdef UNIX
-    if (dcontext->signals_pending) {
+    if (dcontext->signals_pending > 0) {
         /* i#2019: the fcache_enter generated code starts with a check for pending
          * signals, allowing the signal handling code to simply queue signals that
          * arrive in DR code and only attempt to unlink for interruption points known

core/globals.h

@@ -912,7 +912,12 @@ struct _dcontext_t {
 
     dr_where_am_i_t whereami; /* where control is at the moment */
 #ifdef UNIX
-    char signals_pending; /* != 0: pending; < 0: currently handling one */
+    /* On ARM based machines, char is unsigned by default.
+     * https://www.arm.linux.org.uk/docs/faqs/signedchar.php
+     * But we need _signed_ char, so we use sbyte which explicitly qualifies
+     * as that.
+     */
+    sbyte signals_pending; /* != 0: pending; < 0: currently handling one */
 #endif
 
     /************* end of offset-crucial fields *********************/

core/ir/aarch64/instr_create.h

@@ -567,6 +567,8 @@ enum {
     instr_create_2dst_1src(dc, OP_ldp, rt1, rt2, mem)
 #define INSTR_CREATE_ldr(dc, Rd, mem) instr_create_1dst_1src((dc), OP_ldr, (Rd), (mem))
 #define INSTR_CREATE_ldrb(dc, Rd, mem) instr_create_1dst_1src(dc, OP_ldrb, Rd, mem)
+#define INSTR_CREATE_ldrsb(dc, Rd, mem) \
+    instr_create_1dst_1src((dc), OP_ldrsb, (Rd), (mem))
 #define INSTR_CREATE_ldrh(dc, Rd, mem) instr_create_1dst_1src(dc, OP_ldrh, Rd, mem)
 #define INSTR_CREATE_ldur(dc, rt, mem) instr_create_1dst_1src(dc, OP_ldur, rt, mem)
 #define INSTR_CREATE_ldar(dc, Rt, mem) instr_create_1dst_1src((dc), OP_ldar, (Rt), (mem))

core/unix/os_public.h

@@ -134,7 +134,6 @@ typedef kernel_sigcontext_t sigcontext_t;
 #    define SC_SYSNUM_REG SC_XAX
 #    define SC_RETURN_REG SC_XAX
 #elif defined(AARCH64)
-/* FIXME i#1569: NYI */
 #    define SC_XIP SC_FIELD(pc)
 #    define SC_FP SC_FIELD(regs[29])
 #    define SC_R0 SC_FIELD(regs[0])

suite/tests/CMakeLists.txt

@@ -3977,10 +3977,11 @@ if (UNIX)
   endif ()
   # i#784: test app behavior on alarm
   tobuild(linux.alarm linux/alarm.c)
-  # XXX i#2043: enable for A64 once append_fcache_enter_prologue() is finished.
-  if (NOT APPLE AND NOT ANDROID AND NOT AARCH64) # Test uses Linux-specific timer code.
-    tobuild(linux.signal_race linux/signal_race.c)
-    target_link_libraries(linux.signal_race rt)
+  if (NOT APPLE AND NOT ANDROID) # Test uses Linux-specific timer code.
+    if (NOT AARCH64) # TODO i#1569: Enable this for AArch64.
+      tobuild(linux.signal_race linux/signal_race.c)
+      target_link_libraries(linux.signal_race rt)
+    endif ()
     tobuild(linux.signal_racesys linux/signal_racesys.c)
     target_link_libraries(linux.signal_racesys rt)
   endif ()