396512 - Provide a way to get disassembly output

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Jesse Ruderman]

js> void dis(function(){}) 
flags: LAMBDA INTERPRETED
...

This makes it annoying to fuzz dis() for crashes, because it adds a lot of output.  I think dis() should return the disassembly instead of printing it.  For the normal use case of dis(), typing it at the top level in the REPL, this change won't affect the output.

Comment 1

[Brendan Eich [:brendan]]

Great idea (dis is very old, spring 1995). Someone please take this bug and have your way with the JS_FRIEND_API exports from jsopcode.c.

/be

Comment 2

[Brian Crowder]

Attachment: WIP

This is a work-in-progress that seems to work, but hasn't been tested much at all.

Comment 3

[Brian Crowder]

Attachment: fixed a couple of bugs found in testing

Comment 4

[Jesse Ruderman]

The patch breaks opt builds because it defines Sprinter ifdef DEBUG and uses it in SprintEnsureBuffer, which is not ifdef DEBUG.

Comment 5

[Jesse Ruderman]

Aww, non-DEBUG has no dis() at all.  Would it make sense to make a ifdefs for "supports dis()" and "has assertions", so I can make builds with dis() but no assertions?

Comment 6

[Brian Crowder]

Comment on attachment 319484 [details] [diff] [review]
fixed a couple of bugs found in testing

Igor:  This is low-priority, but helpful for fuzzing and NPOTB.

Comment 7

[Brian Crowder]

Attachment: another rev

This breaks less stuff, build-wise

Comment 8

[Brian Crowder]

Jesse:  Let's do comment #5 as another bug, please.

Comment 9

[Jesse Ruderman]

Ok, filed bug 432721.

Comment 10

[Jesse Ruderman]

This patch doesn't apply any more :(  I had some evil ideas.

Comment 11

[Brian Crowder]

Attachment: updated to trunk

Jesse:  Bang on this for a while and if it works for you, I'll request review again.

Comment 12

[Brian Crowder]

Attachment: update to trunk that actually builds in debug (duh).

Better now?

Comment 13

[Brian Crowder]

Attachment: update to trunk without (null) spewage, and a few other bugs fixed

Comment 14

[Jesse Ruderman]

This is good stuff.  I found a bunch of decompiler bugs and bytecode inefficiencies with it, by checking that a round trip through the decompiler doesn't change the bytecode.

This patch doesn't hurt sunspider speed in debug builds, with or without TRACEMONKEY=verbose.

Comment 15

[Jesse Ruderman]

Last hunk of jstracer.cpp fails now :(

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 359394 [details] [diff] [review]
update to trunk without (null) spewage, and a few other bugs fixed

>-js_Disassemble(JSContext *cx, JSScript *script, JSBool lines, FILE *fp)
>+js_Disassemble(JSContext *cx, JSScript *script, JSBool lines, Sprinter *sp)

It seems like it would minimize the changes needed for this patch if this function kept taking a |FILE *| and you made it call a helper that returned the source. That way, you could leave the callers alone and add a new JS_FRIEND_API() point for the shell, taking a |Sprinter *|.

>-static ptrdiff_t
>+ptrdiff_t
> SprintPut(Sprinter *sp, const char *s, size_t len)

Seems like this should be js_SprintPut now that it's no longer static. Same with the rest of the Sprint* functions.

>+typedef struct Sprinter {

Should this be JSSprinter now?

>@@ -4192,6 +4192,12 @@ TraceRecorder::monitorRecording(JSContex
> 
>     /* We check for imacro-calling bytecodes inside the switch cases to resolve
>        the "if" condition at the compile time. */
>+#ifdef DEBUG
>+    void *mark;
>+    Sprinter sprinter;
>+    mark = JS_ARENA_MARK(&cx->tempPool);
>+    INIT_SPRINTER(cx, &sprinter, &cx->tempPool, 0);
>+#endif
>     bool flag;
>     switch (op) {
>       default: goto abort_recording;

This jumps over the JS_ARENA_FREE below, as does the |goto| in the macro.

> static JSBool
> Notes(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>+    *rval = STRING_TO_JSVAL(JS_NewStringCopyZ(cx, sprinter.base));

Need to deal with OOM from JS_NewStringCopyZ here.

> Disassemble(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>+    *rval = STRING_TO_JSVAL(JS_NewStringCopyZ(cx, sprinter.base));

And here.

Comment 17

[Brian Crowder]

Can someone else drive this?  I'm busy with mobile stuff at the moment.

Comment 18

[Brian Crowder]

Thanks for taking, Waldo!

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Updated patch, notably atop bug 499049

This keeps sitting in my queue waiting for me to get to it, too many other things to do so far beyond periodic rebasing.  Still should probably get it out here just in case; it's basically the previous patch here just made to compile again, nothing more (including not having gotten to review comments here).

Comment 20

[Brendan Eich [:brendan]]

This breaks some stuff we do in files, which expect dis to print. The compatible way is diss or sdis ;-). Doesn't assume dis callers are all interactive.

/be

Comment 21

[Jeff Walden [:Waldo]]

We can fix 'em; dis is not an API we must preserve at all costs.

Comment 22

[Brendan Eich [:brendan]]

Who said anything about all costs? I'm suggesting that it's not worth "our" (mine not yours :-/) time to fix what was not broken. A command like dis, ~14 years old (15 really), is not something to change from printing to non-printing just because it seems like a good idea now.

/be

Comment 23

[Brendan Eich [:brendan]]

Alternative: give dis another option, it has "-r" already. "-p" has some precedent in cvs, e.g.

/be

Comment 24

[Brian Crowder]

It is a good idea now because it enables a kind of reflect not otherwise available to our debug tools, I didn't code the original patch on a whim or to waste your time.  Writing and maintaining an additional code-path for a dis() with an argument, or a different "sdis" function seems way worse (for your time, now and in the future).

Comment 25

[Brian Crowder]

s/kind of reflect/kind of reflection/ sorry.  And sorry for the bug-spam.

Comment 26

[Brendan Eich [:brendan]]

No one said "whim" here, either about dis as originally designed (it's modeled after the dis[assemble] built-in found in gdb and other debuggers -- gdb scripts can disassemble to stdout), or in changing it.

The bug summary's "should" or the intent and effect of the original patch does not prove anything unless you are arguing from authority. If you want an argument from authority, I can invoke originator authority.

But let's avoid authority and stick to use cases. :-)

Right now any copy/paste refactoring or correspondence principle applied to the shell says copying a dis that produces stdout to a file should not produce zero output from that dis, instead an ignored string return value. Breaking dis in this breaks not only breaks compatibility, it requires an extra print call not needed in interactive code. Disassemble-to-stdout is a valid use case.

Jesse's use-case of fuzzing dis output is valid too, but that doesn't mean we should violate the correspondence principle or break compatibility for users who face the first use-case. Fuzzers are few and written by experts who can afford to opt in explicitly with sdis(...) or dis("-s", ...).

(My thinko "-p" has the reverse sense, duh! so "-s" for string output).

/be

Comment 27

[Brendan Eich [:brendan]]

s/this breaks not only breaks compatibility/this way not only breaks compatibility

It's not bug-spam to argue about what's best, although it could go better in mail. But I do not think either use-case advocate should gore the other's ox, and opt-in for the later use-case is not onerous.

Moreover, we have precedent with dis('-r', ...).

I'm pretty sure we can reason together to an agreeable solution without too much back-and-forth.

/be

Comment 28

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 29

[Jeff Walden [:Waldo]]

I've been carrying this patch in my queue for almost two years now.  It's time to end this, once and for all: 5.0 or bust.

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Updated patch

With the shell printing source for results, having dis return a string gets a little ugly for easy use, so let's add a disassemble() function that returns a string.

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)
> Created attachment 515941 [details] [diff] [review]
> Updated patch
> 
> With the shell printing source for results, having dis return a string gets a
> little ugly for easy use,

That is true but it's not the only reason for a new name or an option. Ignoring comments (comment 27) is not good peering.

> so let's add a disassemble() function that returns a string.

Finally, the right outcome for any reason. :-|

/be

Comment 32

[Jeff Walden [:Waldo]]

I wasn't ignoring.  I was offering a different, and at this point I believe more compelling even to dissenters, reason for having a new name.

Comment 33

[Brendan Eich [:brendan]]

"<new reason>, so let's <add a new function>" as necessary and sufficient reason, I get it. But my reason was necessary and sufficient too, and continuing to use works like "dissent" implying some principled stand in favor of goring one use-case ox still stinks.

This is not a big deal, but it's also not a "my way or the highway" situation. Unless you insist on changing dis from how it has always behaved, breaking uses of it in non-interactive shell settings (.js files fed to the shell).

That we "disagree" like this is a problem. I don't have a solution. I suggest working on technical win-wins, like, say a new function as suggested way back in comment 20. Glad we're done but why was there so much arguing?

/be

Comment 34

[Brian Crowder]

Sorry for any part I may have had in the disagreement here, it's likely I misread or misinterpreted earlier comments, which on re-reading today seem totally reasonable!

Comment 35

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/cc4fdccc1135

Comment 36

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/cc4fdccc1135