More precise break points and stepping when debugging...

More precise break points and stepping when debugging

Added support for more precise break points when debugging and stepping. To achieve that additional nop instructions are inserted where breaking would otherwise be impossible. The number of nop instructions inserted are sufficient to make place for patching with a call to a debug break code stub. On Intel that is 5 nop's for 32-bit and 13 for 64-bit. Om ARM 3 nop instructions (12 bytes) are required.

In order to avoid inserting nop's in to many places a simple ast checker have been added to check whether there are breakable code in a statement or expression. If it is possible to break in an expression no additional break enabeling code is inserted.

Added break locations to the true and false part of a conditional expression.

Added stepping tests to cover more constructs.

These changes are only in the full compiler.

Changed the default value for the option --debugger in teh d8 shell from true to false. The reason for this is that with --debugger turned on the full compiler will be used for all code in when running d8, which can be unexpeceted.


Committed: http://code.google.com/p/v8/source/detail?r=4820

[yurys, 2010-06-08 08:06:04]

LGTM, did you measure performance impact caused by this change?

http://codereview.chromium.org/2693002/diff/18001/19009
File src/assembler.h (right):

http://codereview.chromium.org/2693002/diff/18001/19009#newcode250
src/assembler.h:250: // Check whether this return debug break slot has been
patched
typo:  this return debug ->  this debug

http://codereview.chromium.org/2693002/diff/18001/19016
File src/debug.cc (right):

http://codereview.chromium.org/2693002/diff/18001/19016#newcode1670
src/debug.cc:1670: addr - Assembler::kPatchReturnSequenceAddressOffset);
should be kPatchDebugBreakSlotAddressOffset

http://codereview.chromium.org/2693002/diff/18001/19016#newcode1697
src/debug.cc:1697: // address to jump to to complete the call which is replaced
by a call to
typo: to to -> to

http://codereview.chromium.org/2693002/diff/18001/19019
File src/full-codegen.cc (right):

http://codereview.chromium.org/2693002/diff/18001/19019#newcode522
src/full-codegen.cc:522: // Mark for statements breakable breakable if the
condition expression is.
 breakable breakable ->  breakable

