work around hipe_mfa_info_table lock omission

HiPE maintains per-MFA information such as native code entry
point in a table. This table was thought to be read-only at
runtime, except when the loader populates it, so it employed
no locking. That turned out to be incorrect: if there is an
apply of a previously unseen MFA, a native code stub for that
MFA is created and recorded in the table, causing it to grow.

Work around this for now by slapping a mutex around accesses
to that table.

Signed-off-by: Mikael Pettersson <[email protected]>

2 files changed, 92 insertions, 24 deletions

diff --git a/erts/emulator/beam/erl_lock_check.c b/erts/emulator/beam/erl_lock_check.c
index 25f1d420d1..937916272c 100644
--- a/erts/emulator/beam/erl_lock_check.c
+++ b/erts/emulator/beam/erl_lock_check.c
@@ -75,6 +75,9 @@ static erts_lc_lock_order_t erts_lock_order[] = {
      *						 the lock name)"
      */
 #ifdef ERTS_SMP
+#ifdef HIPE
+    {	"hipe_mfait_lock",			NULL			},
+#endif
     {	"driver_lock",				"driver_name"		},
     {	"port_lock",				"port_id"		},
 #endif
diff --git a/erts/emulator/hipe/hipe_bif0.c b/erts/emulator/hipe/hipe_bif0.c
index 032bf2e896..a91f0a5190 100644
--- a/erts/emulator/hipe/hipe_bif0.c
+++ b/erts/emulator/hipe/hipe_bif0.c
@@ -656,6 +656,7 @@ static void *hipe_get_emu_address(Eterm m, Eterm f, unsigned int arity, int is_r
 	address = hipe_find_emu_address(m, f, arity);
     if (!address) {
 	/* if not found, stub it via the export entry */
+	/* no lock needed around erts_export_get_or_make_stub() */
 	Export *export_entry = erts_export_get_or_make_stub(m, f, arity);
 	address = export_entry->address;
     }
@@ -1188,8 +1189,31 @@ static struct {
     unsigned int mask;		/* INV: mask == (1 << log2size)-1 */
     unsigned int used;
     struct hipe_mfa_info **bucket;
+    /*
+     * The mfa info table is normally updated by the loader,
+     * which runs in non-concurrent mode. Unfortunately runtime
+     * apply operations (get_na_nofail) update the table if
+     * they create a new stub for the mfa, which forces locking.
+     * XXX: Redesign apply et al to avoid those updates.
+     */
+    erts_smp_mtx_t lock;
 } hipe_mfa_info_table;
 
+static inline void hipe_mfa_info_table_init_lock(void)
+{
+    erts_smp_mtx_init(&hipe_mfa_info_table.lock, "hipe_mfait_lock");
+}
+
+static inline void hipe_mfa_info_table_lock(void)
+{
+    erts_smp_mtx_lock(&hipe_mfa_info_table.lock);
+}
+
+static inline void hipe_mfa_info_table_unlock(void)
+{
+    erts_smp_mtx_unlock(&hipe_mfa_info_table.lock);
+}
+
 #define HIPE_MFA_HASH(M,F,A)	((M) * (F) + (A))
 
 static struct hipe_mfa_info **hipe_mfa_info_table_alloc_bucket(unsigned int size)
@@ -1258,9 +1282,11 @@ void hipe_mfa_info_table_init(void)
     hipe_mfa_info_table.mask = size - 1;
     hipe_mfa_info_table.used = 0;
     hipe_mfa_info_table.bucket = hipe_mfa_info_table_alloc_bucket(size);
+
+    hipe_mfa_info_table_init_lock();
 }
 
-static inline struct hipe_mfa_info *hipe_mfa_info_table_get(Eterm m, Eterm f, unsigned int arity)
+static inline struct hipe_mfa_info *hipe_mfa_info_table_get_locked(Eterm m, Eterm f, unsigned int arity)
 {
     unsigned long h;
     unsigned int i;
@@ -1286,7 +1312,7 @@ void *hipe_mfa_find_na(Eterm m, Eterm f, unsigned int arity)
 }
 #endif
 
