Closed Bug 127944 Opened 20 years ago Closed 3 years ago

RFE - set next statement

Categories

(Other Applications Graveyard :: Venkman JS Debugger, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: timeless, Unassigned)

References

Details

(Whiteboard: [firebug-p3])

Attachments

(2 files, 5 obsolete files)

I have code right now that's doing

if (iid.Equals(something))
  return this;
throw Components.results.NS_NOINTERFACE;

venkman caught the exception, and i'd like to change the execution point from the throw to the return.
Sure this would be killer, but how feasible is this in the current engine?  Is
it possible to determine a range of "safe" pc's we could jump to?  In the 1.0
timeframe?
I think you have the primitives you need.  You know the line number the debugger
user wants to set as the "next statement".  You have JS_LineNumberToPC.  What am
I missing?

/be
I'm missing two things, I think.  The first, that there is no public way the set
the current pc of a stack frame, is an easy fix.  The second, which pc's can I
safely jump to without corrupting the stack, may or may not be a figment of my
imagination.  I assume that when we're in something like a switch or a
try/catch, there is cleanup code that must be run in order to cleanly pass
control back to the outer block.  I also assume that this could involve some
stack cleanup, that, if not properly done, could cause the engine to crash, or
at least become confused, when the containing function returns.

Consider:

 1: function f (x, z)
 2: {
 3:   var o = {y: 0};
 4:   try {
 5:     with (o) {
 6:      switch (x) {
 7:        case 1: y++; break;
 8:        case 2: y--; break;
 9:      }
10:    }
11:   } catch (e) {
12:     return "foo";
13:   }
14:   return o;
15: }

Jumping out of the with block, for example, would surely cause /some/ trouble,
right?  Would it be fatal, or just wierd?  How about jumping out, or in, to the
try or catch blocls?  
Status: NEW → ASSIGNED
for now i'd be willing to jump through hoops pseudo-stepping out of things.

suppose i was at line 12, if i could set the next statement to 13, step to 14, 
set to 4, step a bit ...
similarly if i were at 7 and could set to 9, step to 11 and set to 12.

visual studio forces me to be somewhat careful like this for cases where the 
place i'd like to set isn't in the current function.  (otherwise, it just adds 
the new destination on as a new frame -- i think).

i don't mind having to be careful, although certainly allowing me to be 
careless would be nice, but this is a debugger, it's kind of like rocket 
science :)
rginda: quite right, thanks for telling me what I was missing :-).  To balance
the stack, you could walk through the script's bytecode from its main entry
point to the pc you're trying to set as next statement, computing the stack
depth in the same way that the code generator does.  You'd want to do this in a
new jsdbgapi function.  It would go something like the patch I'm about to attach.

/be
JS_SetFramePC takes care of setting fp->sp for you -- it will fail (return null
after reporting an error) if you try to raise the stack pointer, because it
can't know what values should be pushed (e.g., a With object reference if
you're trying to set-next-statement into the middle of a with statement).

Since this function returns the old fp->pc value for convenience (in the best
case, to save a JS_GetFramePC call), this API must also fail if you're trying
to set the pc before the frame's script has begun execution (because fp->pc
will be null in that case -- note the valuable, unambiguous !fp->pc state
indicator mentioned recently! :-).

/be
That patch is untested, but it looks good to try, so please test well (all hard
cases: with/for..in/multi-level break/continue/try/catch/finally).

/be
rginda pointed out the futility and badness of reporting errors when the
debugger UI should do its own spanking.

/be
Attachment #72759 - Attachment is obsolete: true
What if you are at line 3 below, and set next statement to line 5?

1:  function f(o) {
2:    for (var i in o)
3:	print(i);
4:    with (o)
5:	print(x);
6:  }
7:  f({x:42});

The stack depth decreases, which seems ok, but what's on the stack at the
bottom of the operand stack is not a With object.  The interpreter will crash
and burn.  More work needed to safen this scenario.

/be
Attachment #72838 - Attachment is obsolete: true
js_Interpret maintains a local copy of pc (and len), so changing fp->pc doesn't
have any effect.  I've sprinkled |if (pc != fp->pc) { pc = fp->pc; len = 0 }| in
the places where we call out to a debugger hook.  This probably isn't exactly
right, it leaves me with...
Assertion failure: JS_UPTRDIFF(fp->sp, fp->spbase) <= depthdiff, at jsinterp.c:374
...when I try to change the pc from a breakpoint.  I'll investigate further. 
For the simple case I tested, changing the pc from a debugger; keyword works fine.

