Provide a way to get disassembly output

RESOLVED FIXED

Status

()

enhancement
P1
normal
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: jruderman, Assigned: Waldo)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

12 years ago
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.
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

Updated

11 years ago
Blocks: 429239

Comment 2

11 years ago
Posted 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
Status: NEW → ASSIGNED

Comment 3

11 years ago
Attachment #319477 - Attachment is obsolete: true
(Reporter)

Comment 4

11 years ago
The patch breaks opt builds because it defines Sprinter ifdef DEBUG and uses it in SprintEnsureBuffer, which is not ifdef DEBUG.
(Reporter)

Comment 5

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

11 years ago
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)

Updated

11 years ago
Attachment #319484 - Flags: review?(igor) → review+

Comment 7

11 years ago
Posted patch another rev (obsolete) — Splinter Review
This breaks less stuff, build-wise
Attachment #319484 - Attachment is obsolete: true

Comment 8

11 years ago
Jesse:  Let's do comment #5 as another bug, please.
(Reporter)

Comment 9

11 years ago
Ok, filed bug 432721.
(Reporter)

Comment 10

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

Comment 11

10 years ago
Posted 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

Comment 12

10 years ago
Better now?
Attachment #359373 - Attachment is obsolete: true
(Reporter)

Comment 14

10 years ago
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.

Updated

10 years ago
Attachment #359394 - Flags: review?(mrbkap)
(Reporter)

Comment 15

10 years ago
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
>+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.
Attachment #359394 - Flags: review?(mrbkap) → review-

Comment 17

10 years ago
Can someone else drive this?  I'm busy with mobile stuff at the moment.
Assignee: crowder → general
(Assignee)

Updated

10 years ago
Assignee: general → jwalden+bmo

Comment 18

10 years ago
Thanks for taking, Waldo!
(Assignee)

Comment 19

10 years ago
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.

/be
(Assignee)

Comment 21

10 years ago
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.

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

/be

Comment 24

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

10 years ago
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).

/be
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
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.
(Assignee)

Comment 29

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

Comment 30

8 years ago
Posted 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. :-|

/be
(Assignee)

Comment 32

8 years ago
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?

/be

Comment 34

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

Comment 35

8 years ago
http://hg.mozilla.org/tracemonkey/rev/cc4fdccc1135
Whiteboard: fixed-in-tracemonkey
(Reporter)

Updated

8 years ago
Depends on: 646574
http://hg.mozilla.org/mozilla-central/rev/cc4fdccc1135
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.