Bad js_Emit1(...JSOP_TRACE) return value tests

RESOLVED FIXED in mozilla1.9.2

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({regression})

Trunk
mozilla1.9.2
regression
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta4-fixed, blocking1.9.1 -, status1.9.1 wontfix)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Created attachment 406891 [details] [diff] [review]
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
Flags: blocking1.9.2?
Attachment #406891 - Flags: review?(dvander)
Attachment #406891 - Attachment is patch: true
Attachment #406891 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #406891 - Flags: review?(dvander) → review+
(Assignee)

Comment 2

9 years ago
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
(Assignee)

Comment 3

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

"count on the names", of course.

/be
(Assignee)

Comment 4

9 years ago
http://hg.mozilla.org/tracemonkey/rev/470007524805

/be
Whiteboard: fixed-in-tracemonkey

Updated

9 years ago
blocking1.9.1: --- → ?
How could QA verify this patch? Are there user-visible effects or js snippets that go wrong without it, or better, a testcase?
status1.9.1: --- → wanted
Keywords: regression
(Assignee)

Comment 6

9 years ago
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
(Assignee)

Comment 7

9 years ago
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

9 years ago
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?
We'll look at this for branches when it gets on the active 1.9.2 branch.
blocking1.9.1: ? → needed

Comment 10

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

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2+
Sayre: is this bug really "needed" for 3.5.x or can we drop the blocking marker?
blocking1.9.1: needed → -
status1.9.1: wanted → wontfix
You need to log in before you can comment on or make changes to this bug.