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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(3 files, 5 obsolete files)

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)
Blocks: 987311
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?
(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?
Summary: Make TypedEnumSerializer not do bounds checking → Make EnumSerializer templated on a EnumValidator, rename the old behavior to Contiguous*
Different approach :-)
Attachment #8395854 - Attachment is obsolete: true
Attachment #8395854 - Flags: review?(nfroyd)
Attachment #8397357 - Flags: review?(nfroyd)
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
Attached patch Refactor EnumSerializer (obsolete) — Splinter Review
Now with updated comments.
Attachment #8397357 - Attachment is obsolete: true
Attachment #8397357 - Flags: review?(nfroyd)
Attachment #8397375 - Flags: review?(nfroyd)
Attached patch Refactor EnumSerializer (obsolete) — Splinter Review
Attachment #8397375 - Attachment is obsolete: true
Attachment #8397375 - Flags: review?(nfroyd)
Attachment #8397380 - Flags: review?(nfroyd)
Attached patch Add BitFlagsEnumSerializer (obsolete) — Splinter Review
Attachment #8397381 - Flags: review?(nfroyd)
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)
Updated to fix a comment.
Attachment #8397381 - Attachment is obsolete: true
Attachment #8397381 - Flags: review?(nfroyd)
Attachment #8397520 - Flags: review?(nfroyd)
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+
Attachment #8397520 - Flags: review?(nfroyd) → review+
+1 about boolean parameters being bad, (and my bad, I wrote this a long time 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!
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: nobody → bjacob
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.