Closed Bug 396512 Opened 17 years ago Closed 13 years ago

Provide a way to get disassembly output


(Core :: JavaScript Engine, enhancement, P1)






(Reporter: jruderman, Assigned: Waldo)



(Whiteboard: fixed-in-tracemonkey)


(1 file, 7 obsolete files)

js> void dis(function(){}) 

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

Blocks: 429239
Attached patch WIP (obsolete) — Splinter Review
This is a work-in-progress that seems to work, but hasn't been tested much at all.
Assignee: general → crowder
Attachment #319477 - Attachment is obsolete: true
The patch breaks opt builds because it defines Sprinter ifdef DEBUG and uses it in SprintEnsureBuffer, which is not ifdef DEBUG.
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 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.
Attachment #319484 - Flags: review?(igor)
Attachment #319484 - Flags: review?(igor) → review+
Attached patch another rev (obsolete) — Splinter Review
This breaks less stuff, build-wise
Attachment #319484 - Attachment is obsolete: true
Jesse:  Let's do comment #5 as another bug, please.
Ok, filed bug 432721.
This patch doesn't apply any more :(  I had some evil ideas.
Attached patch updated to trunk (obsolete) — Splinter Review
Jesse:  Bang on this for a while and if it works for you, I'll request review again.
Attachment #319800 - Attachment is obsolete: true
Better now?
Attachment #359373 - Attachment is obsolete: true
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.
Attachment #359394 - Flags: review?(mrbkap)
Last hunk of jstracer.cpp fails now :(
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
> 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);
>     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.
Attachment #359394 - Flags: review?(mrbkap) → review-
Can someone else drive this?  I'm busy with mobile stuff at the moment.
Assignee: crowder → general
Assignee: general → jwalden+bmo
Thanks for taking, Waldo!
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).
Attachment #359394 - Attachment is obsolete: true
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.

We can fix 'em; dis is not an API we must preserve at all costs.
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.

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

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).
s/kind of reflect/kind of reflection/ sorry.  And sorry for the bug-spam.
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).

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.

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

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.
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.
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Summary: dis() should return the disassembly instead of printing it → Provide a way to get disassembly output
Attached patch Updated patchSplinter Review
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.
Attachment #395728 - Attachment is obsolete: true
Attachment #515941 - Flags: review?(mrbkap)
(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. :-|

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.
"<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?

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!
Attachment #515941 - Flags: review?(mrbkap) → review+
Whiteboard: fixed-in-tracemonkey
Depends on: 646574
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.