580904 - assertion by JS_STATIC_ASSERT(sizeof(jsval) == 8) after fatval merge

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Makoto Kato [:m_kato]]

since sizeof(jsval) == 16, not 8, static assertion is failed on Win64 build.

Because sizeof(jsval_layout) == 16 on MSVC9++ x64 by following union define.

typedef union jsval_layout
{
  struct {
    unsigned long long payload47 : 47;
    unsigned long tag : 17;  <--- if this type is unsigned long long, sizeof() becomes 8
  } debugView;
};

Since debugView into jsval_layout is unused, we should remove it.

Comment 1

[Makoto Kato [:m_kato]]

compiler's error message

c:\workspace\hg.mozilla.org\tracemonkey\js\src\jsvalue.h(910) : error C2118: negative subscript
c:\workspace\hg.mozilla.org\tracemonkey\js\src\jsvalue.h(911) : error C2118: negative subscript
c:\workspace\hg.mozilla.org\tracemonkey\js\src\jsobj.h(758) : error C2118: negative subscript

Comment 2

[Sean Stangl [:sstangl]]

debugView is used for easy value inspection in gdb. Please don't remove it :)

Since 'tag' is only 17 bits, we could just use 'unsigned' instead of 'unsigned long'. Does MSVC play nicely with that?

Comment 3

[Makoto Kato [:m_kato]]

(In reply to comment #2)
> debugView is used for easy value inspection in gdb. Please don't remove it :)

Since this code is for gcc/gdb, we should use _MSC_VER macro for MSVC++.
 
> Since 'tag' is only 17 bits, we could just use 'unsigned' instead of 'unsigned
> long'. Does MSVC play nicely with that?

If JSValueTag is uint64, sizeof(jsval_layout) becomes 8 on MSVC.  Since the type of uint64 and JSValueTag is mismatch, MSVC doesn't pack each members.

Comment 4

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 5

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 6

[Sean Stangl [:sstangl]]

(In reply to comment #3)
> If JSValueTag is uint64, sizeof(jsval_layout) becomes 8 on MSVC.  Since the
> type of uint64 and JSValueTag is mismatch, MSVC doesn't pack each members.

I am unfamiliar with MSVS, but looked into how it works with packed bitfields out of morbid curiosity. Its implementation of the specification is useless, and there does not appear to be a workaround.

So the above patch sounds reasonable.

Luke Wagner is the fatval maintainer: switching review to him.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 459329 [details] [diff] [review]
fix

>+#ifndef _MSC_VER
>+    /* Don't need gdb hack on Microsoft C++ compiler */
>     struct {
>         uint64             payload47 : 47;
>         JSValueTag         tag : 17;
>     } debugView;
>+#endif

If the problem is only on x64, why don't we preserve ease-of-debugging for x86 users by changing the #ifdef to:

#if !(defined(_MSC_VER) && JS_BITS_PER_WORD == 64)

Also, it's not that we don't need it, and its also not a "hack", so how about:

/* MSVC does not pack these correctly :-( */

Comment 8

[Makoto Kato [:m_kato]]

(In reply to comment #7)
> Comment on attachment 459329 [details] [diff] [review]
> fix
> 
> >+#ifndef _MSC_VER
> >+    /* Don't need gdb hack on Microsoft C++ compiler */
> >     struct {
> >         uint64             payload47 : 47;
> >         JSValueTag         tag : 17;
> >     } debugView;
> >+#endif
> 
> If the problem is only on x64, why don't we preserve ease-of-debugging for x86
> users by changing the #ifdef to:
> 
> #if !(defined(_MSC_VER) && JS_BITS_PER_WORD == 64)

This block is already into "# elif JS_BITS_PER_WORD == 64".  Do I need double check like your comment?

Comment 9

[Luke Wagner [:luke]]

Oops, I missed that, thanks.  r+ then.

Comment 10

[Luke Wagner [:luke]]

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

Comment 11

[Jacek Caban]

Attachment: mingw-w64 fix

mingw-w64 has the same problem, so the check should be for _WIN64, not compiler specific.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 461919 [details] [diff] [review]
mingw-w64 fix

Thank you sir!

Comment 13

[Jacek Caban]

Thanks for the review, pushed to TM:

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

Comment 14

[[Baboo]]

Shouldn't you have changed the comment too?

Comment 15

[Jacek Caban]

Attachment: comment fix

Probably yes, but most people don't care about mingw, so I didn't want to make comments clean.

Comment 16

[Robert Sayre]

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