Closed Bug 987290 Opened 6 years ago Closed 6 years ago

Allow using MFBT Typed Enums as bitwise flags

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(4 files, 9 obsolete files)

38.44 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.34 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
889 bytes, patch
Details | Diff | Splinter Review
2.77 KB, patch
Details | Diff | Splinter Review
Typing is good, it makes us catch real bugs. Watch the dependent bugs list here: this actually found a serious bug in gfx code.

Traditionally, the way one allows using an enum as bitwise flags is by defining bitwise operators for it, as global functions. We already have a few occurences of that throughout gecko. The present patch adds a MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(EnumClass) macro that is all what one needs to use to enable using a given enum class as bit flags. It's not called by default i.e. usage as bitflags remains opt-in, needs to be manually enabled for each enum type. Notice the name, MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS, is intentionally explicit about what this does rather than what this is useful for, because I believe that it's good to have users aware of what macros actually do rather than rely on opaque magic.

There is some nontrivialness here, that is explained in code comments, but let's quickly mention it here too. We have a lot of existing code doing

   if (myflags & SOME_FLAG) { ...

Such code expects flags to be contextually convertible to bool. The problem is that there is no way to make typed enums convertible to bool: type conversion operators cannot be overloaded from the outside, and enum classes can't have members.

The way we make this work, allowing existing user code to stay unchanged, is we have operator& and other bitwise binary operators have a return type that is a class that wraps the typed enum, and adds the bool conversion operator. That is BoolCastableTypedEnumResult. This depends on bug 987253 for 'explicit' specifiers on conversion operators.

We could have made a different compromise, as explained in a code comment, that would have required adjusting user code. But since the goal here is to be an easy drop-in replacement for the untyped flags that we have all over the place, the value of allowing user code to stay unchanged seems quite high, IMHO high enough to justify having this admittedly crazy BoolCastableTypedEnumResult trick.

That said, we are being careful here. To avoid this being a cause of inefficiency (*), we ensure that BoolCastableTypedEnumResult is a literal type in the C++11 sense (by sprinkling MOZ_CONSTEXPR) and we even verify that in the TestTypedEnum unit test, which is greatly expanded along the way.

(*) : more about the possibility of inefficiency. Given how bit flags are used, I have little doubt that compilers will have no trouble optimizing BoolCastableTypedEnumResult away. I am a bit more concerned about the possibility of adding static constructors if people do things like this at file scope:

const MyFlags kSomeSpecialFlagsCombo = MyFlags::A | MyFlags::B;

Indeed, this constructs a BoolCastableTypedEnumResult at static initialization time. By ensuring that BoolCastableTypedEnumResult is a literal type, I hope to be doing the best I can to make the compiler understand that it can evaporate it away. And while literal types are a C++11 concept, this should help modern compilers regardless of being in c++11 mode. Suggestions welcome if something else needs to be done.
Attachment #8395826 - Flags: review?(jwalden+bmo)
Note that this patch also ensures that EnumClass::Enum is a literal type, which it wasn't before. For this reason, I believe that the reviewer should feel compelled to r+, otherwise the ultimate failure of Mozilla will be blamed on him.
Blocks: 987311
Comment on attachment 8395826 [details] [diff] [review]
Allow using typed enums as bit fields

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

::: mfbt/TypedEnum.h
@@ +322,5 @@
>  
>  #  define MOZ_BEGIN_ENUM_CLASS(...) MOZ_BEGIN_NESTED_ENUM_CLASS(__VA_ARGS__)
>  #  define MOZ_END_ENUM_CLASS(Name) \
>       MOZ_END_NESTED_ENUM_CLASS(Name) \
> +     //MOZ_FINISH_NESTED_ENUM_CLASS(Name)

?
Excellent catch! I needed that at some point to get things to compile in the non-c++11-strong-enums case. I looked back at it; it turns out that the only problem is that the unary operator~ generated by this FINISH macro makes ~MyEnum::MyValue ambiguous with the new operator~ introduced here at least with clang 3.3.  Apparently, being MOZ_DELETE'd doesn't remove an overload from the pool of overloads that might make an expression ambiguous.

My quick fix is to remove this MOZ_DELETE'd operator~ from the FINISH macro. If you are not happy about this, then the alternative is to require using a different FINISH macro when the enum is meant to be used as bit field. But I liked the idea that enabling usage as bit field consists of nothing more than adding some operators to the existing typed enum.
Attachment #8395826 - Attachment is obsolete: true
Attachment #8395826 - Flags: review?(jwalden+bmo)
Attachment #8395913 - Flags: review?(jwalden+bmo)
Needed to generate bitwise operators also for BoolCastableTypedEnumResult for chained ops like A & B & C to compile successfully on B2G GCC 4.4.
Attachment #8395913 - Attachment is obsolete: true
Attachment #8395913 - Flags: review?(jwalden+bmo)
Attachment #8396095 - Flags: review?(jwalden+bmo)
Comment on attachment 8396095 [details] [diff] [review]
Allow using typed enums as bit fields

Temporarily cancelling this review while I'm fixing a bug. The operator bool in BoolCastableTypedEnumResult was kicking in more often than its turn, when the plain integer value of the enum was wanted, giving wrong results on more complex boolean expressions than the current unit test covered. Will come back with a fix (templated explicit conversion operator) and expanded unit test.
Attachment #8396095 - Flags: review?(jwalden+bmo)
Alright, the above-mentioned trick (have only one type conversion operator, explicit, templated in the destination type, to avoid causing any unwanted type coercion) works, and the unit test is now much expanded to cover all sorts of boolean expressions and all integer types. Ready for review!
Attachment #8396095 - Attachment is obsolete: true
Attachment #8396422 - Flags: review?(jwalden+bmo)
Yet more fixes, needed to build on B2G GCC 4.4.

Also, this version has the bitwise stuff added to a new TypedEnumBits.h file, so as to keep TypedEnum.h short and sweet (and cheap to include, although c++ templates *should* be cheap to include as long as we don't instantiate them).
Attachment #8396422 - Attachment is obsolete: true
Attachment #8396422 - Flags: review?(jwalden+bmo)
Attachment #8396702 - Flags: review?(jwalden+bmo)
Attachment #8396702 - Attachment is obsolete: true
Attachment #8396702 - Flags: review?(jwalden+bmo)
Attachment #8396705 - Flags: review?(jwalden+bmo)
Finally green on try! https://tbpl.mozilla.org/?tree=Try&rev=f96fcccad22d
Attachment #8396705 - Attachment is obsolete: true
Attachment #8396705 - Flags: review?(jwalden+bmo)
Attachment #8397029 - Flags: review?(jwalden+bmo)
So for a while I thought that it was horrible that my patch was making bitwise expressions on typed enums explicitly castable to other enum classes. Then I realize that in C++11, enum classes are explicitly castable to other enum classes.

Here is still a new version of the patch:
 - adds unit test verifying that implicit conversions are not allowed (again, explicit conversions are intentionally allowed to follow c++11 enum class behavior)
 - adds unit test verifying that no truncation of high bits occurs
 - fixes a truncation bug in the non-c++11 impl of MOZ_BEGIN_ENUM_CLASS found by that unit test: the constructor taking an int was being implicitly used, coercing 64bit values to it. Fixed by templating that ctor on the integer type.
Attachment #8397029 - Attachment is obsolete: true
Attachment #8397029 - Flags: review?(jwalden+bmo)
Attachment #8397316 - Flags: review?(jwalden+bmo)
Attachment #8397316 - Attachment is obsolete: true
Attachment #8397316 - Flags: review?(jwalden+bmo)
Attachment #8398113 - Flags: review?(jwalden+bmo)
Review ping!
Comment on attachment 8398113 [details] [diff] [review]
Allow using typed enums as bit fields

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

Generally pretty good, but we need to figure out the include story, and there's a bit of formatting-nicety that's needful.

My eyes were glazing over by the end of TestTypedEnum.cpp, but it's probably good.  :-)  Except for the lack of handling of binary CTER<E> operators.  Testing that would require some pairwise craziness.  At least do some ad hoc testing of that; if you want to go whole hog and add exhaustive pairwise testing, feel free, but I'm not going to hold you to it.

::: mfbt/TypedEnumBits.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS allows using a typed enum as bit flags */

Period at end of sentence.

@@ +8,5 @@
> +
> +#ifndef mozilla_TypedEnumBits_h
> +#define mozilla_TypedEnumBits_h
> +
> +#include "mozilla/TypedEnum.h"

Uh.  These two headers circularly include each other.  Which is the master, and which is the apprentice?  If there's no clear answer, which there appears to not be, they need to be combined.

@@ +9,5 @@
> +#ifndef mozilla_TypedEnumBits_h
> +#define mozilla_TypedEnumBits_h
> +
> +#include "mozilla/TypedEnum.h"
> +#include "mozilla/IntegerTypeTraits.h"

Alphabetical.

@@ +49,5 @@
> +    : mValue(value)
> +  {}
> +
> +  MOZ_CONSTEXPR operator E() const { return mValue; }
> +  MOZ_CONSTEXPR E get() const { return mValue; }

Do we expect there will be actual calls to get()?  Are there any now?  I'd kind of rather there only be one way, that is, E(cter).

@@ +60,5 @@
> +
> +  MOZ_CONSTEXPR bool operator !() const { return !bool(mValue); }
> +};
> +
> +#define BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR(Op, OtherType, ReturnType) \

MOZ_ prefix on this.  Feel free to bikeshed a shorter name, too, if you can think of one.  BINOP or something for concision.  Don't care what.  :-)

@@ +62,5 @@
> +};
> +
> +#define BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR(Op, OtherType, ReturnType) \
> +template <typename E>                                                          \
> +MOZ_CONSTEXPR ReturnType operator Op(const OtherType& e, const CastableTypedEnumResult<E>& r) \

