618386 - Compile fails using JS_USE_JSVAL_JSID_STRUCT_TYPES due to jsval.h macros

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Paprocki]

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.

Comment 1

[Andrew Paprocki]

Attachment: Patch to add parens

Comment 2

[Luke Wagner [:luke]]

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

Thanks!

Comment 3

[Luke Wagner [:luke]]

If you don't mind, I'll roll this into my next commit.

Comment 4

[Andrew Paprocki]

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

Comment 5

[Andrew Paprocki]

Attachment: Patch to add parens and operator<

The use of std::set<jsval> also fails to compile because operator< is missing. This adds operator<.

Comment 6

[Luke Wagner [:luke]]

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.

Comment 7

[Andrew Paprocki]

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

[Luke Wagner [:luke]]

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.

Comment 9

[Andrew Paprocki]

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

[Luke Wagner [:luke]]

Fixed the parenthesization bug in
http://hg.mozilla.org/tracemonkey/rev/9aa8c290f633