719294 - cx-less js::PCToLineNumber

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

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:]]

Attachment: 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:]]

Attachment: 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:]]

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]]

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]]

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:]]

Attachment: 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]]

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:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e48c19363a

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3328813a98 - something in that push burned.

Comment 10

[Steve Fink [:sfink] [:s:]]

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] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/90f6626365e3