522920 - Bad js_Emit1(...JSOP_TRACE) return value tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Attachment: fix

This went in with the JSOP_LOOP patch (bug 479109), and I failed to catch it when reviewing the renaming and extension to JSOP_TRACE (bug 515806). This fix should go where the patch for bug 479109 went (1.9.1, it seems -- how do I nominate?).

Clearly old API design flaw, ptrdiff_t with -1 for failure mixed with JSBool. But I will defer fixing that for now.

/be

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 406891 [details] [diff] [review]
fix

I remember duplicating the wrong check when adding JSOP_TRACE to the start of scripts. But of course that offset is usually 0, so everything broke.

For some reason I didn't think to correct the other instances.

Comment 2

[Brendan Eich [:brendan]]

David, did you have a reason to emit JSOP_TRACE in the prolog of global and eval scripts but at the front of the main entry point (after an JSOP_DEF* prolog ops) in functions? Ops such as JSOP_DEFFUN return ARECORD_STOP, so that could be the issue. But it would bite top-level scripts too, where JSOP_TRACE is emitted first thing (front of prolog).

The prolog processes function definitions and variable declarations. We could trace those ops but we have avoided doing the work, since once defined in the variable object we can count the names, either because of the global shape being invariant, or because we have a shaped Call object, or else because an analysis that optimized (JSOP_DEFLOCALFUN, e.g.) let us pin names to stack slots.

All things equal, it would be better to put JSOP_TRACE in a consistent position, either front of entire script (first in prolog) or front of main.

/be

Comment 3

[Brendan Eich [:brendan]]

> variable object we can count the names, either because of the global shape

"count on the names", of course.

/be

Comment 4

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/470007524805

/be

Comment 5

[Daniel Veditz [:dveditz]]

How could QA verify this patch? Are there user-visible effects or js snippets that go wrong without it, or better, a testcase?

Comment 6

[Brendan Eich [:brendan]]

You'd have to inject malloc failure at just the right point. We don't have that kind of test yet; possibly via jsapi-test. Cc'ing jorendorff.

/be

Comment 7

[Brendan Eich [:brendan]]

For this bug, inspection of the patch is enough. For the future, injection of malloc failure would have to hit every possible js_Emit* call.

Better to fix the API to avoid the confusion of -1 (failure) or nonnegative ptrdiff_t (success) for a boolean (C and C++ can't help here; C++ could but at some cost, IIRC -- jimb did something similar in jstracer.h but #ifdef'ed to avoid the overhead for product builds).

/be

Comment 8

[Jim Blandy :jimb]

Brendan is referring here to the stuff selected by DEBUG_RECORDING_STATUS_NOT_BOOL in js/src/jstracer.h.  The idea was to replace the enum values, which the compiler will silently coerce to bool, with class instances that mostly behave like the enum values but whose instances cannot be converted to bool.

I'm not sure how well this trick could be adapted to the js_Emit* return values.  Is it possible to define a type which can be converted to int, but not bool?

Comment 9

[Daniel Veditz [:dveditz]]

We'll look at this for branches when it gets on the active 1.9.2 branch.

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/470007524805

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7de23f96b455

Comment 12

[Daniel Veditz [:dveditz]]

Sayre: is this bug really "needed" for 3.5.x or can we drop the blocking marker?