http://codereview.chromium.org/2693002/diff/18001/19019#newcode795
src/full-codegen.cc:795: if (position_recorded) {
why test for !checker.is_breakable() wouldn't work here?

http://codereview.chromium.org/2693002/diff/18001/19019#newcode809
src/full-codegen.cc:809: if (!Debugger::IsDebuggerActive()) {
This piece of code mostly resembles the one above, consider extracting it in a
method.

http://codereview.chromium.org/2693002/diff/18001/19019#newcode832
src/full-codegen.cc:832: CodeGenerator::RecordPositions(masm_,
stmt->statement_pos());
there is no stmt variable in this scope

http://codereview.chromium.org/2693002/diff/18001/19020
File src/full-codegen.h (right):

http://codereview.chromium.org/2693002/diff/18001/19020#newcode64
src/full-codegen.h:64: // that there will be an IC (loat/store/call) in the code
generated for the
typo: loat -> load

http://codereview.chromium.org/2693002/diff/18001/19034
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/2693002/diff/18001/19034#newcode458
src/x64/assembler-x64.h:458: // Distance between start of patched debug break
slot and and where the
and and -> and

[Mads Ager (chromium), 2010-06-08 10:12:54]

LGTM

http://codereview.chromium.org/2693002/diff/18001/19003
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/2693002/diff/18001/19003#newcode646
src/arm/assembler-arm.h:646: // Return sequence is:
The comments describe the return sequence. Update?

http://codereview.chromium.org/2693002/diff/18001/19006
File src/arm/debug-arm.cc (right):

http://codereview.chromium.org/2693002/diff/18001/19006#newcode97
src/arm/debug-arm.cc:97: // Patch the code changing the return from JS function
sequence from
This talks about the return sequence. The rest of the comment seems to have been
updated correctly.

http://codereview.chromium.org/2693002/diff/18001/19006#newcode101
src/arm/debug-arm.cc:101: // to a call to the debug break return code.
return code -> code?

http://codereview.chromium.org/2693002/diff/18001/19016
File src/debug.cc (right):

http://codereview.chromium.org/2693002/diff/18001/19016#newcode133
src/debug.cc:133: // There is always a possible break point as a debug break
slot.
as -> at

[Søren Thygesen Gjesse, 2010-06-08 12:04:33]

Sorry for all the typos.

Regarding the speed then when the debugger is active all code will be compiled
by the full compiler which generates code which is 20-30% slower. With the added
code to debug break slots code size will also increase depending on the
function.

http://codereview.chromium.org/2693002/diff/18001/19003
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/2693002/diff/18001/19003#newcode646
src/arm/assembler-arm.h:646: // Return sequence is:
On 2010/06/08 10:12:55, Mads Ager wrote:
> The comments describe the return sequence. Update?

Done.

http://codereview.chromium.org/2693002/diff/18001/19006
File src/arm/debug-arm.cc (right):

http://codereview.chromium.org/2693002/diff/18001/19006#newcode97
src/arm/debug-arm.cc:97: // Patch the code changing the return from JS function
sequence from
On 2010/06/08 10:12:55, Mads Ager wrote:
> This talks about the return sequence. The rest of the comment seems to have
been
> updated correctly.

Done.

http://codereview.chromium.org/2693002/diff/18001/19006#newcode101
src/arm/debug-arm.cc:101: // to a call to the debug break return code.
On 2010/06/08 10:12:55, Mads Ager wrote:
> return code -> code?

Done (return code -> slot code).

http://codereview.chromium.org/2693002/diff/18001/19009
File src/assembler.h (right):

http://codereview.chromium.org/2693002/diff/18001/19009#newcode250
src/assembler.h:250: // Check whether this return debug break slot has been
patched
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> typo:  this return debug ->  this debug

Done.

http://codereview.chromium.org/2693002/diff/18001/19016
File src/debug.cc (right):

http://codereview.chromium.org/2693002/diff/18001/19016#newcode133
src/debug.cc:133: // There is always a possible break point as a debug break
slot.
On 2010/06/08 10:12:55, Mads Ager wrote:
> as -> at

Done.

http://codereview.chromium.org/2693002/diff/18001/19016#newcode1670
src/debug.cc:1670: addr - Assembler::kPatchReturnSequenceAddressOffset);
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> should be kPatchDebugBreakSlotAddressOffset

Done.

http://codereview.chromium.org/2693002/diff/18001/19016#newcode1697
src/debug.cc:1697: // address to jump to to complete the call which is replaced
by a call to
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> typo: to to -> to

Rephrased.

http://codereview.chromium.org/2693002/diff/18001/19019
File src/full-codegen.cc (right):

http://codereview.chromium.org/2693002/diff/18001/19019#newcode522
src/full-codegen.cc:522: // Mark for statements breakable breakable if the
condition expression is.
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
>  breakable breakable ->  breakable

Done.

http://codereview.chromium.org/2693002/diff/18001/19019#newcode795
src/full-codegen.cc:795: if (position_recorded) {
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> why test for !checker.is_breakable() wouldn't work here?

There are situations where just checking for !checker.is_breakable() here would
generate to many break slots.

http://codereview.chromium.org/2693002/diff/18001/19019#newcode809
src/full-codegen.cc:809: if (!Debugger::IsDebuggerActive()) {
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> This piece of code mostly resembles the one above, consider extracting it in a
> method.

Yes, I know, however with the difference in argument type refactoring will not
unify much.

http://codereview.chromium.org/2693002/diff/18001/19019#newcode832
src/full-codegen.cc:832: CodeGenerator::RecordPositions(masm_,
stmt->statement_pos());
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> there is no stmt variable in this scope

Good catch. I will ensure it builds with ENABLE_DEBUGGER_SUPPORT not defined
before committing.

http://codereview.chromium.org/2693002/diff/18001/19020
File src/full-codegen.h (right):

http://codereview.chromium.org/2693002/diff/18001/19020#newcode64
src/full-codegen.h:64: // that there will be an IC (loat/store/call) in the code
generated for the
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> typo: loat -> load

Done.

http://codereview.chromium.org/2693002/diff/18001/19034
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/2693002/diff/18001/19034#newcode458
src/x64/assembler-x64.h:458: // Distance between start of patched debug break
slot and and where the
On 2010/06/08 08:06:04, Yury Semikhatsky wrote:
> and and -> and

Done.