i#3098: fix shared ibt table races

Fixes two races with shared ibt tables:

+ Adding a new table entry must write the start_pc before the tag.
  This is accomplished with a new ENTRY_SET_TO_ENTRY hashtablex.h
  optional specifier.  For ARM #2502 a new MEMORY_STORE_BARRIER macro
  is added.

+ Resizing a table must not clear the tags in the old table to avoid
  losing the tag on the target_delete ibl path.

Adds a test api.ibl-stress which uses the DR IR to synthetically
construct thousands of basic blocks with indirect branches betweent
them.

To make the test work, relaxes several is-on-stack checks to support
pre-building basic blocks (#2463) from generated code or other
locations not known prior to starting the application.

Issue: #3098, #2502, #2463

Fixes #3098

core/arch/arch_exports.h

@@ -508,6 +508,7 @@ atomic_add_exchange_int64(volatile int64 *var, int64 value)
                              : "=r"(result), "=m"(var) \
                              : "0"(newval), "m"(var))
 
+#        define MEMORY_STORE_BARRIER() /* not needed on x86 */
 #        define SPINLOCK_PAUSE() __asm__ __volatile__("pause")
 #        ifdef X64
 #            define RDTSC_LL(llval)                                        \
@@ -657,6 +658,7 @@ DEF_ATOMIC_incdec(ATOMIC_INC_int, int, "w", "add") DEF_ATOMIC_incdec(ATOMIC_INC_
     return ret;
 }
 
+#        define MEMORY_STORE_BARRIER() __asm__ __volatile__("dmb st")
 #        define SPINLOCK_PAUSE()             \
             do {                             \
                 __asm__ __volatile__("wfi"); \
@@ -793,6 +795,7 @@ atomic_dec_becomes_zero(volatile int *var)
                                  : "r"(newval)                  \
                                  : "cc", "memory", "r2", "r3");
 
+#        define MEMORY_STORE_BARRIER() __asm__ __volatile__("dmb st")
 #        define SPINLOCK_PAUSE() __asm__ __volatile__("wfi") /* wait for interrupt */
 uint64
 proc_get_timestamp(void);

core/fragment.c

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2011-2017 Google, Inc.  All rights reserved.
+ * Copyright (c) 2011-2018 Google, Inc.  All rights reserved.
  * Copyright (c) 2000-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -644,6 +644,13 @@ static const fragment_entry_t fe_sentinel = { NULL_TAG, HASHLOOKUP_SENTINEL_STAR
 #define ENTRY_IS_INVALID(fe) IBL_ENTRY_IS_INVALID(fe)
 #define IBL_ENTRIES_ARE_EQUAL(fe1, fe2) ((fe1).tag_fragment == (fe2).tag_fragment)
 #define ENTRIES_ARE_EQUAL(table, fe1, fe2) IBL_ENTRIES_ARE_EQUAL(fe1, fe2)
+/* We set start_pc_fragment first to avoid races in a shared table where
+ * another thread matches the tag but then jumps to a bogus address. */
+#define ENTRY_SET_TO_ENTRY(e, f)                                                      \
+    (e).start_pc_fragment = (f).start_pc_fragment;                                    \
+    /* Ensure the start_pc_fragment store completes before the tag_fragment store: */ \
+    MEMORY_STORE_BARRIER();                                                           \
+    (e).tag_fragment = (f).tag_fragment
 #define HASHTABLE_WHICH_HEAP(flags) FRAGTABLE_WHICH_HEAP(flags)
 #define HTLOCK_RANK table_rwlock
 #define HASHTABLE_ENTRY_STATS 1
@@ -907,13 +914,12 @@ safely_nullify_tables(dcontext_t *dcontext, ibl_table_t *new_table,
             continue;
         }
         /* We need these writes to be atomic, so check that they're aligned. */
-        ASSERT(ALIGNED(&table[i].tag_fragment, 4));
-        ASSERT(ALIGNED(&table[i].start_pc_fragment, 4));
-        /* We update the tag first so that so that a thread that's skipping
-         * along a chain will exit ASAP. Breaking the chain is ok since we're
-         * nullifying the entire table.
+        ASSERT(ALIGNED(&table[i].tag_fragment, sizeof(table[i].tag_fragment)));
+        ASSERT(ALIGNED(&table[i].start_pc_fragment, sizeof(table[i].start_pc_fragment)));
+        /* We cannot set the tag to fe_empty.tag_fragment to break the hash chain
+         * as the target_delete path relies on acquiring the tag from the table entry,
+         * so we leave it alone.
          */
-        table[i].tag_fragment = fe_empty.tag_fragment;
         /* We set the payload to target_delete to induce a cache exit.
          *
          * The target_delete path leads to a loss of information -- we can't

core/hashtablex.h

@@ -68,6 +68,8 @@
  *     HASHTABLE_LOCKLESS_ACCESS tables
  *   bool ENTRIES_ARE_EQUAL(table, entry1, entry2)
  *     if using pointers, pointer equality is fine
+ *   void ENTRY_SET_TO_ENTRY(table_entry, new_entry)
+ *     This is optional; if omitted, "table_entry = new_entry" is used.
  *   ENTRY_TYPE ENTRY_EMPTY
  *   ENTRY_TYPE ENTRY_SENTINEL
  *     FIXME: to support structs we'll need lhs like AUX_ENTRY_SET_TO_SENTINEL
@@ -888,7 +890,11 @@ static inline bool HTNAME(hashtable_, NAME_KEY,
 #    endif
     if (ENTRY_IS_INVALID(table->table[hindex]))
         table->unlinked_entries--;
+#    ifdef ENTRY_SET_TO_ENTRY
+    ENTRY_SET_TO_ENTRY(table->table[hindex], e);
+#    else
     table->table[hindex] = e;
+#    endif
     ASSERT(!ENTRY_IS_INVALID(table->table[hindex]));
     LOG(THREAD_GET, LOG_HTABLE, 4,
         "hashtable_" KEY_STRING "_add: added " PFX " to %s at table[%u]\n", ENTRY_TAG(e),
@@ -2208,6 +2214,7 @@ HTNAME(, NAME_KEY, _table_t) *
 #undef ENTRY_IS_SENTINEL
 #undef ENTRY_IS_INVALID
 #undef ENTRIES_ARE_EQUAL
+#undef ENTRY_SET_TO_ENTRY
 #undef ENTRY_EMPTY
 #undef ENTRY_SENTINEL
 #undef TAGS_ARE_EQUAL

core/lib/instrument.c

@@ -7594,9 +7594,6 @@ dr_prepopulate_indirect_targets(dr_indirect_branch_type_t branch_type, app_pc *t
     uint i;
     if (dcontext == NULL)
         return false;
-#    ifdef UNIX
-    os_swap_context(dcontext, false /*to dr*/, DR_STATE_GO_NATIVE);
-#    endif
     /* Initially I took in an opcode and used extract_branchtype(instr_branch_type())
      * but every use case had to make a fake instr to get the opcode and had no
      * good cross-platform method so I switched to an enum.  We're unlikely to
@@ -7610,6 +7607,9 @@ dr_prepopulate_indirect_targets(dr_indirect_branch_type_t branch_type, app_pc *t
     }
     SYSLOG_INTERNAL_INFO("pre-populating ibt[%d] table for %d tags", ibl_type,
                          tags_count);
+#    ifdef UNIX
+    os_swap_context(dcontext, false /*to dr*/, DR_STATE_GO_NATIVE);
+#    endif
     for (i = 0; i < tags_count; i++) {
         fragment_add_ibl_target(dcontext, tags[i], ibl_type);
     }