-static struct hipe_mfa_info *hipe_mfa_info_table_put(Eterm m, Eterm f, unsigned int arity)
+static struct hipe_mfa_info *hipe_mfa_info_table_put_locked(Eterm m, Eterm f, unsigned int arity)
 {
     unsigned long h;
     unsigned int i;
@@ -1313,7 +1339,10 @@ static struct hipe_mfa_info *hipe_mfa_info_table_put(Eterm m, Eterm f, unsigned
 
 static void hipe_mfa_set_na(Eterm m, Eterm f, unsigned int arity, void *address, int is_exported)
 {
-    struct hipe_mfa_info *p = hipe_mfa_info_table_put(m, f, arity);
+    struct hipe_mfa_info *p;
+
+    hipe_mfa_info_table_lock();
+    p = hipe_mfa_info_table_put_locked(m, f, arity);
 #ifdef DEBUG_LINKER
     printf("%s: ", __FUNCTION__);
     print_mfa(m, f, arity);
@@ -1322,19 +1351,30 @@ static void hipe_mfa_set_na(Eterm m, Eterm f, unsigned int arity, void *address,
     p->local_address = address;
     if (is_exported)
 	p->remote_address = address;
+    hipe_mfa_info_table_unlock();
 }
 
 #if defined(__powerpc__) || defined(__ppc__) || defined(__powerpc64__) || defined(__arm__)
 void *hipe_mfa_get_trampoline(Eterm m, Eterm f, unsigned int arity)
 {
-    struct hipe_mfa_info *p = hipe_mfa_info_table_put(m, f, arity);
-    return p->trampoline;
+    struct hipe_mfa_info *p;
+    void *trampoline;
+
+    hipe_mfa_info_table_lock();
+    p = hipe_mfa_info_table_put_locked(m, f, arity);
+    trampoline = p->trampoline;
+    hipe_mfa_info_table_unlock();
+    return trampoline;
 }
 
 void hipe_mfa_set_trampoline(Eterm m, Eterm f, unsigned int arity, void *trampoline)
 {
-    struct hipe_mfa_info *p = hipe_mfa_info_table_put(m, f, arity);
+    struct hipe_mfa_info *p;
+
+    hipe_mfa_info_table_lock();
+    p = hipe_mfa_info_table_put_locked(m, f, arity);
     p->trampoline = trampoline;
+    hipe_mfa_info_table_unlock();
 }
 #endif
 
@@ -1365,12 +1405,13 @@ BIF_RETTYPE hipe_bifs_invalidate_funinfo_native_addresses_1(BIF_ALIST_1)
     struct mfa mfa;
     struct hipe_mfa_info *p;
 
+    hipe_mfa_info_table_lock();
     lst = BIF_ARG_1;
     while (is_list(lst)) {
 	if (!term_to_mfa(CAR(list_val(lst)), &mfa))
-	    BIF_ERROR(BIF_P, BADARG);
+	    break;
 	lst = CDR(list_val(lst));
-	p = hipe_mfa_info_table_get(mfa.mod, mfa.fun, mfa.ari);
+	p = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);
 	if (p) {
 	    p->remote_address = NULL;
 	    p->local_address = NULL;
@@ -1393,6 +1434,7 @@ BIF_RETTYPE hipe_bifs_invalidate_funinfo_native_addresses_1(BIF_ALIST_1)
 	    }
 	}
     }
+    hipe_mfa_info_table_unlock();
     if (is_not_nil(lst))
 	BIF_ERROR(BIF_P, BADARG);
     BIF_RET(NIL);
@@ -1406,7 +1448,8 @@ void hipe_mfa_save_orig_beam_op(Eterm mod, Eterm fun, unsigned int ari, Eterm *p
     orig_beam_op = pc[0];
     if (orig_beam_op != BeamOpCode(op_hipe_trap_call_closure) &&
 	orig_beam_op != BeamOpCode(op_hipe_trap_call)) {
-	p = hipe_mfa_info_table_put(mod, fun, ari);
+	hipe_mfa_info_table_lock();
+	p = hipe_mfa_info_table_put_locked(mod, fun, ari);
 #ifdef DEBUG_LINKER
 	printf("%s: ", __FUNCTION__);
 	print_mfa(mod, fun, ari);
@@ -1414,6 +1457,7 @@ void hipe_mfa_save_orig_beam_op(Eterm mod, Eterm fun, unsigned int ari, Eterm *p
 #endif
 	p->beam_code = pc;
 	p->orig_beam_op = orig_beam_op;
+	hipe_mfa_info_table_unlock();
     } else {
 #ifdef DEBUG_LINKER
 	printf("%s: ", __FUNCTION__);
@@ -1440,12 +1484,12 @@ static void *hipe_make_stub(Eterm m, Eterm f, unsigned int arity, int is_remote)
     return StubAddress;
 }
 
-static void *hipe_get_na_nofail(Eterm m, Eterm f, unsigned int a, int is_remote)
+static void *hipe_get_na_nofail_locked(Eterm m, Eterm f, unsigned int a, int is_remote)
 {
     struct hipe_mfa_info *p;
     void *address;
 
-    p = hipe_mfa_info_table_get(m, f, a);
+    p = hipe_mfa_info_table_get_locked(m, f, a);
     if (p) {
 	/* find address, predicting for a runtime apply call */
 	address = p->remote_address;
@@ -1459,13 +1503,23 @@ static void *hipe_get_na_nofail(Eterm m, Eterm f, unsigned int a, int is_remote)
 	if (address)
 	    return address;
     } else
-	p = hipe_mfa_info_table_put(m, f, a);
+	p = hipe_mfa_info_table_put_locked(m, f, a);
     address = hipe_make_stub(m, f, a, is_remote);
     /* XXX: how to tell if a BEAM MFA is exported or not? */
     p->remote_address = address;
     return address;
 }
 
+static void *hipe_get_na_nofail(Eterm m, Eterm f, unsigned int a, int is_remote)
+{
+    void *p;
+
+    hipe_mfa_info_table_lock();
+    p = hipe_get_na_nofail_locked(m, f, a, is_remote);
+    hipe_mfa_info_table_unlock();
+    return p;
+}
+
 /* used for apply/3 in hipe_mode_switch */
 void *hipe_get_remote_na(Eterm m, Eterm f, unsigned int a)
 {
@@ -1549,6 +1603,8 @@ int hipe_find_mfa_from_ra(const void *ra, Eterm *m, Eterm *f, unsigned int *a)
 
     /* Note about locking: the table is only updated from the
        loader, which runs with the rest of the system suspended. */
+    /* XXX: alas not true; see comment at hipe_mfa_info_table.lock */
+    hipe_mfa_info_table_lock();
     bucket = hipe_mfa_info_table.bucket;
     nrbuckets = 1 << hipe_mfa_info_table.log2size;
     mfa = NULL;
@@ -1564,12 +1620,13 @@ int hipe_find_mfa_from_ra(const void *ra, Eterm *m, Eterm *f, unsigned int *a)
 	    b = b->bucket.next;
 	}
     }
-    if (!mfa)
-	return 0;
-    *m = mfa->m;
-    *f = mfa->f;
-    *a = mfa->a;
-    return 1;
+    if (mfa) {
+	*m = mfa->m;
+	*f = mfa->f;
+	*a = mfa->a;
+    }
+    hipe_mfa_info_table_unlock();
+    return mfa ? 1 : 0;
 }
 
 /*
@@ -1645,8 +1702,9 @@ BIF_RETTYPE hipe_bifs_add_ref_2(BIF_ALIST_2)
       default:
 	goto badarg;
     }
-    callee_mfa = hipe_mfa_info_table_put(callee.mod, callee.fun, callee.ari);
-    caller_mfa = hipe_mfa_info_table_put(caller.mod, caller.fun, caller.ari);
+    hipe_mfa_info_table_lock();
+    callee_mfa = hipe_mfa_info_table_put_locked(callee.mod, callee.fun, callee.ari);
+    caller_mfa = hipe_mfa_info_table_put_locked(caller.mod, caller.fun, caller.ari);
 
     refers_to = erts_alloc(ERTS_ALC_T_HIPE, sizeof(*refers_to));
     refers_to->mfa = callee_mfa;
@@ -1660,6 +1718,7 @@ BIF_RETTYPE hipe_bifs_add_ref_2(BIF_ALIST_2)
     ref->flags = flags;
     ref->next = callee_mfa->referred_from;
     callee_mfa->referred_from = ref;
+    hipe_mfa_info_table_unlock();
 
     BIF_RET(NIL);
 
@@ -1679,10 +1738,12 @@ BIF_RETTYPE hipe_bifs_mark_referred_from_1(BIF_ALIST_1) /* get_refs_from */
 
     if (!term_to_mfa(BIF_ARG_1, &mfa))
 	BIF_ERROR(BIF_P, BADARG);
-    p = hipe_mfa_info_table_get(mfa.mod, mfa.fun, mfa.ari);
+    hipe_mfa_info_table_lock();
+    p = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);
     if (p)
 	for (ref = p->referred_from; ref != NULL; ref = ref->next)
 	    ref->flags |= REF_FLAG_PENDING_REDIRECT;
