Last Comment Bug 719294 - cx-less js::PCToLineNumber
: cx-less js::PCToLineNumber
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on:
Blocks: 698580
  Show dependency treegraph
 
Reported: 2012-01-18 17:27 PST by Steve Fink [:sfink] [:s:]
Modified: 2012-03-01 06:32 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Eliminate the bytecode space optimization where it omits the starting line number (3.49 KB, patch)
2012-01-18 17:27 PST, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Splinter Review
Implement js_MapPCToLineNumber for when you don't have a JSScript, just the necessary fragments for doing the mapping (3.97 KB, patch)
2012-01-18 17:38 PST, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Splinter Review
Eliminate the bytecode space optimization where it omits the starting line number (11.84 KB, patch)
2012-02-03 22:16 PST, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2012-01-18 17:27:33 PST
Implement a js_MapPCToLineNumber that does not require a JSContext, but only needs the data from a script needed to perform the mapping
Comment 1 Steve Fink [:sfink] [:s:] 2012-01-18 17:27:36 PST
Created attachment 589715 [details] [diff] [review]
Eliminate the bytecode space optimization where it omits the starting line number

Eliminate the bytecode space optimization where it omits the starting line number as per this comment:

/*
 * Special case: function definition needs no line number note because
 * the function's script contains its starting line number.
 */

But this is the only reason why you need a JSContext in js_PCToLineNumber, and I have a user that needs to do the lookup without a context.
Comment 2 Steve Fink [:sfink] [:s:] 2012-01-18 17:38:05 PST
Created attachment 589730 [details] [diff] [review]
Implement js_MapPCToLineNumber for when you don't have a JSScript, just the necessary fragments for doing the mapping
Comment 3 Steve Fink [:sfink] [:s:] 2012-01-18 17:41:18 PST
Comment on attachment 589730 [details] [diff] [review]
Implement js_MapPCToLineNumber for when you don't have a JSScript, just the necessary fragments for doing the mapping

This operation is needed for doing JS backtraces that involve scripts that have been deleted since the backtrace was taken.
Comment 4 Luke Wagner [:luke] 2012-01-26 09:23:28 PST
Comment on attachment 589715 [details] [diff] [review]
Eliminate the bytecode space optimization where it omits the starting line number

>             JSOp op = fun->isFlatClosure() ? JSOP_DEFFUN_FC : JSOP_DEFFUN;
>+
>             EMIT_INDEX_OP(op, index);

No need to add the space: with C++, since the decls don't need to be hoisted, we don't use the \n to separate decls from uses.


>diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp
>+#ifdef DEBUG
>+    if (cx) {

This DEBUG block seems like a great thing to run through try server but, once you've done that, I think it would be nicer to remove it so that you can remove the 'cx' parameter altogether

>+     * walk through source notes accumulating their deltas, keeping track of

Capitalize Walk
Comment 5 Luke Wagner [:luke] 2012-01-26 09:26:35 PST
Comment on attachment 589730 [details] [diff] [review]
Implement js_MapPCToLineNumber for when you don't have a JSScript, just the necessary fragments for doing the mapping

Nice.  Instead of having the two names, begging the question "what is Map vs. non-Map", how about giving both functions the same name: the signatures speak for themselves as will a quick look at the definition of the JSScript-taking overload.  Lastly, in the interest of incremental-modernization, could you rename both to js::PCToLineNumber?
Comment 6 Steve Fink [:sfink] [:s:] 2012-02-03 22:16:15 PST
Created attachment 594393 [details] [diff] [review]
Eliminate the bytecode space optimization where it omits the starting line number

You suggested eliminating the cx parameter. Lemme tag you for re-review to be sure you meant it. (Public API does not change.)
Comment 7 Luke Wagner [:luke] 2012-02-06 09:50:11 PST
Comment on attachment 594393 [details] [diff] [review]
Eliminate the bytecode space optimization where it omits the starting line number

Mmm sweet
Comment 8 Steve Fink [:sfink] [:s:] 2012-02-28 17:26:33 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e48c19363a
Comment 9 Phil Ringnalda (:philor, back in August) 2012-02-28 19:12:47 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3328813a98 - something in that push burned.
Comment 10 Steve Fink [:sfink] [:s:] 2012-02-29 21:31:32 PST
Yes, it was this one that burned it. Turned out to be a bad merge. I ended up pushing code that looked like

  if (!operation1())
  if (!operation2())
    return false;
    return false;

which didn't do quite what I wanted. I'm not crazy about significant whitespace, but I wouldn't mind warnings based on whitespace...

Re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/90f6626365e3
Comment 11 Marco Bonardo [::mak] 2012-03-01 06:32:10 PST
https://hg.mozilla.org/mozilla-central/rev/90f6626365e3

Note You need to log in before you can comment on or make changes to this bug.