Closed Bug 719294 Opened 8 years ago Closed 8 years ago

cx-less js::PCToLineNumber

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file, 2 obsolete files)

Implement a js_MapPCToLineNumber that does not require a JSContext, but only needs the data from a script needed to perform the mapping
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.
Blocks: 698580
Attachment #589715 - Flags: review?(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

This operation is needed for doing JS backtraces that involve scripts that have been deleted since the backtrace was taken.
Attachment #589730 - Flags: review?(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
Attachment #589715 - Flags: review?(luke) → review+
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?
Attachment #589730 - Flags: review?(luke) → review+
You suggested eliminating the cx parameter. Lemme tag you for re-review to be sure you meant it. (Public API does not change.)
Attachment #594393 - Flags: review?(luke)
Attachment #589715 - Attachment is obsolete: true
Attachment #589730 - Attachment is obsolete: true
Version: 12 Branch → Trunk
Assignee: general → sphink
Summary: js_MapPCToLineNumber → cx-less js::PCToLineNumber
Comment on attachment 594393 [details] [diff] [review]
Eliminate the bytecode space optimization where it omits the starting line number

Mmm sweet
Attachment #594393 - Flags: review?(luke) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3328813a98 - something in that push burned.
Target Milestone: mozilla12 → ---
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
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/90f6626365e3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.