Chromium Code Reviews
Help | Chromium Project | Sign in
(254)

Issue 2693002: More precise break points and stepping when debugging... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by Søren Thygesen Gjesse
Modified:
2 years, 11 months ago
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 26

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1187 lines, -121 lines) Lint Patch
M src/arm/assembler-arm.h View 1 2 2 chunks +24 lines, -4 lines 0 comments 0 errors Download
M src/arm/assembler-arm.cc View 1 2 4 chunks +18 lines, -3 lines 0 comments 0 errors Download
M src/arm/assembler-arm-inl.h View 1 2 2 chunks +10 lines, -2 lines 0 comments 0 errors Download
M src/arm/codegen-arm.h View 1 2 1 chunk +3 lines, -1 line 0 comments 0 errors Download
M src/arm/codegen-arm.cc View 1 2 1 chunk +4 lines, -2 lines 0 comments 0 errors Download
M src/arm/debug-arm.cc View 1 2 3 chunks +66 lines, -3 lines 0 comments 0 errors Download
M src/arm/full-codegen-arm.cc View 1 2 1 chunk +4 lines, -2 lines 0 comments 0 errors Download
M src/assembler.h View 1 2 4 chunks +11 lines, -3 lines 0 comments 0 errors Download
M src/assembler.cc View 1 2 2 chunks +6 lines, -0 lines 0 comments 0 errors Download
M src/ast.h View 1 2 2 chunks +11 lines, -2 lines 0 comments 0 errors Download
M src/builtins.h View 1 2 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M src/builtins.cc View 1 2 1 chunk +7 lines, -0 lines 0 comments 0 errors Download
M src/checks.h View 1 2 1 chunk +4 lines, -2 lines 0 comments 0 errors Download
M src/codegen.cc View 1 2 1 chunk +14 lines, -5 lines 0 comments 0 errors Download
M src/d8.cc View 1 2 1 chunk +3 lines, -0 lines 0 comments 0 errors Download
M src/debug.h View 1 2 7 chunks +19 lines, -0 lines 0 comments 0 errors Download
M src/debug.cc View 1 2 10 chunks +53 lines, -20 lines 0 comments 0 errors Download
M src/flag-definitions.h View 1 2 1 chunk +1 line, -1 line 0 comments 0 errors Download
M src/full-codegen.h View 1 2 2 chunks +26 lines, -0 lines 0 comments 0 errors Download
M src/full-codegen.cc View 1 2 9 chunks +293 lines, -3 lines 0 comments 0 errors Download
M src/ia32/assembler-ia32.h View 1 2 3 chunks +13 lines, -1 line 0 comments 0 errors Download
src/ia32/assembler-ia32.cc View 1 2 4 chunks +16 lines, -1 line 0 comments 0 errors Download
M src/ia32/assembler-ia32-inl.h View 1 2 3 chunks +17 lines, -5 lines 0 comments 0 errors Download
src/ia32/codegen-ia32.h View 1 2 1 chunk +3 lines, -1 line 0 comments 0 errors Download
src/ia32/debug-ia32.cc View 1 2 2 chunks +42 lines, -0 lines 0 comments 0 errors Download
src/mark-compact.cc View 2 3 chunks +12 lines, -6 lines 0 comments ? errors Download
src/mips/assembler-mips.h View 1 2 2 chunks +4 lines, -1 line 0 comments ? errors Download
src/mips/assembler-mips.cc View 1 2 2 chunks +8 lines, -1 line 0 comments ? errors Download
src/objects.cc View 2 2 chunks +5 lines, -2 lines 0 comments ? errors Download
src/parser.cc View 1 2 1 chunk +4 lines, -1 line 0 comments ? errors Download
src/serialize.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments ? errors Download
src/x64/assembler-x64.h View 1 2 4 chunks +15 lines, -1 line 0 comments ? errors Download
src/x64/assembler-x64.cc View 1 2 3 chunks +15 lines, -1 line 0 comments ? errors Download
src/x64/assembler-x64-inl.h View 1 2 3 chunks +13 lines, -2 lines 0 comments ? errors Download
src/x64/codegen-x64.h View 1 2 1 chunk +3 lines, -1 line 0 comments ? errors Download
src/x64/debug-x64.cc View 1 2 2 chunks +43 lines, -0 lines 0 comments ? errors Download
test/cctest/test-debug.cc View 1 2 28 chunks +382 lines, -35 lines 0 comments ? errors Download
test/mjsunit/debug-conditional-breakpoints.js View 1 2 4 chunks +6 lines, -6 lines 0 comments ? errors Download
test/mjsunit/debug-step.js View 1 2 2 chunks +4 lines, -3 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 4
Søren Thygesen Gjesse
3 years, 10 months ago #1
yurys
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 ...
3 years, 10 months ago #2
Mads Ager (chromium)
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 ...
3 years, 10 months ago #3
Søren Thygesen Gjesse
3 years, 10 months ago #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6