Bug 497618 causes crash on Win x64 build

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
x86_64
Windows Vista
Points:
---
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch patch v0 (obsolete) — Splinter Review
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.
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.
(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.
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #385997 - Attachment is obsolete: true
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.
Attachment #386300 - Flags: review?(jwalden+bmo) → review-
(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.

Updated

10 years ago
Attachment #387836 - Flags: review?(jwalden+bmo) → review+
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!
1.9.2-fodder when sayrer, The One-Man Merge Machine, comes around to clean up this one-horse town...
Flags: wanted1.9.2?
Whiteboard: fixed-in-tracemonkey

Comment 11

10 years ago
http://hg.mozilla.org/mozilla-central/rev/c400f2176840
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: wanted1.9.2? → wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.