From 97a65cd1e99739e0683700719fd33afa6865c2b2 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Sun, 25 Dec 2022 13:37:53 +0100 Subject: [PATCH] Store the debt list as pointers, not refs The idea is that the whole list (tail of the list) is synchronized by the access of the head. Nevertheless, we fill in the next pointer before that and having a reference is formally considered a read of the destination. Even though we _use_ the reference only after the synchronization point, we have it before and MIRI considers it a data race. Using a pointer is fine, as dereferencing is manual and considered read only at that point. --- CHANGELOG.md | 1 + src/debt/list.rs | 13 +++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed9669a..0dea87a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,4 @@ +* Fix a data race reported by MIRI. * Avoid violating stacked borrows (AFAIK these are still experimental and not normative, but better safe than sorry). (#80). * The `AccessConvert` wrapper is needed less often in practice (#77). diff --git a/src/debt/list.rs b/src/debt/list.rs index a98a02e..3d17388 100644 --- a/src/debt/list.rs +++ b/src/debt/list.rs @@ -58,7 +58,12 @@ pub(crate) struct Node { fast: FastSlots, helping: HelpingSlots, in_use: AtomicUsize, - next: Option<&'static Node>, + // Next node in the list. + // + // It is a pointer because we touch it before synchronization (we don't _dereference_ it before + // synchronization, only manipulate the pointer itself). That is illegal according to strict + // interpretation of the rules by MIRI on references. + next: *const Node, active_writers: AtomicUsize, } @@ -68,7 +73,7 @@ impl Default for Node { fast: FastSlots::default(), helping: HelpingSlots::default(), in_use: AtomicUsize::new(NODE_USED), - next: None, + next: ptr::null(), active_writers: AtomicUsize::new(0), } } @@ -94,7 +99,7 @@ impl Node { if result.is_some() { return result; } - current = node.next; + current = unsafe { node.next.as_ref() }; } None } @@ -165,7 +170,7 @@ impl Node { // compare_exchange below. let mut head = LIST_HEAD.load(Relaxed); loop { - node.next = unsafe { head.as_ref() }; + node.next = head; if let Err(old) = LIST_HEAD.compare_exchange_weak( head, node, // We need to release *the whole chain* here. For that, we need to