In the last patch, |depth| needs to be a uintN to avoid signed/unsigned
comparison warnings.  |len| could probably stand to be unsigned as well.
Attached patch js/jsd patch (obsolete) — Splinter Review
Changes to js/jsd, makes the jsdIStackFrame::pc attribute read/write... cool.

To test in venkman, type "loadd chrome://venkman/content/venkman-dev.js",
"treetest", and click "debug".	When you stop for the debugger keyword, type
"evald console.frames[0].pc = 0", and then "step".  If all goes well, you'll be
back at the top of the dbg2() function.
Attached patch js/src changes (obsolete) — Splinter Review
Brendan's last patch plus warning cleanup and jsinterp.c changes.

If I set a breakpoint in line 138 of chrome://venkman/content/tests/tree.js,
hit "debug", type "evald console.frames[0].pc = 0", and "cont", bad things
happen.

Previously I botched the assert at jsinterp.c:374 (noted in comment #10), now
I'm crashing...
Program received signal SIGSEGV, Segmentation fault.
0x400c544a in js_MarkGCThing (cx=0x80d8a30, thing=0x8877f50, arg=0x0)

Setting the pc from a debugger; keyword, however, seems to work fine.
Attachment #72856 - Attachment is obsolete: true
Brendan, do you want to work on this, or should I future it?
Sure, I'll take a look -- I may need your help.

/be
Assignee: rginda → brendan
Status: ASSIGNED → NEW
Priority: -- → P2
Target Milestone: --- → mozilla1.1alpha
Status: NEW → ASSIGNED
This fits in 1.1beta.

/be
Keywords: mozilla1.1
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Argh, missed.  1.2alpha is better than trying to cram this into 1.1.

/be
Keywords: mozilla1.1mozilla1.2
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Attachment #72883 - Attachment is obsolete: true
Moving out, some of these may move to 1.3alpha shortly.

/be
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Fixing TM.

/be
Target Milestone: mozilla1.2beta → mozilla1.3beta
Target Milestone: mozilla1.3beta → mozilla1.5alpha
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
Target Milestone: mozilla1.6alpha → mozilla1.7alpha
Target Milestone: mozilla1.7alpha → mozilla1.8alpha
Product: Core → Other Applications
Target Milestone: mozilla1.8alpha1 → mozilla1.8beta2
Target Milestone: mozilla1.8beta2 → mozilla1.9alpha
Target Milestone: mozilla1.9alpha → Future
QA Contact: caillon → venkman
Duplicate of this bug: 449466
Blocks: 449452
Whiteboard: [firebug-p3]
Thanks to Gijs for pointing this out. I'll try to revive this patch on 1.9.2. If anyone here knows more about the issues than they did in 2002, please speak up.
This is more doable now thanks to js_ReconstructStackDepth and friends, but it still needs work, and I'm not going to have time to do that work. I'll advise and review though, so a volunteer should take and patch.

/be
Assignee: brendan → general
Target Milestone: Future → ---
It seems like the original poster would have been happy with something to force a particular frame to return a particular value, like GDB's 'return' command.

I'm kind of astonished that everyone just jumped into the game of recreating the state needed by some arbitrary expression continuation in arbitrary JavaScript code; you simply don't have enough information to do anything like that in a meaningful way.  A debugger 'return' command is well-defined because it's just using a continuation you already have.

I can imagine a meaning for jumping to an arbitrary statement continuation that is within some lexical scope that encloses the current point of execution.  Those only differ in the PC, and use a scope you already have.
No arbitrary expression continuations -- statements, rather. SpiderMonkey was simple enough in the old days that you could hope to do this if the current pc was in a post-dominator of the next-statement and all stacked state was from outer statements still active.

The goal was not to support all set-next-statement attempts -- some would fail.

But this is a silly bug to keep around now that we are JITting. Feel free to morph or close.

/be
My primary interest is to halt the execution, change the function, and re-execute it in as many cases as possible. So I want "return immediately but back up the caller just enough to call again" with "I can't do that" being one outcome. (JIT is off for debugging). Java has this support and is it heavily used by eclipse users; it can have a big impact for Web 2.0 app devs.
Change the function or create a function if we're in an exception for an unimplemented function call?

It would be a great feature to be able to sit in a debugger, write the next method we needed and hit continue to jump into the newly-created method.

Debugger-driven development is powerful stuff, though the onus would be on the debugger client implementor to create mechanisms to save these created methods.

I think this feature goes beyond the original scope of this bug though.
comment 27 wants to use the __noSuchMethod__ hooks and a bit more trickiness to let the debugger signal that it has defined the method, I think, rather than using a general set-next-statement (since you want it to work as part of expressions).  proxies may make it a bit easier.
Assignee: general → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
Component is obsolete so resolving bugs as INCOMPLETE
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.