501324 - Bug 497618 causes crash on Win x64 build

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Makoto Kato [:m_kato]]

Attachment: patch v0

After fixing bug 497618, My win64 build always crash.

Because the type of enum is "int".

jsstr.h doesn't set specific type to enum.  Some enum values are overflow due to int, so this problem occurs.

Comment 1

[timeless]

http://www.eggheadcafe.com/conversation.aspx?messageid=31447908&threadid=31441371
eVC4 doesn't seem to support this extension (: size), but VS2005 *does*.

http://msdn.microsoft.com/en-us/library/2dzy4k6e(loband).aspx
http://msdn.microsoft.com/en-us/library/2dzy4k6e(VS.71,loband).aspx

So, this would break 7.1 among other compilers.

Comment 2

[Jeff Walden [:Waldo]]

Why is the type of enum int?  The standard says the type of an enum is the largest needed to contain all its constituent values.

Note that in bug 497207 when I had need to constrain the size of an enum I had to ifdef the sizing part of it.  C++0x has sized-enum support, but for now I had to resort to compiler-specific ifdefs to make it all work.  If it really is necessary to use this hack, it definitely needs to be ifdef-guarded in a similar manner.

Comment 3

[Makoto Kato [:m_kato]]

(In reply to comment #2)
> Why is the type of enum int?  The standard says the type of an enum is the
> largest needed to contain all its constituent values.

Microsoft's design:-).  VC++ 8/9 seems to be enum is same as integer.

Also, their document (http://msdn.microsoft.com/ja-jp/library/2dzy4k6e.aspx) said,
"An enumerator can be promoted to an integer value. However, converting an integer to an enumerator requires an explicit cast, and the results are not defined if the integer value is outside the range of the defined enumeration."

I don't test a patch on all platforms, yet. After having tested it, I will send review.

Comment 4

[Makoto Kato [:m_kato]]

Attachment: patch v1

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 386300 [details] [diff] [review]
patch v1

I looked up the verbiage in ISO C++; it's not quite what I thought it was, but the result is the same:

  The underlying typeof an enumeration is an integral type
  that can represent all the enumerator values defined in the
  enumeration. It is implementation-defined which integral
  type is used as the underlying type for an enumeration
  except that the underlying type shall not be larger than
  int unless the value of an enumerator cannot fit in an int
  or unsigned int.

This is a definite compiler bug.


>diff --git a/js/src/jsstr.h b/js/src/jsstr.h

>+/*
>+ * VC++ 64-bit generates enum type as integer by default.
>+ */
>+#if defined(_MSC_VER) && defined(_WIN64)
>+#define JS_ENUM_SIZE_T enum : size_t
>+#else
>+#define JS_ENUM_SIZE_T enum
>+#endif

I think I would prefer this be inlined at the two places where it's needed, rather than completely obscuring the enum-ness of this.

>-    enum {
>+    JS_ENUM_SIZE_T {
>         DEPENDENT =     JSSTRING_BIT(JS_BITS_PER_WORD - 1),

So instead:

>-    enum {
>+    enum
>+#if defined(_MSC_VER) && defined(_WIN64)
>+    : size_t /* VC++ 64-bit incorrectly default this enum's size to int. */
>+#endif
>+    {
>         DEPENDENT =     JSSTRING_BIT(JS_BITS_PER_WORD - 1),


>-        DEPENDENT_LENGTH_BITS = LENGTH_BITS / 2,
>+        /* need size_t cast due to compile error of VC++ 64-bit */
>+        DEPENDENT_LENGTH_BITS = (size_t)LENGTH_BITS / 2,

Shouldn't LENGTH_BITS be treated as the type of the overall enum at this point, that is as size_t?  In any case, isn't the value far smaller than int-sized anyway?  If there actually is a problem this fixes, I'm not fully convinced this is the right fix for it.  If it truly is, use a C++-style cast here, i.e. size_t(LENGTH_BITS).

> public:
>-    enum {
>+    JS_ENUM_SIZE_T {
>         MAX_LENGTH = LENGTH_MASK,

Same #ifdef-ing as requested above here.

Comment 6

[Makoto Kato [:m_kato]]

(In reply to comment #5)

Thank you for reviewing this.

> Shouldn't LENGTH_BITS be treated as the type of the overall enum at this point,
> that is as size_t?  In any case, isn't the value far smaller than int-sized
> anyway?  If there actually is a problem this fixes, I'm not fully convinced
> this is the right fix for it.  If it truly is, use a C++-style cast here, i.e.
> size_t(LENGTH_BITS).

VC++ throws error "Conversion to enumeration type requires an explicit cast" without cast.  So I need cast.

Comment 7

[Makoto Kato [:m_kato]]

Attachment: patch v2

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 387836 [details] [diff] [review]
patch v2

>diff --git a/js/src/jsstr.h b/js/src/jsstr.h

>+    enum
>+#if defined(_MSC_VER) && defined(_WIN64)
>+        : size_t /* VC++ 64-bit incorrectly default this enum's size to int. */
>+#endif

Don't overindent the ": size_t" here (or in the other instance), by which I mean the "e" in enum and the ":" should line up vertically.  This comports with formatting of the JSTraceType_ enum in jstracer.h -- the barest minimum of precedent, but precedent nonetheless.


>+        /*
>+         * VC++ 64-bit throws error "Conversion to enumeration type requires
>+         * an explicit cast" without cast.  So we need cast.
>+         */
>+        DEPENDENT_LENGTH_BITS = size_t(LENGTH_BITS) / 2,

This isn't quite clear, idiomatic language; please use the following for the comment tesxt:

VC++ 64-bit incorrectly produces the compiler error "Conversion to enumeration type requires an explicit cast" unless we cast to size_t here.

I'll promptly r+ and commit a patch with these nits addressed.  Thanks!

Comment 9

[Jeff Walden [:Waldo]]

1.9.2-fodder when sayrer, The One-Man Merge Machine, comes around to clean up this one-horse town...

Comment 10

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/c400f2176840

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c400f2176840