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)

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.
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.

Attachment

General

Created:
Updated:
Size: