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
Attachment #406891 - Flags: review?(dvander)
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+
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
> variable object we can count the names, either because of the global shape "count on the names", of course. /be
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
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
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
status1.9.2: --- → final-fixed
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.