Closed Bug 944164 Opened 10 years ago Closed 8 years ago

Make jorendb a little closer to a real debugger

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox39 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(6 files, 7 obsolete files)

1.29 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.63 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
17.15 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
15.02 KB, text/plain
Details
26.62 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
33.92 KB, patch
terrence
: review+
Details | Diff | Splinter Review
I've been using jorendb successfully to debug some JS shell-run code. Creating a bug to hang my patches off of.
I would love to see this happen.
Attached patch imported patch shell-stuff (obsolete) — Splinter Review
Shell redirection stuff
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Attached patch imported patch jdb-stuff (obsolete) — Splinter Review
debugger stuff.

Neither of these patches is quite ready for review. Probably. But they work. Or at least, they did before I did my last round of "cleanup". ;-)
Very confusing behavior when you try to quit out of the debugger and it reruns your script.
Attachment #8343312 - Flags: review?(jorendorff)
Used for my hacky command-line massaging in jorendb.js. I probably ought to rewrite it in some principled way.
Attachment #8343318 - Flags: review?(jorendorff)
yeah, I totally mangled the debugger in that latest version of the patch. This one works. Well, at least with all the other patches I'm piling on here.
Attachment #8343324 - Flags: review?(jorendorff)
Attachment #8339743 - Attachment is obsolete: true
Comment on attachment 8339742 [details] [diff] [review]
imported patch shell-stuff

This patch is awful, and I feel bad for requesting review instead of sitting down and thinking of how to do this right.
Attachment #8339742 - Flags: review?(jorendorff)
Attached file debug
This is the way I'm using the debugger. I put this script in my $PATH, then run

  debug --js obj/dist/bin/js /tmp/myscript.js

and it pops up an emacs window with jorendb running inside it.

I still need to figure out how to break before it runs the script, though. Right now I depend heavily on 'breakpoint;' statements.
Attachment #8343312 - Flags: review?(jorendorff) → review+
Attachment #8343318 - Flags: review?(jorendorff) → review+
Comment on attachment 8343324 [details] [diff] [review]
imported patch jdb-stuff

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

::: js/examples/jorendb.js
@@ +45,5 @@
>      debuggeeValues["$" + i] = dv;
>      print("$" + i + " = " + dvrepr);
> +    if (options.pretty && (typeof dv === 'object')) {
> +        debuggeeGlobalWrapper.evalInGlobalWithBindings("print(JSON.stringify($" + i + ", null, 4))", debuggeeValues);
> +    }

Does this print a wall of text if dv is the debuggee global? Can you truncate it?

@@ +173,5 @@
> +    var [ name, value ] = rest.split(' ', 2);
> +    if (value == '0')
> +        value = false;
> +    if (name == 'args')
> +        activeTask.scriptArgs = parseArgs(value);

I don't think this works as expected.

  js> 'a b c'.split(' ', 2)
  ["a", "b"]

Better use .shift(), I guess.

@@ +175,5 @@
> +        value = false;
> +    if (name == 'args')
> +        activeTask.scriptArgs = parseArgs(value);
> +    else
> +        options[name] = value;

Huh. This is pretty weird.

Since both existing options are booleans, consider

    var yes = ["1", "yes", "true", "on"];
    var no = ["0", "no", "false", "off"];

    if (yes.indexOf(value) !== -1)
        options[name] = true;
    else if (no.indexOf(value) !== -1)
        options[name] = false;
    else
        print("Set options to either true or false.");

@@ +202,5 @@
>      }
>  }
>  
> +function printCommand(rest) { return doPrint(rest, {plain:true}); }
> +function prettyprintCommand(rest) { return doPrint(rest, {pretty:true}) }

The style argument to doPrint isn't used. (And you probably mean {pretty: false} rather than {plain:true}.

