Closed
Bug 580904
Opened 15 years ago
Closed 15 years ago
assertion by JS_STATIC_ASSERT(sizeof(jsval) == 8) after fatval merge
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: m_kato, Unassigned)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(3 files, 1 obsolete file)
825 bytes,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
357 bytes,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
406 bytes,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
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•15 years ago
|
||
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?
Reporter | ||
Comment 3•15 years ago
|
||
(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.
Reporter | ||
Comment 4•15 years ago
|
||
Reporter | ||
Comment 5•15 years ago
|
||
Attachment #459328 -
Attachment is obsolete: true
Reporter | ||
Updated•15 years ago
|
Attachment #459329 -
Flags: review?(sstangl)
Comment 6•15 years ago
|
||
(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.
Updated•15 years ago
|
Attachment #459329 -
Flags: review?(sstangl) → review?(lw)
![]() |
||
Comment 7•15 years ago
|
||
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 :-( */
Reporter | ||
Comment 8•15 years ago
|
||
(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•15 years ago
|
||
Oops, I missed that, thanks. r+ then.
![]() |
||
Updated•15 years ago
|
Attachment #459329 -
Flags: review?(lw) → review+
![]() |
||
Comment 10•15 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 11•15 years ago
|
||
mingw-w64 has the same problem, so the check should be for _WIN64, not compiler specific.
Attachment #461919 -
Flags: review?(lw)
![]() |
||
Comment 12•15 years ago
|
||
Comment on attachment 461919 [details] [diff] [review]
mingw-w64 fix
Thank you sir!
Attachment #461919 -
Flags: review?(lw) → review+
Comment 13•15 years ago
|
||
Thanks for the review, pushed to TM:
http://hg.mozilla.org/tracemonkey/rev/b8d51faf7ee5
Comment 14•15 years ago
|
||
Shouldn't you have changed the comment too?
Comment 15•15 years ago
|
||
Probably yes, but most people don't care about mingw, so I didn't want to make comments clean.
Comment 16•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•