Closed Bug 965837 Opened 10 years ago Closed 10 years ago

mozilla::Atomic should support bool

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: vikstrous, Assigned: vikstrous)

References

Details

Attachments

(1 file, 9 obsolete files)

Our current implementation of atomics doesn't support bool because it treats it as an integral type and then complains that it's not 32 or 64 bits. We should have atomic bools.
Attached patch atomic_bool (obsolete) — Splinter Review
Attachment #8367990 - Flags: review?(nfroyd)
Assignee: nobody → vstanchev
Comment on attachment 8367990 [details] [diff] [review]
atomic_bool

Review of attachment 8367990 [details] [diff] [review]:
-----------------------------------------------------------------

This is headed in the right direction, but requires some polish.

::: mfbt/Atomics.h
@@ +1008,5 @@
>  
> +/**
> + * Atomic<T> implementation for boolean types.
> + *
> + * The atomic store and load operations and the atomic swap method is provided.

You should have a brief blurb here about how sizeof(Atomic<bool>) != sizeof(bool) for some implementations of bool and/or some implementations of std::atomic, how this is driven by the constraints we have for atomic operations on windows, and how this behavior is explicitly allowed by the standard ([atomic.types.generic]p9).  I can help with the finer points of the wording if you write the first draft.

@@ +1020,5 @@
> +  public:
> +    MOZ_CONSTEXPR Atomic() : Base() {}
> +    MOZ_CONSTEXPR Atomic(bool aInit) : Base(aInit) {}
> +
> +    using Base::operator=;

This is not quite right (nor are the other operations exposed by AtomicBase quite right) because those operations are on uint32_t.  So we have an Atomic<bool> that allows assigning, say, UINT32_MAX to it, and getting that value back out again.  While promotion/conversion rules probably make this not too awful, we should strive to keep the interface well-typed.

I think you have two options:

1) Manually re-implement most of AtomicBase here, with proper bool-taking/returning types; or
2) protected inheritance from AtomicBase<uint32_t, Order> and writing bool-taking/returning wrappers around the operations.

::: mfbt/tests/TestAtomics.cpp
@@ +181,5 @@
> +  // Test exchange.
> +  atomic = false;
> +  result = atomic.exchange(true);
> +  MOZ_ASSERT(atomic == true, "Atomic exchange did not work");
> +  MOZ_ASSERT(result == false, "Atomic exchange returned the wrong value");

You should test compareExchange here too, similarly to TestEnumWithOrdering.
Attachment #8367990 - Flags: review?(nfroyd) → feedback+
Attached patch atomic_bool2 (obsolete) — Splinter Review
Attachment #8367990 - Attachment is obsolete: true
Attachment #8368077 - Flags: review?(nfroyd)
Attached patch atomic_bool2 (obsolete) — Splinter Review
Attachment #8368077 - Attachment is obsolete: true
Attachment #8368077 - Flags: review?(nfroyd)
Attachment #8368085 - Flags: review?(nfroyd)
Comment on attachment 8368085 [details] [diff] [review]
atomic_bool2

Review of attachment 8368085 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there, just a few more things to fix.

::: mfbt/Atomics.h
@@ +1018,5 @@
> + *   [atomic.types.generic]p9.
> + *
> + * - It's not obvious whether the 8-bit atomic functions on Windows are always
> + *   inlined or not. If they are not inlined those functions may not be
> + *   available on Windows XP. This is why we implement the atomic bool

"If they are not inlined, the corresponding functions in the runtime library are not available on Windows XP."

@@ +1019,5 @@
> + *
> + * - It's not obvious whether the 8-bit atomic functions on Windows are always
> + *   inlined or not. If they are not inlined those functions may not be
> + *   available on Windows XP. This is why we implement the atomic bool
> + *   using unit32_t.

"This is why we implement Atomic<bool> with an underlying type of uint32_t."

@@ +1023,5 @@
> + *   using unit32_t.
> + */
> +template<MemoryOrdering Order>
> +class Atomic<bool, Order>
> +  : public detail::AtomicBase<uint32_t, Order>

This still exposes the uint32_t versions of the functions.  You should use protected inheritance here.

@@ +1031,5 @@
> +  public:
> +    MOZ_CONSTEXPR Atomic() : Base() {}
> +    MOZ_CONSTEXPR Atomic(bool aInit) : Base(aInit) {}
> +
> +    // We provide a type safe wrapper for the underlying AtomicBase

"We provide boolean wrappers for the underlying AtomicBase methods."
Attachment #8368085 - Flags: review?(nfroyd) → feedback+
Attached patch atomic_bool3 (obsolete) — Splinter Review
Attachment #8368085 - Attachment is obsolete: true
Attachment #8368137 - Flags: review?(nfroyd)
Comment on attachment 8368137 [details] [diff] [review]
atomic_bool3

Review of attachment 8368137 [details] [diff] [review]:
-----------------------------------------------------------------

