777474 - GetPCCountScriptContents shouldn't use the decompiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

It seems like the ultimate solution would follow from bug 568142; we could just extract the expressions from the source coordinates.

My only other idea was to use the new expression decompiler to produce expression text for opcodes. This is not as good because the expressions couldn't necessarily be matched up with the source.

Comment 1

[:Benjamin Peterson]

Bug 568142 as it stands now won't work because it only updates column information on new statements.

Comment 2

[Brian Hackett [Laid off!]]

A third option, and I believe the best one, is to allow reparsing of scripts that have already been emitted.  If the parser is deterministic (all options affecting the parse tree / code emitting are controlled for), this would give a parse tree identical to the original one with full column/line information, and then emitting could be replayed to see the source location for each opcode.

This would also give us column information for error reporting, at a much finer level of detail than bug 568142.

Comment 3

[:Benjamin Peterson]

I thought about this, too. You'd need to reparse the entire compilation unit, right, though?

Comment 4

[Brian Hackett [Laid off!]]

Bug 678037 should allow parsing functions individually after the fact, as the goal there is to not generate bytecode at all until the script first runs.

Comment 5

[Shu-yu Guo [:shu]]

What I'd like to do is, now that we save the original source of functions, is to merge the PC count info (+ other JIT info that's tied to program text locations rather than execution) into the Reflect API.

With the Reflect API it's not necessary to get column/line info per se, since all we really need is pn_offset from the ParseNode to be able to index the opcode and get its PC count/whatever other JIT info we need.

I don't fully understand bhackett's wariness of parsing/code emitting being nondeterministic. Luke, care to clarify on possible non-determinism in the frontend?

Comment 6

[Luke Wagner [:luke]]

Sometimes the parser picks up ambient state to determine which bytecodes to emit; one example is the permanent properties of the global object (determines JSOP_GETGNAME vs. JSOP_NAME); I wouldn't be surprised if there weren't 4 more cases.

Instead of trying to pretty-print, why can't we just list the opcodes and their types directly?  If this is for internal moz devs, that should be fine.  I can't imagine the current jit inspector output being used by non-SM devs and if we wanted it to be, we'd want to integrate it with devtools.  In that case, devtools already has a source viewer, so I think we'd just want to map pc -> line/col (using the existing debugger machinery) and (somehow) communicate this to devtools.  Does that make sense jimb?

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #6)
> Sometimes the parser picks up ambient state to determine which bytecodes to
> emit; one example is the permanent properties of the global object
> (determines JSOP_GETGNAME vs. JSOP_NAME); I wouldn't be surprised if there
> weren't 4 more cases.
> 

I see, thanks.

> Instead of trying to pretty-print, why can't we just list the opcodes and
> their types directly?  If this is for internal moz devs, that should be
> fine.  I can't imagine the current jit inspector output being used by non-SM
> devs and if we wanted it to be, we'd want to integrate it with devtools.  In
> that case, devtools already has a source viewer, so I think we'd just want
> to map pc -> line/col (using the existing debugger machinery) and (somehow)
> communicate this to devtools.  Does that make sense jimb?

Of course the goal is to integrate with devtools. AFAIK the debugger bytecode offset <-> line mapping isn't fine grained enough for the TI info in PC counts, and benjamin above mentioned that column info is only recorded at a statement level instead of an expression level.

In any case if you're suggesting to expose this via Debugger.Script, I have no opinions now if that makes more sense than exposing this as a different API that gives you more fine grained source <-> bytecode mapping and lets you ask questions of AST nodes/bytecode.

I'll talk to robcee.

Comment 8

[:Benjamin Peterson]

Attachment: don't include opcode text

So, given bug 718969 comment 9, we can do something like this now?

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 708835 [details] [diff] [review]
don't include opcode text

Review of attachment 708835 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ -7119,5 @@
> -        }
> -
> -        const char *text = decompiledOpcodes[offset].text;
> -        if (text && *text != 0) {
> -            AppendJSONProperty(buf, "text");

Can you keep this "text" JSON property, and just give it whatever is produced by the expression decompiler, for those opcodes where the expression decompiler works?  It'd be nice to have an "x.f" text to show in the UI, rather than a plain "getprop".

Comment 10

[:Benjamin Peterson]

Attachment: try to use text from expression decompiler

Like so?

Comment 11

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/050a0a27024a

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a62ffd085b9d to clear a path to back out de-e4x.

Comment 13

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e06607d8fe

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8e06607d8fe