Potential Unsound Issue of Arc

[CXWorks]

Hi, thanks for your time to read the issue. While reporting issues for another crate(Nugine/wgp#1), our statc analyzer find a potential unsound issue:

The issue is in line 654, where a load acq is used as a fence:

triomphe/src/arc.rs

Lines 627 to 659 in 6ff0527

	fn drop_inner(&mut self) {
	// Because `fetch_sub` is already atomic, we do not need to synchronize
	// with other threads unless we are going to delete the object.
	if self.inner().count.fetch_sub(1, Release) != 1 {
	return;
	}
	// FIXME(bholley): Use the updated comment when [2] is merged.
	//
	// This load is needed to prevent reordering of use of the data and
	// deletion of the data. Because it is marked `Release`, the decreasing
	// of the reference count synchronizes with this `Acquire` load. This
	// means that use of the data happens before decreasing the reference
	// count, which happens before this load, which happens before the
	// deletion of the data.
	//
	// As explained in the [Boost documentation][1],
	//
	// > It is important to enforce any possible access to the object in one
	// > thread (through an existing reference) to *happen before* deleting
	// > the object in a different thread. This is achieved by a "release"
	// > operation after dropping a reference (any access to the object
	// > through this reference must obviously happened before), and an
	// > "acquire" operation before deleting the object.
	//
	// [1]: (www.boost.org/doc/libs/1_55_0/doc/html/atomic/usage_examples.html)
	// [2]: https://github.com/rust-lang/rust/pull/41714
	self.inner().count.load(Acquire);
	unsafe {
	self.drop_slow();
	}
	}

The std library's impl uses atomic::fence(Acquire) as the fence because the the compiler may optimize out this unused load(although it's unlikely to happen in practice). You can check following discussions for details:

1. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4455.html
2. Use a load rather than a fence when dropping the contents of an Arc. rust-lang/rust#41714

In a nutshell, load acq is 99.99% correct but a fence is 100%. Meanwhile, you can check the second discussion for the potential performence influence of this operation: rust-lang/rust#41714 (comment)

Thanks again for your help.

[Manishearth]

Yeah, we were matching the old behavior of Rust from rust-lang/rust#41714

I'd support switching back to a fence here. Don't have time to make a PR right now though.

[CXWorks]

Thanks for your quick response. Do you need my help? Like to draft a PR?

[Manishearth]

Yes, a PR would be helpful

[CXWorks]

Please check PR #114