mozilla::Atomic should support bool

RESOLVED FIXED in mozilla30

Status

()

Core
MFBT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vikstrous, Assigned: vikstrous)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8367990 [details] [diff] [review]
atomic_bool
Attachment #8367990 - Flags: review?(nfroyd)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 3

4 years ago
Created attachment 8368077 [details] [diff] [review]
atomic_bool2
Attachment #8367990 - Attachment is obsolete: true
Attachment #8368077 - Flags: review?(nfroyd)
(Assignee)

Comment 4

4 years ago
Created attachment 8368085 [details] [diff] [review]
atomic_bool2
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+
(Assignee)

Comment 6

4 years ago
Created attachment 8368137 [details] [diff] [review]
atomic_bool3
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+
(Assignee)

Comment 8

4 years ago
Created attachment 8368148 [details] [diff] [review]
atomic_bool3
Attachment #8368137 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8368157 [details] [diff] [review]
Bug 965837 - Add bool support to mozilla::Atomic; r=froydnj
Attachment #8368148 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f434624463d9
Flags: in-testsuite+
Keywords: checkin-needed
Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/09e27ba697c2

https://tbpl.mozilla.org/php/getParsedLog.php?id=33833357&tree=Mozilla-Inbound
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.

Comment 13

4 years ago
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.
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Comment 15

4 years ago
Created attachment 8368696 [details] [diff] [review]
Bug 965837 - Add bool support to mozilla::Atomic
Attachment #8368157 - Attachment is obsolete: true
Attachment #8368696 - Flags: review?(nfroyd)
(Assignee)

Comment 16

4 years ago
Created attachment 8368699 [details] [diff] [review]
8368696: Bug 965837 - Add bool support to mozilla::Atomic

(typo)
Attachment #8368696 - Attachment is obsolete: true
Attachment #8368696 - Flags: review?(nfroyd)
Attachment #8368699 - Flags: review?(nfroyd)

Comment 17

4 years ago
(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+
(Assignee)

Comment 20

4 years ago
Created attachment 8369563 [details] [diff] [review]
Bug 965837 - Add bool support to mozilla::Atomic r=froydnj
Attachment #8368699 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8369564 [details] [diff] [review]
Bug 965837 - Add bool support to mozilla::Atomic r=froydnj
Attachment #8369563 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c85be9b269a3
(Assignee)

Comment 23

4 years ago
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
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb87577b5221
Keywords: checkin-needed
Blocks: 967926
Blocks: 969165
https://hg.mozilla.org/mozilla-central/rev/fb87577b5221
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.