+    hipe_mfa_info_table_unlock();
     BIF_RET(NIL);
 }
 
@@ -1695,7 +1756,8 @@ BIF_RETTYPE hipe_bifs_remove_refs_from_1(BIF_ALIST_1)
 
     if (!term_to_mfa(BIF_ARG_1, &mfa))
 	BIF_ERROR(BIF_P, BADARG);
-    caller_mfa = hipe_mfa_info_table_get(mfa.mod, mfa.fun, mfa.ari);
+    hipe_mfa_info_table_lock();
+    caller_mfa = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);
     if (caller_mfa) {
 	refers_to = caller_mfa->refers_to;
 	while (refers_to) {
@@ -1725,6 +1787,7 @@ BIF_RETTYPE hipe_bifs_remove_refs_from_1(BIF_ALIST_1)
 	}
 	caller_mfa->refers_to = NULL;
     }
+    hipe_mfa_info_table_unlock();
     BIF_RET(NIL);
 }
 
@@ -1742,14 +1805,15 @@ BIF_RETTYPE hipe_bifs_redirect_referred_from_1(BIF_ALIST_1)
 
     if (!term_to_mfa(BIF_ARG_1, &mfa))
 	BIF_ERROR(BIF_P, BADARG);
-    p = hipe_mfa_info_table_get(mfa.mod, mfa.fun, mfa.ari);
+    hipe_mfa_info_table_lock();
+    p = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);
     if (p) {
 	prev = &p->referred_from;
 	ref = *prev;
 	while (ref) {
 	    if (ref->flags & REF_FLAG_PENDING_REDIRECT) {
 		is_remote = ref->flags & REF_FLAG_IS_REMOTE;
-		new_address = hipe_get_na_nofail(p->m, p->f, p->a, is_remote);
+		new_address = hipe_get_na_nofail_locked(p->m, p->f, p->a, is_remote);
 		if (ref->flags & REF_FLAG_IS_LOAD_MFA)
 		    res = hipe_patch_insn(ref->address, (Uint)new_address, am_load_mfa);
 		else
@@ -1772,6 +1836,7 @@ BIF_RETTYPE hipe_bifs_redirect_referred_from_1(BIF_ALIST_1)
 	    }
 	}
     }
+    hipe_mfa_info_table_unlock();
     BIF_RET(NIL);
 }