impl Debug for Atomic types #27129

arthurprs · 2015-07-19T16:28:34Z

I'm being constantly bitten by the lack of this implementation.

I'm unsure if there's a reason to avoid these implementations though.

Since we have a "lossy" implementation for both Mutex and RWLock (RWLock {{ locked }}) I don't think there's a big reason for not having a Debug implementation for the atomic types, even if the user can't specify the ordering.

rust-highfive · 2015-07-19T16:28:51Z

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

emberian · 2015-07-19T16:37:48Z

src/libcore/atomic.rs

+
+macro_rules! impl_Debug {
+    ($($t:ident)*) => ($(
+        impl fmt::Debug for $t {


I believe these would need a stability attribute (#[stable(feature = "atomic_debug", since = "1.3.0")])

Trait impls are insta-stable, so it seems kinda useless...?

I feel like I've had this conversation a ton of times and keep forgetting why it might be useful.

Other impls are still marked, though. Even if it's insta-stable having the version information of when an impl appeared can be useful for understanding the version requirements of your code.

Does it though? Either you have a compiler that can build it, or you don't. I suppose if you have one that can build it, you can query if you can use an older one...?

So in order to determine the oldest Rust version I support, I should do I binary search on all Rust releases to determine which can build my project?

emberian · 2015-07-19T16:39:42Z

Seems fine to me, in terms of the impl existing.

emberian · 2015-07-19T16:40:29Z

src/libcore/atomic.rs

+    ($($t:ident)*) => ($(
+        impl fmt::Debug for $t {
+            fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+                write!(f, "Atomic {{ {:?} }}", self.load(Ordering::Relaxed))


I believe this should be write!(f, concat!(stringify!($t), "{{ {:?} }}", ...) so as to actually print which type this is.

These implementations should match the relevant RFC. I'm not sure if these should be treated like smart pointers, in which case this would just be self.load(Ordering::Relaxed).fmt(fmt), or a struct, in which case something like this should suffice:

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_tuple(stringify!($t)).field(&self.load(Ordering::Relaxed)).finish() }

arthurprs · 2015-07-19T17:35:16Z

Updated the commit. I tried to address all points raised in the comments.

emberian · 2015-07-19T17:52:03Z

src/libcore/atomic.rs

+macro_rules! impl_Debug {
+    ($($t:ident)*) => ($(
+        impl fmt::Debug for $t {
+            #[stable(feature = "atomic_debug", since = "1.3.0")]


These attributes go on the impl, not the method.

Oh, of course. Amending it right away

arielb1 · 2015-07-19T18:17:48Z

Why Relaxed rather than SeqCst? Seems like SeqCst should always be preferred unless there is a performance reason.

nagisa · 2015-07-19T18:31:22Z

src/libcore/atomic.rs

+#[stable(feature = "atomic_debug", since = "1.3.0")]
+impl<T: fmt::Debug> fmt::Debug for AtomicPtr<T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.debug_tuple(stringify!($t)).field(&self.load(Ordering::Relaxed)).finish()


This looks wrong. Especially the stringify!($t) part.

This is not inside a macro, therefore at least the $t part is invalid.

Oh right, I just realized this is the real impl :)

duh, I gotta remind myself of these failures when I try to fast amend things

emberian · 2015-07-19T18:36:33Z

@arielb1 on hardware with a weak memory model, SeqCst would force synchronization between cores running this code and cores modifying the data, which is unnecessary since this code doesn't have synchronization requirements. It'd still be racy even with SeqCst (since by the time the value is formatted it could have changed)

alexcrichton · 2015-07-20T04:33:09Z

src/libcore/atomic.rs

+impl_Debug!{ AtomicUsize AtomicIsize AtomicBool }
+
+#[stable(feature = "atomic_debug", since = "1.3.0")]
+impl<T: fmt::Debug> fmt::Debug for AtomicPtr<T> {


I believe this doesn't need the bound on fmt::Debug as that impl isn't used

Good catch. Fixed.

arthurprs · 2015-07-20T12:27:20Z

We still have to figure out the default Ordering for it. I'm with @cmr on this one since even with strong ordering semantics there's no guarantee whatsoever that the underlining value didn't change after the load.

arielb1 · 2015-07-20T12:39:00Z

@cmr

Debug isn't supposed to be a performance bottleneck. Using a weak memory model (esp. Relaxed) can only lead to trouble. Even on weak-memory-model processors (at least the ones I know of - ARM), SeqCst does not affect the performance of the other side.

arthurprs · 2015-07-20T13:05:01Z

@arielb1 what kind of trouble?

arielb1 · 2015-07-20T14:31:10Z

@arthurprs

Unpredictable results. If you do have some kind of synchronization, Relaxed-stores can still return a random-ish value.

arthurprs · 2015-07-20T15:50:32Z

@arielb1
It's not random, relaxed loads are still atomic, so in the worst possible case it'll return the valid value as seen by that core (possibly a few nanoseconds away from the newest value).

alexcrichton · 2015-07-20T16:20:57Z

@arthurprs let's switch it to SeqCst. If thorough research is done at a later date to determine that it's not necessary, it's backwards compatible to switch. All atomic operations in the standard library are as conservative as possible until proven otherwise, so these need to be SeqCst.

arthurprs · 2015-07-20T17:12:39Z

Commit updated to use SeqCst.

alexcrichton · 2015-07-20T17:34:01Z

@bors: r+ 225ad17

Thanks!

I'm being constantly bitten by the lack of this implementation. I'm unsure if there's a reason to avoid these implementations though. Since we have a "lossy" implementation for both Mutex and RWLock (RWLock {{ locked }}) I don't think there's a big reason for not having a Debug implementation for the atomic types, even if the user can't specify the ordering.

bors · 2015-07-21T00:03:39Z

⌛ Testing commit 225ad17 with merge 118a5c4...

bors · 2015-07-21T01:38:10Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

rust-highfive assigned alexcrichton Jul 19, 2015

emberian reviewed Jul 19, 2015
View reviewed changes

arthurprs force-pushed the debug_atomic branch from 59492d0 to 2bc970e Compare July 19, 2015 17:33

emberian reviewed Jul 19, 2015
View reviewed changes

arthurprs force-pushed the debug_atomic branch from 2bc970e to 41236cc Compare July 19, 2015 17:54

nagisa reviewed Jul 19, 2015
View reviewed changes

arthurprs force-pushed the debug_atomic branch from 41236cc to afa2844 Compare July 19, 2015 18:52

alexcrichton reviewed Jul 20, 2015
View reviewed changes

arthurprs force-pushed the debug_atomic branch from afa2844 to 5552ef0 Compare July 20, 2015 12:22

impl Debug for Atomic types

225ad17

arthurprs force-pushed the debug_atomic branch from 5552ef0 to 225ad17 Compare July 20, 2015 17:11

bors merged commit 225ad17 into rust-lang:master Jul 21, 2015

impl Debug for Atomic types #27129

impl Debug for Atomic types #27129

Uh oh!

Conversation

arthurprs commented Jul 19, 2015

Uh oh!

rust-highfive commented Jul 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emberian commented Jul 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurprs commented Jul 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Jul 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emberian commented Jul 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurprs commented Jul 20, 2015

Uh oh!

arielb1 commented Jul 20, 2015

Uh oh!

arthurprs commented Jul 20, 2015

Uh oh!

arielb1 commented Jul 20, 2015

Uh oh!

arthurprs commented Jul 20, 2015

Uh oh!

alexcrichton commented Jul 20, 2015

Uh oh!

arthurprs commented Jul 20, 2015

Uh oh!

alexcrichton commented Jul 20, 2015

Uh oh!

bors commented Jul 21, 2015

Uh oh!

bors commented Jul 21, 2015

Uh oh!

Uh oh!