@@ +399,5 @@
>          return repl();
>      }
>  
>      function stepStepped() {
> +        // print("stepStepped: " + this.fullDescription());

Please also comment out the similar print() call in stepEntered().

@@ +450,5 @@
>  function stepCommand() { return doStepOrNext({step:true}); }
>  function nextCommand() { return doStepOrNext({next:true}); }
> +function finishCommand() { return doStepOrNext({finish:true}); }
> +
> +// FIXME: DOES NOT WORK YET

Aww.

@@ +459,5 @@
> +        print("Unable to break at line " + where);
> +        return;
> +    }
> +    for (var offset of offsets) {
> +        script.setBreakpoint(offset, handleBreakpoint);

You need {hit: handleBreakpoint}, at least.

@@ +480,4 @@
>      nextCommand, "n",
>      printCommand, "p",
>      quitCommand, "q",
> +    runCommand, "R",

Why a capital R?

@@ +645,5 @@
> +        args.shift();
> +    } else if (!scriptSeen && (arg.indexOf("-") != 0)) {
> +        scriptSeen = true;
> +    } else {
> +        restArgs.push(arg);

Can we instead make it so that the first nonoption argument marks the end of arguments to jorendb, and any after that are scriptArgs, even if they look like options?
Attachment #8343324 - Flags: review?(jorendorff) → review+
Comment on attachment 8339742 [details] [diff] [review]
imported patch shell-stuff

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

Is this an emacs thing? I'm open to it, I just can't tell what I'm looking at here.

::: js/src/shell/js.cpp
@@ +3482,5 @@
>              return false;
> +    } else if (args[0].isNull()) {
> +        // This is still no good, because the debugger still interferes with program state.
> +        gPrevOutFile = gOutFile;
> +        gOutFile = gOrigOutFile;

I'm afraid I don't understand what's happening here.

@@ +3487,5 @@
> +    } else if (args[0].isUndefined()) {
> +        if (gPrevOutFile)
> +            gOutFile = gPrevOutFile;
> +        gPrevOutFile = NULL;
> +    }