Line break after ReturnType would be nice here, same elsewhere.

@@ +64,5 @@
> +#define BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR(Op, OtherType, ReturnType) \
> +template <typename E>                                                          \
> +MOZ_CONSTEXPR ReturnType operator Op(const OtherType& e, const CastableTypedEnumResult<E>& r) \
> +{                                                                              \
> +  return static_cast<ReturnType>(e Op static_cast<OtherType>(r));              \

OtherType(r) would be more readable, employed most places in this patch.

@@ +86,5 @@
> +BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR(!=, E, bool)
> +BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR(||, bool, bool)
> +BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR(&&, bool, bool)
> +
> +#define BOOLCASTABLETYPEDENUMRESULT_COMPOUND_ASSIGN_OPERATOR(Op) \

Same MOZ_, shortening bit.

@@ +88,5 @@
> +BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR(&&, bool, bool)
> +
> +#define BOOLCASTABLETYPEDENUMRESULT_COMPOUND_ASSIGN_OPERATOR(Op) \
> +template <typename E>                                            \
> +void                                                             \

Technically |a &= b &= c;| normally works and groups as |a &= (b &= c);|, right?  But we're not supporting it here.  It's of dubious value, to be sure, just want to be sure we're wittingly doing this semi-deliberately as a simplification.  I'm fine with it myself, chained assignments are often not super-readable, and not all languages implement it (e.g. JS has |a = b = c| as one or the other ordering of |<tmp> = c; a = <tmp>; b = <tmp>;|).

@@ +105,5 @@
> +#undef BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR
> +
> +#ifndef MOZ_HAVE_CXX11_STRONG_ENUMS
> +
> +#define BOOLCASTABLETYPEDENUMRESULT_BINARY_OPERATOR_EXTRA_NON_CXX11(Op, ReturnType) \

MOZ_, shortening.

Is there a reason we're not supporting (en & Enum::A) | (en & Enum::B) by not having an operator Op(const CTER<E>&, const CTER<E>&)?  Or am I misreading/not thinking hard enough here?

@@ +128,5 @@
> +
> +#endif // not MOZ_HAVE_CXX11_STRONG_ENUMS
> +
> +namespace detail {
> +template <typename E>

template<

::: mfbt/tests/Makefile.in
@@ +14,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  LIBS= $(call EXPAND_LIBNAME_PATH,mfbt,$(DEPTH)/mfbt)
> +
> +CXXFLAGS += -O0 -g3
\ No newline at end of file

Why this at all?  (And it may belong in moz.build these days as a per-file thing, if it belongs at all.  Probably it doesn't.)

::: mfbt/tests/TestTypedEnum.cpp
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/Assertions.h"
> +#include "mozilla/TypedEnumBits.h"
> +

Add #include <stdint.h> up here.

@@ +41,5 @@
> +RequireLiteralType(const T&)
> +{
> +  static_assert(std::is_literal_type<T>::value, "Expected a literal type");
> +}
> +#else // not MOZ_HAVE_CXX11_STRONG_ENUMS

That comment looks wrong.

@@ +142,5 @@
> +MAKE_STANDARD_BITFIELD_FOR_TYPE(unsigned_long_long)
> +
> +#undef MAKE_STANDARD_BITFIELD_FOR_TYPE
> +
> +template <typename TypedEnum>

template<
Attachment #8398113 - Flags: review?(jwalden+bmo) → review-
Applied review comments; basically rewrote the unit test to avoid large macros and be much more systematic and exhaustive in its coverage.
Attachment #8398113 - Attachment is obsolete: true
Attachment #8409731 - Flags: review?(jwalden+bmo)
mostly green try push:

https://tbpl.mozilla.org/?tree=Try&rev=08e71f96996b

The emulator-debug red is the subject of the next patch:
Ehsan: see the emulator-debug red on the above try link for why I need this. Regarding the question of "what changed, that made this necessary", the answer is "something in the big patch above", more specifically, in TypedEnum.h in the legacy non-c++11 implementation of MOZ_BEGIN/END_ENUM_CLASS. I tweaked various things that made typed enums more strict, and this audiochannel code was apparently relying on the ability to cast between unrelated enum types.
Attachment #8409734 - Flags: review?(ehsan)
Right, I see it now. The old code in TypedEnum.h had

  explicit Name(int num) : mEnum((Enum)num) {} \

and the new code has:

  template<typename Other> \
  explicit MOZ_CONSTEXPR Name(Other num) : mEnum((Enum)num) {} \

So the old code caused an implicit conversion to int, while the new code, being templated, does not implicitly convert to int, and instead tries to do an explicit conversion directly to the requested type.

The old code was incorrect anyway as even plain c++98 enums can have a range exceeding that of int.
Attachment #8409734 - Flags: review?(ehsan) → review+
Oops, comment 19 didn't make sense as the old code already had 'explicit'. Anyway, thanks for the r+ :-)
Fully green try run with both patches:
https://tbpl.mozilla.org/?tree=Try&rev=87807f1f1ab5
This should be perfectly cromulent for review now.
Comment on attachment 8409731 [details] [diff] [review]
Allow using typed enums as bit fields

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

::: mfbt/TypedEnumBits.h
@@ +15,5 @@
> +namespace mozilla {
> +
> +#define MOZ_CASTABLETYPEDENUMRESULT_BINOP(Op, OtherType, ReturnType) \
> +template<typename E>                                                 \
> +MOZ_CONSTEXPR ReturnType                                             \

Nitpicky, but I'd rather not see the continuations aligned -- just makes more mess when they inevitably have to be realigned.

@@ +29,5 @@
> +}                                                                    \
> +template<typename E>                                                 \
> +MOZ_CONSTEXPR ReturnType                                             \
> +operator Op(const CastableTypedEnumResult<E>& r1,                    \
> +                       const CastableTypedEnumResult<E>& r2)         \

Misaligned.

::: mfbt/tests/TestTypedEnum.cpp
@@ +4,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/TypedEnum.h"
> +#include "mozilla/TypedEnumBits.h"
> +#include <stdint.h>

Blank line before <stdint.h>.

I sort of skimmed this file.  :-)  Looks generally reasonable enough, not hugely worried about some esoteric corner having been missed, it'll show up quickly enough in practice if so.

@@ +303,5 @@
> +}
> +
> +// Similar to TestBinOp but testing the unary ~ operator.
> +template<typename TypedEnum, typename T>
> +void TestTilda(const T& t)

Tilde
Attachment #8409731 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23)
> ::: mfbt/TypedEnumBits.h
> @@ +15,5 @@
> > +namespace mozilla {
> > +
> > +#define MOZ_CASTABLETYPEDENUMRESULT_BINOP(Op, OtherType, ReturnType) \
> > +template<typename E>                                                 \
> > +MOZ_CONSTEXPR ReturnType                                             \
> 
> Nitpicky, but I'd rather not see the continuations aligned -- just makes
> more mess when they inevitably have to be realigned.

FWIW, emacs will smartly realign continuation backslashes when you indent the macro anyway.  I assume vim can do the same.
<insert overwrought statement about editor snobbishness, and anointing particular editors as being things people should use, here -- and that unaligned backslashes are not inherently unreadable in any event :-) >
<insert statement about the joy of receiving r+ for this patch, and the ease of addressing this particular review comment by a regex>
Also, more generally: I only care about equivalence classes of code modulo applying invertible, automatable changes. You'll rarely see me argue strongly one way or the other between two options differing only in this way.
https://hg.mozilla.org/mozilla-central/rev/080ff1f86465
https://hg.mozilla.org/mozilla-central/rev/4ad1f662f521
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Backed out due to the same issues that bug 987311 had:
 https://hg.mozilla.org/mozilla-central/rev/c67a79064fd4

See bug 987311 comment 21, bug 987311 comment 23, bug 987311 comment 24.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla31 → ---
(In reply to Blair McBride [:Unfocused] from comment #30)
> Backed out due to the same issues that bug 987311 had:
>  https://hg.mozilla.org/mozilla-central/rev/c67a79064fd4
> 
> See bug 987311 comment 21, bug 987311 comment 23, bug 987311 comment 24.

FWIW it doesn't seem that these, by themselves, imply that the present patches also had to be backed out, unless I'm missing something. Of course, it's only easy for me to tell because I've been working on these patches --- not complaining here.

(In reply to Mark Capella [:capella] from comment #31)
> fyi, was seeing https://pastebin.mozilla.org/4975389

Whoa, internal compiler error? That doesn't happen on our build slaves, or for me locally. Can you share details of your compiler version, linux distro, etc? maybe `c++ --version`. Also your mozconfig please.
Reproduced the error as I saw it in the Win side ...

Win8U1 w/VS2012 Express for Win Desktop, produces: https://pastebin.mozilla.org/4982280
Regarding the internal compiler error on GCC 4.6:
 - I could not reproduce this on GCC 4.6.2 built from sources, with or without optimization.
 - This was only reported by someone on #introduction, not reproduced by anyone here AFAIU.

So I'm not caring about this particular issue at this point.

Looking into the MSVC issue reported in comment 33 now.
Hooray, I'm all set up on MSVC 2012 now and I can reproduce. And I have 30 days before my trial period expires. I could hardly be happier.
Alright, here's what was happening. These static asserts verify that CastableTypedEnumResult is not implicitly convertible to bool, int, etc. (It should only be explicitly convertible). However, that rests on the availability of c++11 explicit conversion operators. On bug 987253, I decided (from reading a stackoverflow thread, see the comment in the code) to stay away from exposing c++11 explicit conversions on MSVC, even on recent versions. The problem is that the TestTypedEnum.cpp test verifying non-implicit-convertibility forgot to check for availability of explicit conversions before requiring this behavior. The resulting patch is trivial enough that I don't think it requires a review.
https://hg.mozilla.org/mozilla-central/rev/38aa0e936d87
https://hg.mozilla.org/mozilla-central/rev/2b71adff7fe4
https://hg.mozilla.org/mozilla-central/rev/ba55dd0b1a12
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Target Milestone: mozilla32 → mozilla31
I was notified again of GCC 4.6 ICE's, now specifically on 4.6.3, so I built it myself and could reproduce. Landed a simple work-around:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2b85aaa9f3
Nice :-)
Could you land the workaround on aurora so people can build aurora with gcc 4.6?
Flags: needinfo?(bjacob)
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 987290, present bug
User impact if declined: can't build on GCC 4.6.3 due to a GCC 4.6.3 bug. Other GCC versions such as 4.6.2 seem to be unaffected.
Testing completed (on m-c, etc.): been on m-i for half a day.
Risk to taking this patch (and alternatives if risky): Zero risk at all, completely trivial, and not part of the build we ship to users (only touching a unit test compiled as a separate executable)
String or IDL/UUID changes made by this patch: none
Attachment #8414024 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bjacob)
Note: this landed on inbound as ae2b85aaa9f3.
Attachment #8414024 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Yay, the 3 first hex digits of the aurora hash are identical to the central ones. It's going to be a lucky day.
You need to log in before you can comment on or make changes to this bug.