[Git][ghc/ghc][wip/tsan/fixes] 3 commits: Things
Ben Gamari (@bgamari)
gitlab at gitlab.haskell.org
Fri Apr 14 09:58:42 UTC 2023
Ben Gamari pushed to branch wip/tsan/fixes at Glasgow Haskell Compiler / GHC
Commits:
c632f481 by Ben Gamari at 2023-04-13T18:50:32-04:00
Things
- - - - -
1bf60a63 by Ben Gamari at 2023-04-14T05:57:48-04:00
rts/IPE: Fix unused mutex warning
- - - - -
a4106472 by Ben Gamari at 2023-04-14T05:58:23-04:00
rts: Relax info pointer stores
- - - - -
8 changed files:
- compiler/GHC/Cmm/Info.hs
- rts/Apply.cmm
- rts/IPE.c
- rts/ThreadPaused.c
- rts/Updates.h
- rts/include/Cmm.h
- rts/include/rts/storage/ClosureMacros.h
- rts/include/stg/SMP.h
Changes:
=====================================
compiler/GHC/Cmm/Info.hs
=====================================
@@ -450,7 +450,7 @@ wordAligned platform align_check e
-- | Takes a closure pointer and returns the info table pointer
closureInfoPtr :: Platform -> DoAlignSanitisation -> CmmExpr -> CmmExpr
closureInfoPtr platform align_check e =
- cmmLoadBWord platform (wordAligned platform align_check e)
+ CmmMachOp (MO_RelaxedRead (wordWidth platform)) [wordAligned platform align_check e]
-- | Takes an info pointer (the first word of a closure) and returns its entry
-- code
=====================================
rts/Apply.cmm
=====================================
@@ -694,7 +694,7 @@ INFO_TABLE(stg_AP_STACK,/*special layout*/0,0,AP_STACK,"AP_STACK","AP_STACK")
// Can't add StgInd_indirectee(ap) to UpdRemSet here because the old value is
// not reachable.
%release StgInd_indirectee(ap) = CurrentTSO;
- SET_INFO_RELEASE(ap, __stg_EAGER_BLACKHOLE_info);
+ SET_INFO_RELAXED(ap, __stg_EAGER_BLACKHOLE_info);
/* ensure there is at least AP_STACK_SPLIM words of headroom available
* after unpacking the AP_STACK. See bug #1466 */
=====================================
rts/IPE.c
=====================================
@@ -57,7 +57,6 @@ this all IPE lists of all IpeBufferListNode are traversed to insert all IPEs.
After the content of a IpeBufferListNode has been inserted, it's freed.
*/
-static Mutex ipeMapLock;
static HashTable *ipeMap = NULL;
// Accessed atomically
@@ -65,6 +64,8 @@ static IpeBufferListNode *ipeBufferList = NULL;
#if defined(THREADED_RTS)
+static Mutex ipeMapLock;
+
void initIpe(void) { initMutex(&ipeMapLock); }
void exitIpe(void) { closeMutex(&ipeMapLock); }
=====================================
rts/ThreadPaused.c
=====================================
@@ -353,7 +353,7 @@ threadPaused(Capability *cap, StgTSO *tso)
// The payload of the BLACKHOLE points to the TSO
RELEASE_STORE(&((StgInd *)bh)->indirectee, (StgClosure *)tso);
- SET_INFO_RELEASE(bh,&stg_BLACKHOLE_info);
+ SET_INFO_RELAXED(bh,&stg_BLACKHOLE_info);
// .. and we need a write barrier, since we just mutated the closure:
recordClosureMutated(cap,bh);
=====================================
rts/Updates.h
=====================================
@@ -86,7 +86,7 @@ INLINE_HEADER void updateWithIndirection (Capability *cap,
}
OVERWRITING_CLOSURE(p1);
RELEASE_STORE(&((StgInd *)p1)->indirectee, p2);
- SET_INFO_RELEASE(p1, &stg_BLACKHOLE_info);
+ SET_INFO_RELAXED(p1, &stg_BLACKHOLE_info);
LDV_RECORD_CREATE(p1);
}
=====================================
rts/include/Cmm.h
=====================================
@@ -596,6 +596,7 @@
/* Getting/setting the info pointer of a closure */
#define SET_INFO(p,info) StgHeader_info(p) = info
#define SET_INFO_RELEASE(p,info) %release StgHeader_info(p) = info
+#define SET_INFO_RELAXED(p,info) %relaxed StgHeader_info(p) = info
#define GET_INFO(p) StgHeader_info(p)
#define GET_INFO_ACQUIRE(p) %acquire GET_INFO(p)
=====================================
rts/include/rts/storage/ClosureMacros.h
=====================================
@@ -47,6 +47,11 @@
EXTERN_INLINE void SET_INFO(StgClosure *c, const StgInfoTable *info);
EXTERN_INLINE void SET_INFO(StgClosure *c, const StgInfoTable *info) {
+ c->header.info = info;
+}
+
+EXTERN_INLINE void SET_INFO_RELAXED(StgClosure *c, const StgInfoTable *info);
+EXTERN_INLINE void SET_INFO_RELAXED(StgClosure *c, const StgInfoTable *info) {
RELAXED_STORE(&c->header.info, info);
}
=====================================
rts/include/stg/SMP.h
=====================================
@@ -124,6 +124,47 @@ EXTERN_INLINE void store_load_barrier(void);
EXTERN_INLINE void load_load_barrier(void);
/*
+ * Note [C11 memory model]
+ * ~~~~~~~~~~~~~~~~~~~~~~~
+ * When it comes to memory, real multiprocessors provide a wide range of
+ * concurrency semantics due to out-of-order execution and caching.
+ * To provide consistent reasoning across architectures, GHC relies the C11
+ * memory model. Not only does this provide a well-studied, fairly
+ * easy-to-understand conceptual model, but the C11 memory model gives us
+ * access to a number of tools which help us verify the compiler (see Note
+ * [ThreadSanitizer] in rts/include/rts/TSANUtils.h).
+ *
+ * Under the C11 model, each processor can be imagined to have a potentially
+ * out-of-date view onto the system's memory, which can be manipulated with two
+ * classes of memory operations:
+ *
+ * - non-atomic operations (e.g. loads and stores) operate strictly on the
+ * processor's local view of memory and consequently may not be visible
+ * from other processors.
+ *
+ * - atomic operations (e.g. load, store, fetch-and-{add,subtract,and,or},
+ * exchange, and compare-and-swap) parametrized by ordering semantics.
+ *
+ * The ordering semantics of an operation (acquire, release, or sequentially
+ * consistent) will determine the amount of synchronization the operation
+ * requires.
+ *
+ * A processor may synchronize its
+ * view of memory with that of another processor by performing an atomic
+ * memory operation.
+ *
+ * While non-atomic operations can be thought of as operating on a local
+ *
+ * See also:
+ *
+ * - The C11 standard, ISO/IEC 14882 2011.
+ *
+ * - Boehm, Adve. "Foundations of the C++ Concurrency Memory Model."
+ * PLDI '08.
+ *
+ * - Batty, Owens, Sarkar, Sewall, Weber. "Mathematizing C++ Concurrency."
+ * POPL '11.
+ *
* Note [Heap memory barriers]
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~
* Machines with weak memory ordering semantics have consequences for how
@@ -132,23 +173,32 @@ EXTERN_INLINE void load_load_barrier(void);
* stores which formed the new object are visible (e.g. stores are flushed from
* cache and the relevant cachelines invalidated in other cores).
*
- * To ensure this we must use memory barriers. Which barriers are required to
- * access a field depends upon the identity of the field. In general, fields come
- * in three flavours:
+ * To ensure this we must issue memory barriers when accessing closures and
+ * their fields. Since reasoning about concurrent memory access with barriers tends to be
+ * subtle and platform dependent, it is more common to instead write programs
+ * in terms of an abstract memory model and let the compiler (GHC and the
+ * system's C compiler) worry about what barriers are needed to realize the
+ * requested semantics on the target system. GHC relies on the widely used C11
+ * memory model for this; see Note [C11 memory model] for a brief introduction.
+ *
+ * Also note that the majority of this Note are only concerned with mutation
+ * by the mutator. The GC is free to change nearly any field (which is
+ * necessary for a moving GC). Naturally, doing this safely requires care which
+ * we discuss in the "Barriers during GC" section below.
+ *
+ * Field access
+ * ------------
+ * Which barriers are required to access a field of a closure depends upon the
+ * identity of the field. In general, fields come in three flavours:
*
- * * Mutable GC Pointers (C type StgClosure*, Cmm type StgPtr)
- * * Immutable GC Pointers (C type MUT_FIELD StgClosure*, Cmm type StgPtr)
- * * Non-pointers (C type StgWord, Cmm type StdWord)
+ * * Mutable GC Pointers (C type `StgClosure*`, Cmm type `StgPtr`)
+ * * Immutable GC Pointers (C type `MUT_FIELD StgClosure*`, Cmm type `StgPtr`)
+ * * Non-pointers (C type `StgWord`, Cmm type `StgWord`)
*
* Note that Addr# fields are *not* GC pointers and therefore are classified
* as non-pointers. In this case responsibility for barriers lies with the
* party dereferencing the Addr#.
*
- * Also note that we are only concerned with mutation by the mutator. The GC
- * is free to change nearly any field as this is necessary for a moving GC.
- * Naturally, doing this safely requires care which we discuss in section
- * below.
- *
* Immutable pointer fields are those which the mutator cannot change after
* an object is made visible on the heap. Most objects' fields are of this
* flavour (e.g. all data constructor fields). As these fields are written
@@ -156,7 +206,7 @@ EXTERN_INLINE void load_load_barrier(void);
* safe due to an argument hinging on causality: Consider an immutable field F
* of an object O which refers to object O'. Naturally, O' must have been
* visible to the creator of O when O was constructed. Consequently, if O is
- * visible to a reader, O' must also be visible to the same reader..
+ * visible to a reader, O' must also be visible to the same reader.
*
* Mutable pointer fields are those which can be modified by the mutator. These
* require a bit more care as they may break the causality argument given
@@ -165,6 +215,10 @@ EXTERN_INLINE void load_load_barrier(void);
* into F. Without explicit synchronization O' may not be visible to another
* thread attempting to dereference F.
*
+ * To ensure the visibility of the referent, writing to a mutable pointer field
+ * must be done via a release-store. Conversely, reading from such a field is
+ * done via an acquire-load.
+ *
* Mutable fields include:
*
* - StgMutVar: var
@@ -180,9 +234,6 @@ EXTERN_INLINE void load_load_barrier(void);
* - StgTSO: block_info
* - StgInd: indirectee
*
- * Writing to a mutable pointer field must be done via a release-store.
- * Reading from such a field is done via an acquire-load.
- *
* Finally, non-pointer fields can be safely mutated without barriers as
* they do not refer to other memory locations. Technically, concurrent
* accesses to non-pointer fields still do need to be atomic in many cases to
@@ -217,7 +268,7 @@ EXTERN_INLINE void load_load_barrier(void);
* As noted above, thunks are a rather special (yet quite common) case. In
* particular, they have the unique property of being updatable (that is, can
* be transformed from a thunk into an indirection after evaluation). This
- * transformation requires its own synchronization protocol mediating the
+ * transformation requires its own synchronization protocol to mediate the
* interaction between the updater and the reader. In particular, we
* must ensure that a reader examining a thunk being updated by another core
* can see the indirectee. Consequently, a thunk update (see rts/Updates.h)
@@ -357,6 +408,11 @@ EXTERN_INLINE void load_load_barrier(void);
* The work-stealing queue (WSDeque) also requires barriers; these are
* documented in WSDeque.c.
*
+ * Verifying memory ordering
+ * -------------------------
+ * To verify that GHC's RTS and the code produced by the compiler are free of
+ * data races.
+ *
*/
/* ----------------------------------------------------------------------------
View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/589a4dc3c0caac8ffeb6dd260e6e5defacf5e224...a4106472154c2f815129acb6149eab933bdc390c
--
View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/589a4dc3c0caac8ffeb6dd260e6e5defacf5e224...a4106472154c2f815129acb6149eab933bdc390c
You're receiving this email because of your account on gitlab.haskell.org.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-commits/attachments/20230414/c18c7215/attachment-0001.html>
More information about the ghc-commits
mailing list