The default bug view has changed. See this FAQ.

cx-less js::PCToLineNumber

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Implement a js_MapPCToLineNumber that does not require a JSContext, but only needs the data from a script needed to perform the mapping
(Assignee)

Comment 1

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

Updated

5 years ago
Blocks: 698580
(Assignee)

Comment 2

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

Updated

5 years ago
Attachment #589715 - Flags: review?(luke)
(Assignee)

Comment 3

5 years ago
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 4

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

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

Comment 6

5 years ago
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.)
Attachment #594393 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #589715 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #589730 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Version: 12 Branch → Trunk
(Assignee)

Updated

5 years ago
Assignee: general → sphink
Summary: js_MapPCToLineNumber → cx-less js::PCToLineNumber

Comment 7

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

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e48c19363a
Target Milestone: --- → mozilla12
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3328813a98 - something in that push burned.
Target Milestone: mozilla12 → ---
(Assignee)

Comment 10

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.