Compile fails using JS_USE_JSVAL_JSID_STRUCT_TYPES due to jsval.h macros

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Andrew Paprocki, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
When JS_USE_JSVAL_JSID_STRUCT_TYPES is defined, the JSVAL_BITS() and JSID_BITS() macros do not wrap the parameter in parens, so it can fail if a jsval pointer is deferenced:

jsval *vp = comes_from_somewhere;
jsval_layout l;
l.asBits = JSVAL_BITS(*vp);

That evaluates to:

l.asBits = (*vp.asBits);

... which fails to compile. Simply adding parens to the macros fixes it. I see someone must have already hit this before because the macros that are defined when in the non-JS_USE_JSVAL_JSID_STRUCT_TYPES case already have the parens.
(Reporter)

Comment 1

7 years ago
Created attachment 496869 [details] [diff] [review]
Patch to add parens

Comment 2

7 years ago
Comment on attachment 496869 [details] [diff] [review]
Patch to add parens

Thanks!
Attachment #496869 - Flags: review+

Comment 3

7 years ago
If you don't mind, I'll roll this into my next commit.
(Reporter)

Comment 4

7 years ago
Yeah no prob, this is so small.. just as long as it gets in I don't have to worry about it being patched in my version. :)
(Reporter)

Comment 5

7 years ago
Created attachment 496883 [details] [diff] [review]
Patch to add parens and operator<

The use of std::set<jsval> also fails to compile because operator< is missing. This adds operator<.
Attachment #496869 - Attachment is obsolete: true

Comment 6

7 years ago
Hmm, I'm not sure if we want to add an ordering relation to the public API.  It seems like your embedding could define operator< in terms of JSVAL_BITS.
(Reporter)

Comment 7

7 years ago
This is just to make the debug build work correctly. The release build is already naturally ordered because std::set<jsval> will simply use std::less<jsval> which is the same as std::less<unsigned long long>, providing JSVAL_BITS ordering. The debug build just flat out does not build because there is no operator< for the jsval_layout struct. Providing the operator makes the debug build have the same behavior as the release build.

Comment 8

7 years ago
Right.  The question isn't whether it works in release mode or not, but whether this is an official part of the public API vs. something that just happens to work.  For example, a lot of people used to cast back and forth between jsval and void* or size_t, and that happened to work until we changed value representation to always be 64-bits (and we may change it again to be 128-bit on 64-bit systems).  Likewise, an ordering operator may happen to work, but break if later the representation changes to, say, leave the payload word of JSVAL_VOID undefined.
(Reporter)

Comment 9

7 years ago
I was just going under the assumption that adding something to make the debug build compile does not necessary imply that it is part of the public API. Since the other operators are defined there when jsvals are structs, I assumed whoever added them felt the same way.

Comment 10

7 years ago
Fixed the parenthesization bug in
http://hg.mozilla.org/tracemonkey/rev/9aa8c290f633
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.