Make RwLockReadGuard covariant

[r-raymond]

Hi, first time contributor here, if anything is not as expected, please let me know.

RwLockReadGoard's type constructor is invariant. Since it behaves like a smart pointer to an immutable reference, there is no reason that it should not be covariant. Take e.g.

fn test_read_guard_covariance() {
    fn do_stuff<'a>(_: RwLockReadGuard<'_, &'a i32>, _: &'a i32) {}
    let j: i32 = 5;
    let lock = RwLock::new(&j);
    {
        let i = 6;
        do_stuff(lock.read().unwrap(), &i);
    }
    drop(lock);
}

where the compiler complains that &i doesn't live long enough. If RwLockReadGuard is covariant, then the above code is accepted because the lifetime can be shorter than 'a.

In order for RwLockReadGuard to be covariant, it can't contain a full reference to the RwLock, which can never be covariant (because it exposes a mutable reference to the underlying data structure). By reducing the data structure to the required pieces of RwLock, the rest falls in place.

If there is a better way to do a test that tests successful compilation, please let me know.

Fixes #80392

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[m-ou-se]

@rfcbot merge

[rfcbot]

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[RustyYato]

I don't think this change is sound because of problems like this:

• Stacked Borrows: Retag (private) fields of ADTs? unsafe-code-guidelines#125
• Ref parameter incorrectly decorated with noalias attribute #63787

tldr: with this change Ref gets the noalias attribute (from the reference), which is unsound.

In fact, if we check the IR we see a noalias attribute with the new change, but not the existing code.

https://rust.godbolt.org/z/jbe6Yah6j

struct RefCell<T: ?Sized> {
    borrow_flag: core::cell::Cell<usize>,
    cell: core::cell::UnsafeCell<T>,
}

struct RefStd<'a, T> {
    cell: &'a RefCell<T>,
}

struct Ref<'a, T> {
    cell: &'a core::cell::Cell<usize>,
    value: &'a T,
}

// has noalias
#[no_mangle] fn foo(rc: &RefCell<i32>, r: Ref<'_, i32>) {}
// does not have noalias
#[no_mangle] fn bar(rc: &RefCell<i32>, r: RefStd<'_, i32>) {}

generated llvm-ir

; ModuleID = 'example.6cc7c99e-cgu.0'
source_filename = "example.6cc7c99e-cgu.0"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone willreturn
define void @foo({ i64, i32 }* nocapture noundef align 8 dereferenceable(16) %rc, i64* nocapture noundef align 8 dereferenceable(8) %r.0, i32* noalias nocapture noundef readonly align 4 dereferenceable(4) %r.1) unnamed_addr #0 {
start:
  ret void
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone willreturn
define void @bar({ i64, i32 }* nocapture noundef align 8 dereferenceable(16) %rc, i64* nocapture noundef align 8 dereferenceable(16) %r) unnamed_addr #0 {
start:
  ret void
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind nonlazybind readnone willreturn "probe-stack"="__rust_probestack" "target-cpu"="x86-64" }

!llvm.module.flags = !{!0, !1}

!0 = !{i32 7, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}

[coolreader18]

Separately from the soundness concern; should the data: &T field be first? It's a lot more common to access the data than the mutex, and so then it could compile to the same as a pointer access without having to add an offset. (Maybe?)

[r-raymond]

Hi @RustyYato, thanks for speaking up, I was not aware of the noalias problem. Seems like the designated "fix" (replacing by * const) also works in this case.

struct RefCell<T: ?Sized> {
    borrow_flag: core::cell::Cell<usize>,
    cell: core::cell::UnsafeCell<T>,
}

struct RefStd<'a, T> {
    cell: &'a RefCell<T>,
}

struct Ref<'a, T> {
    cell: &'a core::cell::Cell<usize>,
    value: &'a T,
}

struct RefNew<'a, T> {
    value: *const T,
    cell: &'a core::cell::Cell<usize>,
}

// has noalias
#[no_mangle] fn foo(rc: &RefCell<i32>, r: Ref<'_, i32>) {}
// does not have noalias
#[no_mangle] fn bar(rc: &RefCell<i32>, r: RefStd<'_, i32>) {}
// does not have noalias
#[no_mangle] fn baz(rc: &RefCell<i32>, r: RefNew<'_, i32>) {}

; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone willreturn
define void @foo({ i64, i32 }* nocapture noundef align 8 dereferenceable(16) %rc, i64* nocapture noundef align 8 dereferenceable(8) %r.0, i32* noalias nocapture noundef readonly align 4 dereferenceable(4) %r.1) unnamed_addr #0 {
start:
  ret void
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone willreturn
define void @bar({ i64, i32 }* nocapture noundef align 8 dereferenceable(16) %rc, i64* nocapture noundef align 8 dereferenceable(16) %r) unnamed_addr #0 {
start:
  ret void
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind readnone willreturn
define void @baz({ i64, i32 }* nocapture noundef align 8 dereferenceable(16) %rc, i32* nocapture %r.0, i64* nocapture noundef align 8 dereferenceable(8) %r.1) unnamed_addr #0 {
start:
  ret void
}

(see https://rust.godbolt.org/z/GrKP9oT99).

Luckily * const preserves our covariancy, so this still solved the initial problem. Any objections to making this modification by the existing reviewers?

[r-raymond]

Thanks for the suggestion @coolreader18. Seems like there shouldn't be a difference for most architectures:

use std::ops::Deref;

struct Test1<'a, T: ?Sized> {
    cell: &'a core::cell::Cell<usize>,
    data: *const T,
}
struct Test2<'a, T: ?Sized> {
    data: *const T,
    cell: &'a core::cell::Cell<usize>,
}


impl<T: ?Sized> Deref for Test1<'_, T> {
    type Target = T;

    fn deref(&self) -> &T {
        unsafe { &*self.data }
    }
}

impl<T: ?Sized> Deref for Test2<'_, T> {
    type Target = T;

    fn deref(&self) -> &T {
        unsafe { &*self.data }
    }
}

#[no_mangle] fn t1(t: &Test1<i32>) -> i32 {
    **t
}

#[no_mangle] fn t2(t: &Test2<i32>) -> i32 {
    **t
}

gives

        .text
        .intel_syntax noprefix
        .file   "example.6cc7c99e-cgu.0"
        .section        .text.t1,"ax",@progbits
        .globl  t1
        .p2align        4, 0x90
        .type   t1,@function
t1:
        mov     rax, qword ptr [rdi + 8]
        mov     eax, dword ptr [rax]
        ret
.Lfunc_end0:
        .size   t1, .Lfunc_end0-t1

        .section        .text.t2,"ax",@progbits
        .globl  t2
        .p2align        4, 0x90
        .type   t2,@function
t2:
        mov     rax, qword ptr [rdi]
        mov     eax, dword ptr [rax]
        ret
.Lfunc_end1:
        .size   t2, .Lfunc_end1-t2

        .section        ".note.GNU-stack","",@progbits

(see https://rust.godbolt.org/z/neh1KW6eY)
AFAICT the mov with offset is not more expensive than the mov without offset. However since there is no drawback to your suggestion, I'll go ahead and make the change.

[BurntSushi]

I would feel a lot better if the unsafe usages in this PR had safety justifications. And also that the unsafe fn new routine documents what conditions the caller is required to uphold for safety.

[RalfJung]

Luckily * const preserves our covariancy, so this still solved the initial problem. Any objections to making this modification by the existing reviewers?

You probably want to use NonNull to preserve the niche. But other than that, seems fine to me.

[r-raymond]

@RalfJung thanks for the pointer. Seems like NonNull is potentially a bigger footgun, but it makes sense to keep the niche for optimizations. Modified in 1ab5f09

@BurntSushi thanks for the concerns. I have added some documentation to the unsafe fn new's of ReadGuard and WriteGuard. Let me know if you consider this sufficient. I kept it brief considering that there should really be no reason to instantiate these Guards manually, so it is more of an internal documentation.

Wrt the arguments for safety, here is why they are safe.

@@ -536,7 +549,7 @@ fn deref_mut(&mut self) -> &mut T {
 impl<T: ?Sized> Drop for RwLockReadGuard<'_, T> {
     fn drop(&mut self) {
         unsafe {
-            self.lock.inner.read_unlock();
+            self.inner_lock.read_unlock();
         }
     }
 }

This really doesn't change the behavior from before. However, if the precondition documented on the new function holds, then the current thread has read access, in which case it is safe to call read_unlock according to the documentation of MoveableRwLock.

@@ -512,7 +525,7 @@ impl<T: ?Sized> Deref for RwLockReadGuard<'_, T> {
    type Target = T;

    fn deref(&self) -> &T {
-        unsafe { &*self.lock.data.get() }
+        unsafe { self.data.as_ref() }
    }
}

According to the documentation of NonNull https://doc.rust-lang.org/std/ptr/struct.NonNull.html this is safe if

• the pointer is properly aligned
• the pointer is dereferenceable
• the pointer points to an initialized instance of T
• Rust's aliasing rules are respected (wrt the lifetime returned)

Making arguments for these points basically takes us through the entire construction of the RwLock. If we accept that &*self.lock.data.get() is safe, then we know than the first 3 points are given. The last point is given since the lifetime is restricted by the lifetime of the RwLockReadGuard. As long as the instance of the latter is alive, we have (shared) read access to the underlying data structure.

Finally

@@ -468,12 +470,23 @@ fn from(t: T) -> Self {
 }

 impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> {
+    /// Create a new instance of `RwLockReadGuard<T>` from a `RwLock<T>`.
+    ///
+    /// It is safe to call this function if and only if `lock.inner.read()` (or
+    /// `lock.inner.try_read()`) has been successfully called before instantiating this object.
     unsafe fn new(lock: &'rwlock RwLock<T>) -> LockResult<RwLockReadGuard<'rwlock, T>> {
-        poison::map_result(lock.poison.borrow(), |_| RwLockReadGuard { lock })
+        poison::map_result(lock.poison.borrow(), |_| RwLockReadGuard {
+            data: NonNull::new_unchecked(lock.data.get()),
+            inner_lock: &lock.inner,
+        })
     }
 }

The concerning point is NonNull::new_unchecked. Since an initialized RwLock will always hold a valid instance of the underlying object, lock.data.get() can't be null.

Let me know if these arguments are sufficient for you. Happy to dive deeper into anything mentioned here. If anyone else has comments on the above, please share.

[BurntSushi]

@r-raymond Thanks! I generally prefer to see SAFETY: comments on all uses of unsafe, but I'll leave that up to @rust-lang/libs, as I'm not terribly familiar with the convention here in std. But your reasoning in the comment makes sense to me. :)

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[r-raymond]

@BurntSushi: Thanks for the response. I was not aware of the norm around SAFETY comments. Can you share an example (rust and safety is a bit hard to search)? Should we abort the merge and address the issue?

[RalfJung]

If you grep for 'SAFETY:' in the library folder of this repo, you will find hundreds of examples. :)

[cuviper]

@bors r+

[bors]

📌 Commit 048a801 has been approved by cuviper

[JohnTitor]

Seems failed in rollup: #98367 (comment)
@bors r-

[r-raymond]

Seems failed in rollup: #98367 (comment) @bors r-

I have taken a stab at fixing this in 5656de7. However, I don't have access to a windows box. Is there any way I can verify this without one?

[cuviper]

I don't know how to test it without Windows, but let's just try it without a rollup, so we don't break others' CI.

@bors r+ rollup=never

[bors]

📌 Commit 5656de7 has been approved by cuviper

[bors]

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[bors]

⌛ Testing commit 5656de7 with merge 00ce472...

[bors]

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 00ce472 to master...

[rust-timer]

Finished benchmarking commit (00ce472): comparison url.

Instruction count

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 🎉 relevant improvement found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.7%	-0.7%	1
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: 😿 relevant regressions found

	mean1	max	count2
Regressions 😿 (primary)	2.6%	3.0%	2
Regressions 😿 (secondary)	6.4%	9.1%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.6%	3.0%	2

Cycles

Results

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	4.2%	4.2%	1
Improvements 🎉 (primary)	-2.9%	-2.9%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.9%	-2.9%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³