I assume you are folding this into the previous patch.  r=me on the whole thing if so.
Attachment #8368137 - Flags: review?(nfroyd) → review+
Attached patch atomic_bool3 (obsolete) — Splinter Review
Attachment #8368137 - Attachment is obsolete: true
Attachment #8368148 - Attachment is obsolete: true
Keywords: checkin-needed
So, the fun of C++.

[class.conv]p5 says that conversion functions from a derived class do not hide conversion functions in a base class unless they both convert to the same type.  That makes some amount of sense.  [class.conv]p2 says that access control is applied *after* ambiguity resolution.  So |operator uint32_t()| and |operator bool()| are both available in an expression like |atomicBool == false|, even though |operator uint32_t()| isn't actually accessible.

So why are we selecting |operator uint32_t()| in the first place?  I believe this is because, according to the table at [over.ics.scs]p3, |operator uint32_t()| counts as an integral promotion, whereas |operator bool()| counts as a boolean conversion.  Promotion is to be preferred over conversion for the purposes of overload resolution, so |operator uint32_t()| gets selected.  There's also some verbiage at [over.ics.user] that probably applies to this case, but I think that is the jist of it.  (Implicit promotion to int may also take effect here, which is another wrinkle.)

Viktor just pointed out http://en.cppreference.com/w/cpp/language/implicit_cast and http://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool, too.  C++ is awful.
This problem keeps coming up.  Somebody should just add the infrastructure for the safe bool idiom to MFBT so that we can start using it.
The problem in this particular case is not really with the safe bool itself. The safe bool would be nice improvement, but the real issue is that we have two () operators and they both match when comparing with a bool. There are two possible ways for == to compare them, so it complains that the statement is ambiguous. I don't think we really need safe bools for this to work. They just provide additional compile time checks so that no one can write a == b where a is Atomic<bool> and b is unit_32.
Attachment #8368157 - Attachment is obsolete: true
Attachment #8368696 - Flags: review?(nfroyd)
(typo)
Attachment #8368696 - Attachment is obsolete: true
Attachment #8368696 - Flags: review?(nfroyd)
Attachment #8368699 - Flags: review?(nfroyd)
(In reply to comment #14)
> The problem in this particular case is not really with the safe bool itself.
> The safe bool would be nice improvement, but the real issue is that we have two
> () operators and they both match when comparing with a bool. There are two
> possible ways for == to compare them, so it complains that the statement is
> ambiguous. I don't think we really need safe bools for this to work. They just
> provide additional compile time checks so that no one can write a == b where a
> is Atomic<bool> and b is unit_32.

Oh, sorry, yes.  As long as you have a uint32_t conversion, you're doomed.  :-)
Comment on attachment 8368699 [details] [diff] [review]
8368696: Bug 965837 - Add bool support to mozilla::Atomic

Review of attachment 8368699 [details] [diff] [review]:
-----------------------------------------------------------------

I only super-skimmed the logic/test bits, so don't take this as a review.  :-)  I'll never remember the argument meanings for compareAndSwap and exchange and the non-trivial cases without looking it up each time.  (Or doing lots, lots more atomic code than I do at present.)

::: mfbt/Atomics.h
@@ +845,5 @@
>      MOZ_CONSTEXPR AtomicBase() : mValue() {}
>      MOZ_CONSTEXPR AtomicBase(T aInit) : mValue(aInit) {}
>  
> +    // Note: we can't provide operator T() here because Atomic<bool> inherits
> +    // from AtimcBase with T=unit32_t and not T=bool. If we implemented

AtomicBase
Comment on attachment 8368699 [details] [diff] [review]
8368696: Bug 965837 - Add bool support to mozilla::Atomic

Review of attachment 8368699 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with Jeff's comments addressed too.  Please make sure to build debug on at least one major platform before pushing. :)

::: mfbt/Atomics.h
@@ +845,5 @@
>      MOZ_CONSTEXPR AtomicBase() : mValue() {}
>      MOZ_CONSTEXPR AtomicBase(T aInit) : mValue(aInit) {}
>  
> +    // Note: we can't provide operator T() here because Atomic<bool> inherits
> +    // from AtimcBase with T=unit32_t and not T=bool. If we implemented

T=uint32_t

@@ +847,5 @@
>  
> +    // Note: we can't provide operator T() here because Atomic<bool> inherits
> +    // from AtimcBase with T=unit32_t and not T=bool. If we implemented
> +    // operator T() here, it would cause errors when comparing Atomic<bool> with
> +    // a regular bool

Period at the end of sentence.
Attachment #8368699 - Flags: review?(nfroyd) → review+
Attachment #8368699 - Attachment is obsolete: true
Oops... the previous push to try didn't have osx and wasn't a debug build. Here's another one: https://tbpl.mozilla.org/?tree=Try&rev=c0767287d956
Keywords: checkin-needed
Blocks: 969165
https://hg.mozilla.org/mozilla-central/rev/fb87577b5221
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: