Closed
Bug 987305
Opened 10 years ago
Closed 10 years ago
Make EnumSerializer templated on a EnumValidator, rename the old behavior to Contiguous*, and add BitFlags variant
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
Attachments
(3 files, 5 obsolete files)
30.39 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
2.69 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
32.06 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
EnumSerializer takes an enum, and two bounds so it can check that values are in an interval of the form [smallestLegal, highBound) i.e. highBound is the first illegal value. That prevents us from serializing enum types where the highest representable value is legal, which is typical with bit flags, and we're making Typed enums usable as bitflags. The other thing is that this interval check always was very crude, since for many enums the legal value range is non-contiguous, and in the case of typed enums, such checking is mostly useless in the first place as typed enums make it hard to accidentally pass in bad values, which is what this check tries to prevent. So it seems to make sense to make TypedEnumSerializer not do bounds checking. Along the way, this patch fixes a bug where EnumSerializer and TypedEnumSerializer were coercing values to int, losing the high bits in the case of a 64bit enum. This depends on bug 987274 for IntegerTypeTraits.
Attachment #8395854 -
Flags: review?(nfroyd)
Comment 1•10 years ago
|
||
Comment on attachment 8395854 [details] [diff] [review] Make TypedEnumSerializer not do bounds checking Review of attachment 8395854 [details] [diff] [review]: ----------------------------------------------------------------- Using the unsigned integer type is definitely good on its own. I'm curious about your thoughts on my comments below. Come talk to me tomorrow if you'd rather discuss this in person. ::: ipc/glue/IPCMessageUtils.h @@ +148,5 @@ > + * bit-fields and it would be inconvenient to have to specify range boundaries > + * for bit-fields, and anyway that would require a different range > + * representation than the [smallestLegal, highGuard) convention used by > + * EnumSerializer, as there is no way to represent the range of a bit field > + * using the highest bit in that representation. I don't know if I buy this. You could do something like (not entirely sure that the high bound is exactly this) static bool IsLegalValue(const paramType& aValue) { return smallestLegal <= ... && ... < powerOfTwoCeiling(highBound+1); } Granted, there are issues: - powerOfTwoCeiling is a really gross estimate of the limit, but if you just want to make sure you're not introducing bad bitflags, it's good enough. - powerOfTwoCeiling might overflow the range of the unsigned integer type for the enum. I assert that you can take care of this with a careful implementation. (The 64-bit typed enum case is tricky, but I still think you can do it.) Can you make the typed-enum-as-bitflags has a distinct, mixin-inherited type or something so we could differentiate between the two in the serializer?
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #1) > Comment on attachment 8395854 [details] [diff] [review] > Make TypedEnumSerializer not do bounds checking > > Review of attachment 8395854 [details] [diff] [review]: > ----------------------------------------------------------------- > > Using the unsigned integer type is definitely good on its own. I'm curious > about your thoughts on my comments below. Come talk to me tomorrow if you'd > rather discuss this in person. > > ::: ipc/glue/IPCMessageUtils.h > @@ +148,5 @@ > > + * bit-fields and it would be inconvenient to have to specify range boundaries > > + * for bit-fields, and anyway that would require a different range > > + * representation than the [smallestLegal, highGuard) convention used by > > + * EnumSerializer, as there is no way to represent the range of a bit field > > + * using the highest bit in that representation. > > I don't know if I buy this. You could do something like (not entirely sure > that the high bound is exactly this) > > static bool IsLegalValue(const paramType& aValue) { > return smallestLegal <= ... && ... < powerOfTwoCeiling(highBound+1); > } > > Granted, there are issues: > > - powerOfTwoCeiling is a really gross estimate of the limit, but if you just > want to make sure you're not introducing bad bitflags, it's good enough. > - powerOfTwoCeiling might overflow the range of the unsigned integer type > for the enum. I assert that you can take care of this with a careful > implementation. (The 64-bit typed enum case is tricky, but I still think > you can do it.) That's the issue: we can't do it with the same API, we'd need to find a different API, e.g. Low <= x <= High instead of Low <= x < High, but then it would be very tricky if the API for TypedEnumSerializer was subtly different from EnumSerializer (<= instead of <) so maybe we'd want to change EnumSerializer too, but that would require adapting many existing enums to have a Max value not a Max+1 value, which in turn is a lot less convenient because of how C++ enums work (each value defaulting to the previous one plus 1). Since this bounds checking seemed to add so little value for Typed enums, this didn't seem worth it. > > Can you make the typed-enum-as-bitflags has a distinct, mixin-inherited type > or something so we could differentiate between the two in the serializer? I actually had that working locally, with TypedEnumSerializer doing range checking like EnumSerializer, and then a UncheckedTypedEnumSerializer variant not doing range checking, but it was looking stupid because nothing used TypedEnumSerializer and everything wanted to use UncheckedTypedEnumSerializer. Maybe we could rename EnumSerializer to RangedEnumSerializer and TypedEnumSerializer to RangedTypedEnumSerializer; and then, add a TypedEnumSerializer not doing any range checking?
Assignee | ||
Updated•10 years ago
|
Summary: Make TypedEnumSerializer not do bounds checking → Make EnumSerializer templated on a EnumValidator, rename the old behavior to Contiguous*
Assignee | ||
Comment 3•10 years ago
|
||
Different approach :-)
Attachment #8395854 -
Attachment is obsolete: true
Attachment #8395854 -
Flags: review?(nfroyd)
Attachment #8397357 -
Flags: review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Summary: Make EnumSerializer templated on a EnumValidator, rename the old behavior to Contiguous* → Make EnumSerializer templated on a EnumValidator, rename the old behavior to Contiguous*, and add BitFlags variant
Assignee | ||
Comment 4•10 years ago
|
||
Now with updated comments.
Attachment #8397357 -
Attachment is obsolete: true
Attachment #8397357 -
Flags: review?(nfroyd)
Attachment #8397375 -
Flags: review?(nfroyd)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8397375 -
Attachment is obsolete: true
Attachment #8397375 -
Flags: review?(nfroyd)
Attachment #8397380 -
Flags: review?(nfroyd)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8397381 -
Flags: review?(nfroyd)
Assignee | ||
Comment 7•10 years ago
|
||
Updated to work around a GCC bug found on try (I use clang locally).
Attachment #8397380 -
Attachment is obsolete: true
Attachment #8397380 -
Flags: review?(nfroyd)
Attachment #8397518 -
Flags: review?(nfroyd)
Assignee | ||
Comment 8•10 years ago
|
||
Updated to fix a comment.
Attachment #8397381 -
Attachment is obsolete: true
Attachment #8397381 -
Flags: review?(nfroyd)
Attachment #8397520 -
Flags: review?(nfroyd)
Comment 9•10 years ago
|
||
Comment on attachment 8397518 [details] [diff] [review] Refactor EnumSerializer Review of attachment 8397518 [details] [diff] [review]: ----------------------------------------------------------------- r=me. I looked at the IPCMessageUtils.h changes and skimmed some of the other headers, assuming that the compiler will complain appropriately. ::: ipc/glue/IPCMessageUtils.h @@ +105,5 @@ > +template <typename E, typename EnumValidator> > +struct EnumSerializer { > + typedef E paramType; > + typedef typename mozilla::StdintTypeForSizeAndSignedness< > + sizeof(paramType), false>::Type Unrelated to this patch, but can we please not have templates like this take boolean parameters? Have them take enums with real names, please. @@ +142,5 @@ > +class ContiguousTypedEnumValidator > +{ > + // Silence overzealous -Wtype-limits bug in GCC fixed in GCC 4.8: > + // "comparison of unsigned expression >= 0 is always true" > + // http://gcc.gnu.org/bugzilla/show_bug.cgi?id=11856 Fun.
Attachment #8397518 -
Flags: review?(nfroyd) → review+
Updated•10 years ago
|
Attachment #8397520 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 10•10 years ago
|
||
+1 about boolean parameters being bad, (and my bad, I wrote this a long time ago).
Comment 11•10 years ago
|
||
(In reply to Benoit Jacob [:bjacob] from comment #10) > +1 about boolean parameters being bad, (and my bad, I wrote this a long time > ago). I said this because presumably you are the person implementing StdintTypeForSizeAndSignedness. ;) Go forth and fix it!
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8398114 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Main patch (refactor EnumSerializer) https://hg.mozilla.org/integration/mozilla-inbound/rev/17597c063177 Leaving open for part two (BitFlagEnumSerializer), which I'll land together with the stuff that needs it.
Whiteboard: [leave open]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bjacob
Assignee | ||
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d424037ce20
Whiteboard: [leave open]
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d424037ce20
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•