i#1578 arm detach: Restore TLS register on detach

Sets os_should_swap_state() to true. Adds support for os_swap_context() twice in a row with the same state. Removes os_swap_context_go_native(). The api.detach test now passes on AArch64. Enables all of the api.* and api.static_* tests for AArchXX, except api.startstop( crash) and api.static_prepop (hang) which will be fixed separately. Fixes a build error in the api.static_crash test: a missing header. Issue: #1578, #1582
DynamoRIO · Sep 10, 2020 · a136238 · a136238
1 parent db2d201
commit a136238
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 62 deletions.
diff --git a/core/arch/interp.c b/core/arch/interp.c
@@ -4114,8 +4114,7 @@ build_bb_ilist(dcontext_t *dcontext, build_bb_t *bb)
         dcontext->native_exec_postsyscall = bb->start_pc;
         dcontext->next_tag = BACK_TO_NATIVE_AFTER_SYSCALL;
         dynamo_thread_not_under_dynamo(dcontext);
-        /* i#1582: required for now on ARM */
-        IF_UNIX(os_swap_context_go_native(dcontext, DR_STATE_GO_NATIVE));
+        IF_UNIX(os_swap_context(dcontext, true /*to app*/, DR_STATE_GO_NATIVE));
         /* i#1921: for now we do not support re-attach, so remove handlers */
         os_process_not_under_dynamorio(dcontext);
         bb_build_abort(dcontext, true /*free vmlist*/, false /*don't unlock*/);

diff --git a/core/unix/os.c b/core/unix/os.c
@@ -2637,6 +2637,13 @@ os_swap_dr_tls(dcontext_t *dcontext, bool to_app)
             os_set_dr_tls_base(dcontext, real_tls, (byte *)real_tls);
         }
     }
+#elif defined(AARCHXX)
+    /* For aarchxx we don't have a separate thread register for DR, and we
+     * always leave the DR pointer in the slot inside the app's or privlib's TLS.
+     * That means we have nothing to do here.
+     * For SYS_clone, we are ok with the parent's TLS being inherited until
+     * new_thread_setup() calls set_thread_register_from_clone_record().
+     */
 #endif
 }
 
@@ -2698,12 +2705,7 @@ os_should_swap_state(void)
     return (INTERNAL_OPTION(mangle_app_seg) &&
             IF_CLIENT_INTERFACE_ELSE(INTERNAL_OPTION(private_loader), false));
 #elif defined(AARCHXX)
-    /* FIXME i#1582: this should return true, but there is a lot of complexity
-     * getting os_switch_seg_to_context() to do the right then when called
-     * at main thread init, secondary thread init, early and late injection,
-     * and thread exit, since it is fragile with its writes to app TLS.
-     */
-    return false;
+    return IF_CLIENT_INTERFACE_ELSE(INTERNAL_OPTION(private_loader), false);
 #endif
 }
 
@@ -2738,20 +2740,6 @@ os_swap_context(dcontext_t *dcontext, bool to_app, dr_state_flags_t flags)
         os_swap_dr_tls(dcontext, to_app);
 }
 