And especially here; this ability to pass undefined doesn't seem to be used in the other patches.
Attachment #8339742 - Flags: review?(jorendorff)
Attached patch imported patch shell-stuff (obsolete) — Splinter Review
Hm, this patch isn't really ready for prime time. But updating anyway, since it sounds like avih has a use for it.
Attachment #8339742 - Attachment is obsolete: true
(In reply to Jason Orendorff [:jorendorff] from comment #10)
> Comment on attachment 8339742 [details] [diff] [review]
> imported patch shell-stuff
> 
> Review of attachment 8339742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this an emacs thing? I'm open to it, I just can't tell what I'm looking
> at here.

No, not emacs. And sorry, I realize that this wasn't really ready. I probably ought to talk to you about this, since I'm not sure about the right approach.

The problem I'm trying to solve is that I want to debug scripts that redirect their output to a file. That means I need the debugger (ie, jorendb) to be able to write stuff to stdout without preventing the script's output from going where it wanted it to go.

So one option would be for the debugger to query the current output destination, redirect the output to stdout so the debugger can talk to the user, and then redirect the output to the saved destination when it allowed the script to proceed. But what is that saved destination? It smells a lot like a File object, which is a slippery slope. Perhaps that *is* the best way, though.

My solution was to instead embed the ability to temporarily set an output destination, and then reset it. The js.cpp code remembers the previous destination. This patch implements a depth-1 saved stack. (I earlier used a Vector of previous destinations. I can't remember why I killed it. Too much complexity for a niche case for which I am the sole user, I guess.)

Anyway, the patch at the very least needs comments as to what's going on. Rereading it, I realize I did not intend to put it up for review yet. Sorry about that.
(In reply to Steve Fink [:sfink] from comment #12)
> The problem I'm trying to solve is that I want to debug scripts that
> redirect their output to a file.

Oh, interesting. Well, sharing the redirection state across all globals seems unfortunate.

The ocap approach would be to leave the debugger's global print() alone, but replace debuggeeGlobal.print. I think this is the way to go, even if it means having a File object or something.
Ok, so I created an opaque FileObject limited in use to just the redirect command. It returns the previous output destination, so the caller is responsible for restoring older destinations if desired. This, together with making jorendb wrap the debugging global's print/printErr/putstr, seems much nicer. But there are some weird details that you may not like.
Attachment #8387245 - Flags: review?(jorendorff)
Attachment #8380091 - Attachment is obsolete: true
Comment on attachment 8387245 [details] [diff] [review]
Make redirect() more robust with a FileObject

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

I'm sorry for the extra slow review.

::: js/src/shell/js.cpp
@@ +3843,5 @@
> +            return false;
> +        if (!redirect(cx, outPath, outFile))
> +            return false;
> +    } else if (args[0].isObject() && args[0].toObject().is<FileObject>()) {
> +        if (!(prev = FileObject::create(cx, *outFile, false)))

I'm not sure I'm following the logic of when to create a FileObject that owns the pointer. It seems like you could get it to double-free a FILE* this way:

    var original = redirectOutput("foo.txt");
    var foo1 = redirectOutput("other1.txt");
    redirectOutput(foo1);
    var foo2 = redirectOutput("other2.txt");
    foo1 = foo2 = null;
    gc();

I think it's also possible to get an owning File finalized while gOutFile still points to the corresponding FILE*.

Assuming that is correct, the trouble comes from using finalizers to release a resource that is also pointed to directly (from gOutFile and gErrFile). So maybe the answer is to replace gOutFile/gErrFile, at least in most places, with functions that fetch the FILE* from a property or slot (or something) on the current global --- that way, these resources behave like other GC'd resources, and there would be no use-after-free or double-free mistakes. All File objects would be owning in that model.
Attachment #8387245 - Flags: review?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #15)
> Comment on attachment 8387245 [details] [diff] [review]
> Make redirect() more robust with a FileObject
> 
> ::: js/src/shell/js.cpp
> @@ +3843,5 @@
> I'm not sure I'm following the logic of when to create a FileObject that
> owns the pointer. It seems like you could get it to double-free a FILE* this
> way:

Yeah, that's because it was utterly nonsensical.

>     var original = redirectOutput("foo.txt");
>     var foo1 = redirectOutput("other1.txt");
>     redirectOutput(foo1);
>     var foo2 = redirectOutput("other2.txt");
>     foo1 = foo2 = null;
>     gc();
> 
> I think it's also possible to get an owning File finalized while gOutFile
> still points to the corresponding FILE*.
> 
> Assuming that is correct, the trouble comes from using finalizers to release
> a resource that is also pointed to directly (from gOutFile and gErrFile). So
> maybe the answer is to replace gOutFile/gErrFile, at least in most places,
> with functions that fetch the FILE* from a property or slot (or something)
> on the current global --- that way, these resources behave like other GC'd
> resources, and there would be no use-after-free or double-free mistakes. All
> File objects would be owning in that model.

I took another swing at this. That's not the only problem. I could put a slot on the global holding a FileObject for the current output destination, then when redirecting I would return that FileObject and update the global with the new one, changing gOutFile at the same time.

But that'll break down if you call it from another global, since gOutFile is process-global and the global is just global to compartment.

And yet, I don't want this to be scoped to a compartment either, since if you're debugging a script that calls newGlobal, I don't want newGlobal.print() to ignore the redirection done by the original debuggee.

Ugh. My current approach is to make largely do-nothing FileObjects that do *not* close the files during finalization, and just leak the open files for now. I could almost use PrivateValue(FILE*) instead, but I still don't exactly understand what PrivateValues mean if they're exposed to JS. They seem to just be numbers? That you can forge? Confused. I guess you probably can't forge them.

Anyway, this approach is very limited in that it's fundamentally leaky, and I can't even expose an explicit close() function because then you'd be able to close the same FILE* twice. I guess I need a layer of abstraction, and to make these runtime-wide, but this is an incredibly silly thing for me to be spending my time on right now anyway.
While messing around with this, I thought it'd be cleaner to split out the file-related stuff from the global namespace. I'd kind of like to do this with almost all the shell functions, because having them in one big pile sucks. help() is useless; it's too long.

So this patch moves some code to OSObject.cpp, and creates an os.file object with methods for working on files. Then it aliases the original names to the os.file.* names.
Attachment #8584784 - Flags: review?(jorendorff)
Attached patch imported patch bug--shell-stuff (obsolete) — Splinter Review
Latest iteration of redirect() stuff. I still need to try out using it, so no point in requesting review yet.
Attachment #8387245 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/bebc75f51b5d
https://hg.mozilla.org/mozilla-central/rev/654dcfdaccca
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Comment on attachment 8584784 [details] [diff] [review]
Move a bunch of file-related stuff onto an os.file object, creating aliases from the old names

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

Glad to see redirect going away.

I would have spelled "readfile" with a capital F, but it's not like the shell is a beautiful garden, steeped in convention.

::: js/src/shell/OSObject.cpp
@@ +519,5 @@
> +        { "readfile", "snarf" },
> +        { "readRelativeToScript", "readRelativeToScript" }
> +    };
> +
> +    for (auto pair: osfile_exports) {

Fancy!
Attachment #8584784 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #20)
> Comment on attachment 8584784 [details] [diff] [review]
> Move a bunch of file-related stuff onto an os.file object, creating aliases
> from the old names
> 
> Review of attachment 8584784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Glad to see redirect going away.

Uh... oops. Sorry, that's from a bad patch split. I only meant to move it to os.file.redirect in this patch, not remove it entirely. I have a followup patch that adds it back in a different form, but I didn't post it yet because I'm still not happy with it.

redirect sucks but the one thing I ever use this stuff for depends heavily on it.

> I would have spelled "readfile" with a capital F, but it's not like the
> shell is a beautiful garden, steeped in convention.

I'm fine with renaming it before landing. 

> > +    for (auto pair: osfile_exports) {
> 
> Fancy!

My first ever use! Maybe I need a space before the colon, though. That looks funny.
Backed out for failing to compile on Windows XP:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e60a1963130c



c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\js\src\shell\OSObject.cpp(68) : error C3861: 'isalpha': identifier not found
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\js\src\shell\OSObject.cpp(86) : error C2065: 'MAX_PATH' : undeclared identifier
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\js\src\shell\OSObject.cpp(86) : error C2133: 'buffer' : unknown size
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\js\src\shell\OSObject.cpp(101) : error C2065: 'MAX_PATH' : undeclared identifier
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\js\src\shell\OSObject.cpp(101) : error C3861: 'getcwd': identifier not found
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\js\src\shell\OSObject.cpp(108) : error C2070: 'char []': illegal sizeof operand
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\src\js\src\shell\OSObject.cpp(109) : error C2065: 'MAX_PATH' : undeclared identifier
Flags: needinfo?(sphink)
Sorry, I wish I didn't need this redirect() nuisance. But at least this seems semantically sound, at the cost of touching more code than I wanted to.
Attachment #8603444 - Flags: review?(jorendorff)
Attachment #8584785 - Attachment is obsolete: true
Depends on: 1163379
We have to back this out, this breaks fuzzing in a way it cannot be unbroken.

The OS object is disabled with --fuzzing-safe but read and readline are essential for fuzzing. We either must move them back outside the OS object or make --fuzzing-safe only partially disable the OS object.

Until that has happened, I request backing out this patch.
This also caused a regression on kraken-darkroom.

Note there were two regression on kraken-darkroom. Bug 1067610 and this one.
This link tracks them both. So a bit hard to see.
http://arewefastyet.com/regressions/#/regression/1476122

But very visible: the backout fixed this:
http://arewefastyet.com/regressions/#/regression/1488119
Sorry about that. Re-landed with the missing functions added back in. Now the os object exists in fuzzing-safe mode, it just doesn't have any functions on it. It only has a 'file' property, with all of the read* functions.
Flags: needinfo?(sphink)
Depends on: 1166101
terrence - sorry, putting this in your review queue because I want to finally get it out of my patch queue. At first glance, this is going to look needlessly complex. The purpose is for supporting a shell debugger that can handle output redirection -- since Debugger runs in the same runtime and everything as the debuggee, I want the Debugger output to go to the console even when the debuggee has redirected it to a file it's building up. (Coincidentally, this is used by the hazard analysis scripts.) So I want to be able to redirect, remember the pre-redirection destination, and restore it. But that immediately brings up issues of ownership and reference counting in order to prevent massive filehandle leakage.

I tried several "simpler" approaches that were fewer lines of code at the cost of not being correct. You can read back through this bug if you care. (Some of the intermediate ones were just Dumb with a capital Duh, because I thought myself in circles.)

Anyway, take a look. Nothing else would be using it for now, so it's not high risk to have it in, but if you think it's too complex for the tiny niche case, then... I don't know, I'll probably give up on the whole redirection thing or something.
Attachment #8670104 - Flags: review?(terrence)
Attachment #8603444 - Attachment is obsolete: true
Attachment #8603444 - Flags: review?(jorendorff)
Comment on attachment 8670104 [details] [diff] [review]
Implement proper redirection with ref counted underlying files

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

I'm not against the overall strategy, but we should re-use the browser's refcounting infra for this.

::: js/src/shell/OSObject.cpp
@@ +291,5 @@
> +    this->close();
> +    return true;
> +}
> +
> +class FileObject : public JSObject {

I can't image there's not already a RefPtr compatible FILE* wrapper somewhere in the tree.

::: js/src/shell/jsshell.h
@@ +47,5 @@
>      }
>  };
>  
> +// Reference counted file.
> +struct RCFile {

I was going to say: please use the newly mfbt'ified mozilla::RefCounted<File*> and mozilla::RefPtr<FILE*> classes here instead of rolling our own refcount class. But it sounds like that's all changing tomorrow, so do whatever is most appropriate after bug 1207245 lands.
Attachment #8670104 - Flags: review?(terrence)
(In reply to Terrence Cole [:terrence] from comment #35)
> Comment on attachment 8670104 [details] [diff] [review]
> Implement proper redirection with ref counted underlying files
> 
> Review of attachment 8670104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not against the overall strategy, but we should re-use the browser's
> refcounting infra for this.

We discussed this in our call. I spent some hours on this, and it's not pretty. One problem is that we're hiding some of the references from the type system by storing these in PrivateValues, and RefPtr/RefCounted try to make that hard (understandably). But it also ends up being a rather cryptic tangle of alreadyAddRefed<RefPtr<File>> gunk. It seems it just isn't meant for making simple, basic refcounting like this nice.

Heck, RefCounted.h starts with

/* CRTP refcounting templates.  Do not use unless you are an Expert. */

That was the first clue.

I could probably make it work, possibly by hacking in some backdoors or subclasses with friend declarations, but it just feels like I'm trying to stuff a round rabbit into a square hole, one with sharp, metallic edges.

> ::: js/src/shell/OSObject.cpp
> @@ +291,5 @@
> > +    this->close();
> > +    return true;
> > +}
> > +
> > +class FileObject : public JSObject {
> 
> I can't image there's not already a RefPtr compatible FILE* wrapper
> somewhere in the tree.

If you can find it, let me know. I tried every dxr search I could come up. nsCOMPtr<nsIFile> is certainly used, but that doesn't do us any good.

> ::: js/src/shell/jsshell.h
> @@ +47,5 @@
> >      }
> >  };
> >  
> > +// Reference counted file.
> > +struct RCFile {
> 
> I was going to say: please use the newly mfbt'ified
> mozilla::RefCounted<File*> and mozilla::RefPtr<FILE*> classes here instead
> of rolling our own refcount class. But it sounds like that's all changing
> tomorrow, so do whatever is most appropriate after bug 1207245 lands.

That still seems to be the thing you'd use, more or less, if you wanted to push this through.

MozReview-Commit-ID: 7vQguTwjhI1
  I have no idea what puts ^ here. Maybe bzexport has been sabotaged to do this now?

I've rebased and re-uploaded the pre-RefPtr patch. It's probably not very different from the previous one you reviewed.
Attachment #8729203 - Flags: review?(terrence)
Attachment #8670104 - Attachment is obsolete: true
Comment on attachment 8729203 [details] [diff] [review]
Implement proper redirection with ref counted underlying files

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

Oh well, expecting sanity was probably too much.
Attachment #8729203 - Flags: review?(terrence) → review+
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: mozilla39 → mozilla48
You need to log in before you can comment on or make changes to this bug.