Assert correct type in JSVAL_TO_* macros

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: Waldo)

Tracking

({verified1.9.1})

Trunk
verified1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
Applications shouldn't call JSVAL_TO_STRING(v) unless they're sure v is actually a string.

Internally, we sometimes cheat.  The assertions could be carefully crafted to allow certain flavors of cheating, or we could define new macros for those patterns.  For example, String_p_match arguably wants a OBJECT_OR_BOOLEAN_JSVAL_TO_TESTABLE_POINTER(v) macro.
How much of a slowdown will this inflict on debug builds, e.g. for trace-test.js?

/be
(Assignee)

Comment 2

10 years ago
I don't know how much slower it would be, but it would make type errors fail-fast rather than potentially surfacing far from where the error actually occurred.

Unfortunately, I was going to implement this and then do a speed comparison, but I'm not sure it's even possible to implement this, because then you can't use the macros when initializing consts (or doing static assertions, but that be worked around with runtime assertions).  They'd also have to be converted to inline functions in order to avoid double-evaluating arguments, which further complicates things.  As far as I can figure, this bug is unfortunately CANTFIX and therefore WONTFIX.  Is anyone else clever enough to figure out a way around this problem?

Comment 3

10 years ago
(In reply to comment #2)
> They'd also have to be converted to
> inline functions in order to avoid double-evaluating arguments, which further
> complicates things.

Why converting the macros into always inline functions for a debug build would complicate things? I.e. what is wrong with

#ifdef DEBUG
#undef JSVAL_TO_STRING
#define JSVAL_TO_STRING(v) JS_CastValueToString(v)

static JS_ALWAYS_INLINE JSString *
JS_CastValueToString(jsval v)
{
    JS_ASSERT(JSVAL_IS_STRING(v));
    return (JSString *) JS_CLEAR_TAG(v);
}

#endif
(Assignee)

Comment 4

10 years ago
It's mostly just the same hassle of no longer being able to use any of the "macros" in compile-time const expressions, but the big thing I was noticing is that it does force explicit types into the code sometimes.  Consider:

#define JSVAL_NULL OBJECT_TO_JSVAL(0)

Non-engine code which has been playing fast and loose with types (particularly, say, void* pointers) will be hit when this happens, I think, and might no longer compile with the inline-function change.  Should we care?  Maybe.
Jeff, the types involved there don't change (since JSVAL_TO_OBJECT casts) and IMO such type system abuses aren't supported.
(Assignee)

Comment 6

10 years ago
Created attachment 347932 [details] [diff] [review]
Patch, round 1

This adds unwanted UNSAFE_JSVAL_TO_BOOLEAN and UNSAFE_BOOLEAN_TO_JSVAL macros to jsapi.h, but I don't see a way around it unless we want to just define magical constants (and duplicate the macros inside the safe inline-function versions of the macros), move the macros to jsbool.h as their most apt home, and do the asserts at runtime.  Most of this is just casting away type errors, and the rest is making sure to use the unsafe versions in cases where we can and wish to accept pseudobooleans.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #347932 - Flags: review?(brendan)
(Reporter)

Comment 7

10 years ago
+     cond = UNSAFE_JSVAL_TO_BOOLEAN(v) == PR_TRUE;

I'd rather see JS_TRUE here.

-    stack(0, INS_CONST(JSVAL_TO_BOOLEAN(JSVAL_HOLE)));
+    stack(0, INS_CONST(UNSAFE_JSVAL_TO_BOOLEAN(JSVAL_VOID)));

We definitely want HOLE here, not VOID.

Generally I think the UNSAFE_ functions should instead be called JSVAL_TO_PSEUDOBOOLEAN and vice versa.  That seems to reflect how they're used better than UNSAFE.

If we can come up with a better name than pseudo-boolean for these special values, so much the better.
Same generated optimized code (check codesize)? Better? I hope not worse.

/be
(Assignee)

Comment 9

10 years ago
Perf difference between a debug build with the patch and a debug build without, on a system that I didn't make a particularly rigorous effort to make sure wasn't under load (dormant browser, terminal, IRC, etc.):

find-waldo-now:~/moz/js-tm jwalden$ time tm-tests
-#- Executing 2485 test(s).
-#- Wrote failures to 'results-2008-11-13-155731-smdebug-failures.txt'.
-#- Wrote results to 'results-2008-11-13-155731-smdebug.html'.

real    6m59.647s
user    5m48.650s
sys     1m12.388s
find-waldo-now:~/moz/js-tm jwalden$ time tm-tests
-#- Executing 2485 test(s).
-#- Wrote failures to 'results-2008-11-13-162721-smdebug-failures.txt'.
-#- Wrote results to 'results-2008-11-13-162721-smdebug.html'.

real    6m39.250s
user    5m35.322s
sys     1m16.283s

So some impact, but ~5% or so; enough to not care?  I tend to think so.

Still need to run opt tests...

I have no idea how the HOLE->VOID change happened, ditto for the PR_TRUE malapropism.
(Assignee)

Comment 10

10 years ago
Bad news, good news...

First bad news is with JS_INLINE, the size of dist/bin/js increases by 116 bytes with this patch.  Second bad news is changing that to JS_ALWAYS_INLINE spews warnings about the various inline functions not always being used.  Good news is that changing JS_ALWAYS_INLINE to expand to __attribute__((always_inline)) JS_INLINE restores the original size of dist/bin/js, so with that change this patch is no net change.  Odd that gcc doesn't always respect JS_ALWAYS_INLINE...

I'm still trying to write a test that would have caught the s/JSVAL_HOLE/JSVAL_VOID/, which should be the only thing holding up a revised patch.

Comment 11

10 years ago
(In reply to comment #10)
> Odd that gcc
> doesn't always respect JS_ALWAYS_INLINE...

From js/src/jstypes.h:

#ifndef JS_ALWAYS_INLINE
# if defined DEBUG
#  define JS_ALWAYS_INLINE   JS_INLINE
# elif defined _MSC_VER
#  define JS_ALWAYS_INLINE   __forceinline
# elif defined __GNUC__
#  define JS_ALWAYS_INLINE   __attribute__((always_inline))
# else
#  define JS_ALWAYS_INLINE   JS_INLINE
# endif
#endif

My reasoning behind that JS_ALWAYS_INLINE -> JS_INLINE under DEBUG was to allow to set brekpoints inside always-inline functions. But so far that was not of a particular use. So I suggest to drop that.

(In reply to comment #7)
> Generally I think the UNSAFE_ functions should instead be called
> JSVAL_TO_PSEUDOBOOLEAN and vice versa.  That seems to reflect how they're used
> better than UNSAFE.

I second that.
Some new macros do not parenthesize their parameters in their bodies, always bad business. Please fix.

We haven't invented a pseudoboolean compound word, so please hyphenate: pseudo-boolean. This means an _: PSEUDO_BOOLEAN. Same goes for comments in the patch that say "pseudoboolean".

UNSAFE might scare kids off of an attractive nuisance, but too many don't read or actually take it as a dare.

May as well be concrete about pseudo-booleans being boolean-tagged, even though it is public API. We do not want to attract people to make up their own pseudo-booleans, of course. The assertions should catch any such flowing into API entry points that take jsval parameters -- right?

/be
(Assignee)

Comment 13

10 years ago
Created attachment 348235 [details] [diff] [review]
Better, faster, stronger

Okay, adopted the PSEUDO names, added a test for the typo that would have affected correctness and verified it fails with the typo in place, and modified JS_ALWAYS_INLINE to include JS_INLINE when using gcc in non-debug builds, where a same-size js shell is created with and without the patch, opt.  (icpc apparently defines __GNUC__ and thus will get this logic correct as well, if it happened to have a similar bug.)

I also found a *second* typo in TraceRecorder::elem, see original patch, and addressed that too.  I don't quite understand the details of what the last part of that method is doing, so at least for now I can't add a test for that typo.
Attachment #347932 - Attachment is obsolete: true
Attachment #348235 - Flags: review?(brendan)
Attachment #347932 - Flags: review?(brendan)
(Assignee)

Comment 14

10 years ago
Created attachment 348311 [details] [diff] [review]
With pseudo-boolean nit picked and a semi-detailed explanatory comment
Attachment #348235 - Attachment is obsolete: true
Attachment #348311 - Flags: review?(brendan)
Attachment #348235 - Flags: review?(brendan)

Updated

10 years ago
Severity: normal → enhancement
Is this the first patch to force jsapi.h clients to use C++ instead of C?

/be
(Assignee)

Comment 16

10 years ago
I don't see anything that requires use of C++ over C with jsapi.h.  Inline functions are C99 and not C89, but I think you'd have a hard time coming up with a production compiler, even one that doesn't support C99 well, that doesn't support inline.  I don't know the world of esoteric C compilers well (and do we really care about esoteric compilers?), but even TinyCC will at the very least ignore the keyword and not break compilation.

Comment 17

10 years ago
(In reply to comment #16)
> I don't see anything that requires use of C++ over C with jsapi.h.  Inline
> functions are C99 and not C89, but I think you'd have a hard time coming up
> with a production compiler, even one that doesn't support C99 well, that
> doesn't support inline.

Besides the patch uses static JS_ALWAYS_INLINE which expands to just static if inline is really not avaibale. This is fully C89 compatible.
(Assignee)

Comment 18

10 years ago
Created attachment 360110 [details] [diff] [review]
Unbitrotted

I note this would have prevented bug 475593 had it either been reviewed and committed or had I been keeping this patch up-to-date when I added that bug.
Attachment #348311 - Attachment is obsolete: true
Attachment #360110 - Flags: review?(brendan)
Attachment #348311 - Flags: review?(brendan)
Comment on attachment 360110 [details] [diff] [review]
Unbitrotted

>+    jsval funval = OBJECT_TO_JSVAL(static_cast<JSObject *>(aHandler));

Why static_cast here but reinterpret_cast much later, at the end of the patch, for similar code?

>+static JS_ALWAYS_INLINE jsval
>+OBJECT_TO_JSVAL(JSObject *obj)
>+{
>+    JS_ASSERT(((jsval) obj & JSVAL_TAGBITS) == JSVAL_OBJECT);
>+    return (jsval) obj;

I see

jstracer.cpp:JS_STATIC_ASSERT(JSVAL_OBJECT == 0);

which is critical here -- comment in the static inline function and move the static assertion to jsapi.cpp.

>+/*
>+ * A pseudo-boolean is a value which is not equal to JS_TRUE or JS_FALSE which
>+ * can be converted to a distinct jsval using the same process used to convert
>+ * booleans to jsvals.  JSVAL_VOID happens to be encoded in a pseudo-boolean,
>+ * but embedders MUST NOT rely on this.  All other possible pseudo-boolean
>+ * values are implementation-reserved and MUST NOT be used in any embedding of
>+ * SpiderMonkey.  Their use generally will trigger assertions in debug builds
>+ * and undefined behavior in optimized builds.  These macros are used
>+ * internally in SpiderMonkey, and they are defined here only to allow sharing
>+ * the conversion process used in each with the JSVAL_TO_BOOLEAN and
>+ * BOOLEAN_TO_JSVAL inline functions.

This over-specifies (we want decimal to be pseudo-boolean but it will not be "converted to a distinct jsval using the same process used to convert booleans to jsvals". Here's a suggested replacement comment:

/*
 * A pseudo-boolean is a 29-bit (for 32-bit jsval) or 61-bit (for 64-bit jsval)
 * value other than 0 or 1 encoded as a jsval whose tag is JSVAL_BOOLEAN.
 *
 * JSVAL_VOID happens to be defined as a jsval encoding a pseudo-boolean, but
 * embedders MUST NOT rely on this. All other possible pseudo-boolean values
 * are implementation-reserved and MUST NOT be constructed by any embedding of
 * SpiderMonkey.
 */

/be

Comment 20

10 years ago
(In reply to comment #19)
> (From update of attachment 360110 [details] [diff] [review])
> >+    jsval funval = OBJECT_TO_JSVAL(static_cast<JSObject *>(aHandler));
> 
> Why static_cast here but reinterpret_cast much later, at the end of the patch,
> for similar code?

Which also raises the question of using C-casts and not reinterpret_cast in the rest of the patch. So should the new C++ code in SM sources use C++ casts?
(Assignee)

Comment 21

10 years ago
Created attachment 360137 [details] [diff] [review]
With better comment

(In reply to comment #19)
> (From update of attachment 360110 [details] [diff] [review])
> >+    jsval funval = OBJECT_TO_JSVAL(static_cast<JSObject *>(aHandler));
> 
> Why static_cast here but reinterpret_cast much later, at the end of the patch,
> for similar code?

The latter is a function pointer cast, so static_cast doesn't work because it's only upward/downward, not "crossways" (even tho technically function-to-object-pointer casts aren't really legal regardless); I generally use static_cast when I can unless another type of cast is actually necessary.  I switched to reinterpret_cast here, but it's not really necessary (and I'm not entirely sure it's the best style here).


> This over-specifies (we want decimal to be pseudo-boolean but it will not be
> "converted to a distinct jsval using the same process used to convert booleans
> to jsvals". Here's a suggested replacement comment:

Nice, I like it.

(In reply to comment #20)
> Which also raises the question of using C-casts and not reinterpret_cast in
> the rest of the patch. So should the new C++ code in SM sources use C++ casts?

The C++ cast uses aren't SM, and Mozilla style is generally C++ casts instead of C casts.
Attachment #360110 - Attachment is obsolete: true
Attachment #360137 - Flags: review?(brendan)
Attachment #360110 - Flags: review?(brendan)

Updated

10 years ago
Attachment #360137 - Flags: review?(brendan) → review+
Comment on attachment 360137 [details] [diff] [review]
With better comment

>+    jsval funval = OBJECT_TO_JSVAL(reinterpret_cast<JSObject *>(aHandler));

Sorry, forgot the context here -- void* aHandler means static_cast is better, I agree. Your call.

>+static JS_ALWAYS_INLINE jsval
>+OBJECT_TO_JSVAL(JSObject *obj)
>+{
>+    /* Necessary for object (un)boxing in traced code to be a no-op. */
>+    JS_STATIC_ASSERT(JSVAL_OBJECT == 0);

This may break Sun compilers. Safest course is to put static asserts at top level in the relevant .cpp file.

Also, the comment is too narrow and implementation-oriented. The requirement that JSVAL_OBJECT == 0 lies in the very next non-DEBUG line (the return):

>+
>+    JS_ASSERT(((jsval) obj & JSVAL_TAGBITS) == JSVAL_OBJECT);
>+    return (jsval) obj;

r=me with these addressed.

/be
(Assignee)

Comment 23

10 years ago
In TM:

http://hg.mozilla.org/tracemonkey/rev/bee9d9de99b1
Flags: wanted1.9.1?
Whiteboard: fixed-in-tracemonkey

Comment 24

10 years ago
http://hg.mozilla.org/mozilla-central/rev/bee9d9de99b1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 478390

Comment 25

10 years ago
http://hg.mozilla.org/mozilla-central/rev/e9226dd6c073
Flags: in-testsuite+
Flags: in-litmus-

Comment 26

10 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/97649b4de036
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1

Comment 27

9 years ago
js1_8_1/trace/trace-test.js
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
(In reply to comment #21)
> Created an attachment (id=360137) [details]
> With better comment
> 
> (In reply to comment #19)
> > (From update of attachment 360110 [details] [diff] [review] [details])
> > >+    jsval funval = OBJECT_TO_JSVAL(static_cast<JSObject *>(aHandler));
> > 
> > Why static_cast here but reinterpret_cast much later, at the end of the patch,
> > for similar code?
> 
> The latter is a function pointer cast, so static_cast doesn't work because it's
> only upward/downward, not "crossways" (even tho technically
> function-to-object-pointer casts aren't really legal regardless); I generally
> (...)

Just for the record: I tried to compile FF trunk recently and it seems that gcc 3.4.6 doesn't like this code:

/home/users/fox/src/js/src/xpconnect/src/xpcquickstubs.cpp: In function `JSBool LookupGetterOrSetter(JSContext*, JSBool, jsval*)':
/home/users/fox/src/js/src/xpconnect/src/xpcquickstubs.cpp:258: error: ISO C++ forbids casting between pointer-to-function and pointer-to-object
/home/users/fox/src/js/src/xpconnect/src/xpcquickstubs.cpp:267: error: ISO C++ forbids casting between pointer-to-function and pointer-to-object

The relevant lines are:

   258              JS_SET_RVAL(cx, vp,
   259                          OBJECT_TO_JSVAL(reinterpret_cast<JSObject *>(getter)));
...
   267              JS_SET_RVAL(cx, vp,
   268                          OBJECT_TO_JSVAL(reinterpret_cast<JSObject *>(setter)));
(Reporter)

Comment 29

9 years ago
These need JS_FUNC_TO_DATA_PTR.
You need to log in before you can comment on or make changes to this bug.