-void
-os_swap_context_go_native(dcontext_t *dcontext, dr_state_flags_t flags)
-{
-#ifdef AARCHXX
-    /* FIXME i#1582: remove this routine once os_should_swap_state()
-     * is not disabled and we can actually call
-     * os_swap_context_go_native() safely from multiple places.
-     */
-    os_switch_seg_to_context(dcontext, LIB_SEG_TLS, true /*to app*/);
-#else
-    os_swap_context(dcontext, true /*to app*/, flags);
-#endif
-}
-
 void
 os_thread_under_dynamo(dcontext_t *dcontext)
 {
@@ -6453,6 +6441,10 @@ os_switch_seg_to_context(dcontext_t *dcontext, reg_id_t seg, bool to_app)
     os_thread_data_t *ostd = (os_thread_data_t *)dcontext->os_field;
     ASSERT(INTERNAL_OPTION(private_loader));
     if (to_app) {
+        /* We need to handle being called when we're already in the requested state. */
+        ptr_uint_t cur_seg = read_thread_register(LIB_SEG_TLS);
+        if ((void *)cur_seg == os_tls->app_lib_tls_base)
+            return true;
         /* On switching to app's TLS, we need put DR's TLS base into app's TLS
          * at the same offset so it can be loaded on entering code cache.
          * Otherwise, the context switch code on entering fcache will fault on
@@ -6476,6 +6468,10 @@ os_switch_seg_to_context(dcontext_t *dcontext, reg_id_t seg, bool to_app)
             __FUNCTION__, (to_app ? "app" : "dr"), os_tls->app_lib_tls_base);
         res = write_thread_register(os_tls->app_lib_tls_base);
     } else {
+        /* We need to handle being called when we're already in the requested state. */
+        ptr_uint_t cur_seg = read_thread_register(LIB_SEG_TLS);
+        if ((void *)cur_seg == ostd->priv_lib_tls_base)
+            return true;
         /* Restore the app's TLS slot that we used for storing DR's TLS base,
          * and put DR's TLS base back to privlib's TLS slot.
          */

diff --git a/core/unix/os_exports.h b/core/unix/os_exports.h
@@ -153,7 +153,7 @@ extern uint android_tls_base_offs;
  *   {
  *     dtv_t *dtv;
  *     void *private;
- *   } tcbhead_t;
+ *   } tcb_head_t;
  * When using the private loader, we control all the TLS allocation and
  * should be able to avoid using that field.
  * This is also used in asm code, so we use literal instead of sizeof.
@@ -203,8 +203,6 @@ ushort
 os_get_app_tls_reg_offset(reg_id_t seg);
 void *
 os_get_app_tls_base(dcontext_t *dcontext, reg_id_t seg);
-void
-os_swap_context_go_native(dcontext_t *dcontext, dr_state_flags_t flags);
 
 #ifdef DEBUG
 void

diff --git a/core/unix/tls_linux_aarchxx.c b/core/unix/tls_linux_aarchxx.c
@@ -1,5 +1,5 @@
 /* *******************************************************************************
- * Copyright (c) 2014-2019 Google, Inc.  All rights reserved.
+ * Copyright (c) 2014-2020 Google, Inc.  All rights reserved.
  * *******************************************************************************/
 
 /*
@@ -66,6 +66,9 @@ void
 tls_thread_init(os_local_state_t *os_tls, byte *segment)
 {
     ASSERT((byte *)(os_tls->self) == segment);
+    /* XXX: Keep whether we change the thread register consistent with
+     * os_should_swap_state() and os_switch_seg_to_context() code.
+     */
     if (IF_CLIENT_INTERFACE_ELSE(INTERNAL_OPTION(private_loader), false)) {
         LOG(GLOBAL, LOG_THREADS, 2, "tls_thread_init: cur priv lib tls base is " PFX "\n",
             os_tls->os_seg_info.priv_lib_tls_base);

diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt
@@ -2776,40 +2776,41 @@ if (CLIENT_INTERFACE)
     endif ()
   endif (ARM)
 
-  if (NOT ARM AND NOT AARCH64) # FIXME i#1578: fix detach on ARM/AArch64
+  if (NOT AARCHXX) # TODO i#1578: Crashing on AArch64.
     tobuild_api(api.startstop api/startstop.c "" "" OFF OFF)
     link_with_pthread(api.startstop)
-    tobuild_api(api.detach api/detach.c "" "" OFF OFF)
-    link_with_pthread(api.detach)
-    if (LINUX AND X64) # Will take extra work to port to 32-bit.
-      tobuild_api(api.detach_state api/detach_state.c "" "" OFF OFF)
-      target_include_directories(api.detach_state PRIVATE
-        ${CMAKE_CURRENT_SOURCE_DIR}/api)
-      link_with_pthread(api.detach_state)
-      if (proc_supports_avx512)
-        append_property_string(TARGET api.detach_state COMPILE_FLAGS "${CFLAGS_AVX512}")
-        set(api.detach_state_runavx512 1)
-      elseif (proc_supports_avx)
-        append_property_string(TARGET api.detach_state COMPILE_FLAGS "${CFLAGS_AVX}")
-        set(api.detach_state_runavx 1)
-      endif ()
+  endif ()
+  tobuild_api(api.detach api/detach.c "" "" OFF OFF)
+  link_with_pthread(api.detach)
+  # TODO: port detach_state to aarchxx
+  if (X86 AND LINUX AND X64) # Will take extra work to port to 32-bit.
+    tobuild_api(api.detach_state api/detach_state.c "" "" OFF OFF)
+    target_include_directories(api.detach_state PRIVATE
+      ${CMAKE_CURRENT_SOURCE_DIR}/api)
+    link_with_pthread(api.detach_state)
+    if (proc_supports_avx512)
+      append_property_string(TARGET api.detach_state COMPILE_FLAGS "${CFLAGS_AVX512}")
+      set(api.detach_state_runavx512 1)
+    elseif (proc_supports_avx)
+      append_property_string(TARGET api.detach_state COMPILE_FLAGS "${CFLAGS_AVX}")
+      set(api.detach_state_runavx 1)
     endif ()
-    if (NOT WIN32) # XXX i#2611: fix for Windows
-      if (X64)
-        set(detach_spawn_name api.detach_spawn)
-      else ()
-        # FIXME i#2694: failing sometimes on 32-bit Linux.
-        set(detach_spawn_name api.detach_spawn_FLAKY)
-      endif ()
-      set(detach_spawn_stress_name api.detach_spawn_stress_FLAKY)
-      set(detach_spawn_quick_exit_name api.detach_spawn_quick_exit)
-      tobuild_api(${detach_spawn_name} api/detach_spawn.c "" "" OFF OFF)
-      link_with_pthread(${detach_spawn_name})
-      tobuild_api(${detach_spawn_stress_name} api/detach_spawn_stress.c "" "" OFF OFF)
-      link_with_pthread(${detach_spawn_stress_name})
-      tobuild_api(${detach_spawn_quick_exit_name} api/detach_spawn_quick_exit.c "" "" OFF OFF)
-      link_with_pthread(${detach_spawn_quick_exit_name})
+  endif ()
+  if (NOT WIN32) # XXX i#2611: fix for Windows
+    if (X64)
+      set(detach_spawn_name api.detach_spawn)
+    else ()
+      # FIXME i#2694: failing sometimes on 32-bit Linux.
+      set(detach_spawn_name api.detach_spawn_FLAKY)
     endif ()
+    set(detach_spawn_stress_name api.detach_spawn_stress_FLAKY)
+    set(detach_spawn_quick_exit_name api.detach_spawn_quick_exit)
+    tobuild_api(${detach_spawn_name} api/detach_spawn.c "" "" OFF OFF)
+    link_with_pthread(${detach_spawn_name})
+    tobuild_api(${detach_spawn_stress_name} api/detach_spawn_stress.c "" "" OFF OFF)
+    link_with_pthread(${detach_spawn_stress_name})
+    tobuild_api(${detach_spawn_quick_exit_name} api/detach_spawn_quick_exit.c "" "" OFF OFF)
+    link_with_pthread(${detach_spawn_quick_exit_name})
   endif ()
   if (X86)
     if (X64)
@@ -2867,9 +2868,7 @@ if (CLIENT_INTERFACE)
   # Test DR as a static library
   # XXX i#1996: not fully supported on Android yet
   # XXX i#1997: not fully supported on Mac yet
-  # XXX i#1551: start/stop API NYI on ARM
-  # XXX i#1569: start/stop API NYI on AArch64
-  if (NOT ANDROID AND NOT APPLE AND NOT ARM AND NOT AARCH64)
+  if (NOT ANDROID AND NOT APPLE)
     tobuild_api(api.static_startstop api/static_startstop.c "" "" OFF ON)
     target_link_libraries(api.static_startstop ${libmath})
     tobuild_api(api.static_noclient api/static_noclient.c "" "" OFF ON)
@@ -2878,8 +2877,10 @@ if (CLIENT_INTERFACE)
     target_link_libraries(api.static_noinit ${libmath})
     tobuild_api(api.static_detach api/static_detach.c "" "" OFF ON)
     target_link_libraries(api.static_detach ${libmath})
-    tobuild_api(api.static_prepop api/static_prepop.c "" "" OFF ON)
-    target_link_libraries(api.static_prepop ${libmath})
+    if (NOT AARCHXX) # TODO i#1578: Hanging on AArch64.
+      tobuild_api(api.static_prepop api/static_prepop.c "" "" OFF ON)
+      target_link_libraries(api.static_prepop ${libmath})
+    endif ()
 
     if (NOT WIN32)
       tobuild_api(api.static_signal api/static_signal.c "" "" OFF ON)

diff --git a/suite/tests/api/static_crash.c b/suite/tests/api/static_crash.c
@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2016 Google, Inc.  All rights reserved.
+ * Copyright (c) 2016-2020 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -36,6 +36,7 @@
 #endif
 #include "dr_api.h"
 #include "tools.h"
+#include <math.h>
 #include <unistd.h>
 #include <signal.h>