Closed Bug 580904 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: m_kato, Unassigned)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files, 1 obsolete file)

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.
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
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?
(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.
Attached patch fix (obsolete) — Splinter Review
Attached patch fixSplinter Review
Attachment #459328 - Attachment is obsolete: true
Attachment #459329 - Flags: review?(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.
Attachment #459329 - Flags: review?(sstangl) → review?(lw)
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 :-( */
(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?
Oops, I missed that, thanks.  r+ then.
Attachment #459329 - Flags: review?(lw) → review+
Attached patch mingw-w64 fixSplinter Review
mingw-w64 has the same problem, so the check should be for _WIN64, not compiler specific.
Attachment #461919 - Flags: review?(lw)
Comment on attachment 461919 [details] [diff] [review]
mingw-w64 fix

Thank you sir!
Attachment #461919 - Flags: review?(lw) → review+
Thanks for the review, pushed to TM:

http://hg.mozilla.org/tracemonkey/rev/b8d51faf7ee5
Shouldn't you have changed the comment too?
Attached patch comment fixSplinter Review
Probably yes, but most people don't care about mingw, so I didn't want to make comments clean.
http://hg.mozilla.org/mozilla-central/rev/e2bf3ec85c46
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.