core/unix/os.c

@@ -9304,7 +9304,13 @@ get_stack_bounds(dcontext_t *dcontext, byte **base, byte **top)
             ok = get_memory_info_from_os((app_pc)get_mcontext(dcontext)->xsp,
                                          &ostd->stack_base, &size, NULL);
         }
-        ASSERT(ok);
+        if (!ok) {
+             /* This can happen with dr_prepopulate_cache() before we start running
+              * the app.
+              */
+             ASSERT(!dynamo_started);
+             return false;
+        }
         ostd->stack_top = ostd->stack_base + size;
         LOG(THREAD, LOG_THREADS, 1, "App stack is " PFX "-" PFX "\n", ostd->stack_base,
             ostd->stack_top);

core/vmareas.c

@@ -3855,7 +3855,11 @@ is_on_stack(dcontext_t *dcontext, app_pc pc, vm_area_t *area)
     }
     if (query_esp) {
         ok = get_memory_info(esp, &esp_base, &size, NULL);
-        ASSERT(ok);
+        if (!ok) {
+          /* This can happen with dr_prepopulate_cache(). */
+          ASSERT(!dynamo_started);
+          return false;
+        }
         LOG(THREAD, LOG_VMAREAS, 3,
             "stack vs " PFX ": region " PFX ".." PFX ", esp " PFX "\n", pc, esp_base,
             esp_base + size, esp);

suite/tests/CMakeLists.txt

@@ -2289,6 +2289,18 @@ if (CLIENT_INTERFACE)
     tobuild_api(api.drdecode api/drdecode_x86.c "" "" ON OFF)
   endif ()
 
+  if (X86) # Generated code is x86-specific.
+    tobuild_api(api.ibl-stress api/ibl-stress.c
+      # This tests indirect branches between blocks.
+      # We use -checklevel 0 to disable the DOCHECK in check_thread_vm_area
+      # which makes building 150K bbs very slow.
+      "-disable_traces -shared_bb_ibt_tables -checklevel 0" "" OFF OFF)
+    if (UNIX)
+      target_link_libraries(api.ibl-stress ${libpthread})
+    endif ()
+    use_DynamoRIO_extension(api.ibl-stress drcontainers)
+  endif ()
+
   # XXX: we should expand this test of drsyms standalone to be cross-platform and
   # not just check libstdc++-6.dll.
   if (WIN32)