Fix exit frame type in breakpoint stub

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

[SeRya, 2010-01-29 16:00:03]

I found that my prev CL changed type of ExitFrame for the stub in a breakpoint.
This case is not covered by unit test. Probably it could break a debugger. I'm
still looking for how to test it properly. So if you have an idea give me a
hint.

[Søren Thygesen Gjesse, 2010-02-01 08:57:19]

Thank you for spotting this. I missed the true prarmeter to Generate in the stub
inherited from CEntryStub.

To test this you will need to have recursive breakpoints and/or GC involved,
where the register content differs. What happens is that when a break is hit all
registers are saved to a static memory area. During the exit from JavaScript the
ones that contains objects are saved on the JavaScript stack and later they are
all saved on the C stack. When returning to JavaScript the static memory area is
first populated from the values on the C stack and then some of the values are
overwritten from the JavaScript stack. Without this fix the static memory area
will be used to save and restore without the values being moved to the stack.

Also please change the name c_entry_debug_break_code (root in heap.h) to
something like debugger_statement_code. Actually I don't think we heed to have
the DebuggerStatementStub as a root any more. You can do that as a separate
change.

Whether we need the CEntryStub with MODE_DEBUG instead I am not sure. Also I
just wondered about 64-bit Windows, where we have CEntryStub returning 1 value
as a root but not the one returning 2 values. This requires some more looking
into Heap::CreateFixedStubs() (where I also think a #ifdef
ENABLE_DEBUGGER_SUPPORT/#endif might be missing.

http://codereview.chromium.org/555164/diff/1/7
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/555164/diff/1/7#newcode6201
src/arm/codegen-arm.cc:6201: int CEntryStub::MinorKey() {
Don't you also need a new minor key here?

See comment for src/x64/codegen-x64.cc

http://codereview.chromium.org/555164/diff/1/4
File src/codegen.h (right):

http://codereview.chromium.org/555164/diff/1/4#newcode334
src/codegen.h:334: ExitFrame::Mode mode = ExitFrame::MODE_NORMAL)
Indention.

http://codereview.chromium.org/555164/diff/1/2
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/555164/diff/1/2#newcode9078
src/ia32/codegen-ia32.cc:9078: int CEntryStub::MinorKey() {
See comment for src/x64/codegen-x64.cc.

http://codereview.chromium.org/555164/diff/1/6
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/555164/diff/1/6#newcode7337
src/x64/codegen-x64.cc:7337: int CEntryStub::MinorKey() {
Please change this to use two 1-bit bitfields as e.g. CallFunctionStub. if
_WIN64 is not defined just encode 0 is the ResultSize bitfield.

You can then move CEntryStub::MinorKey() to codegen.h (codegen.cc) and have it
shared between platforms.

[SeRya, 2010-02-01 15:25:57]

http://codereview.chromium.org/555164/diff/1/7
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/555164/diff/1/7#newcode6201
src/arm/codegen-arm.cc:6201: int CEntryStub::MinorKey() {
On 2010/02/01 08:57:19, Søren Gjesse wrote:
> Don't you also need a new minor key here?
> 
> See comment for src/x64/codegen-x64.cc

Done.

http://codereview.chromium.org/555164/diff/1/4
File src/codegen.h (right):

http://codereview.chromium.org/555164/diff/1/4#newcode334
src/codegen.h:334: ExitFrame::Mode mode = ExitFrame::MODE_NORMAL)
On 2010/02/01 08:57:19, Søren Gjesse wrote:
> Indention.

Done.

http://codereview.chromium.org/555164/diff/1/2
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/555164/diff/1/2#newcode9078
src/ia32/codegen-ia32.cc:9078: int CEntryStub::MinorKey() {
On 2010/02/01 08:57:19, Søren Gjesse wrote:
> See comment for src/x64/codegen-x64.cc.

Done.

http://codereview.chromium.org/555164/diff/1/6
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/555164/diff/1/6#newcode7337
src/x64/codegen-x64.cc:7337: int CEntryStub::MinorKey() {
On 2010/02/01 08:57:19, Søren Gjesse wrote:
> Please change this to use two 1-bit bitfields as e.g. CallFunctionStub. if
> _WIN64 is not defined just encode 0 is the ResultSize bitfield.
> 
> You can then move CEntryStub::MinorKey() to codegen.h (codegen.cc) and have it
> shared between platforms.

Done.

[Søren Thygesen Gjesse, 2010-02-02 11:18:32]

LGTM

http://codereview.chromium.org/555164/diff/36/1022
File src/codegen.cc (right):

http://codereview.chromium.org/555164/diff/36/1022#newcode482
src/codegen.cc:482: ASSERT(result_size_ <= 2);
I suggest you collect the comments on how values are returned from C++ on the
different platforms (normally one return value but in rare cases two) either
here or some other place in codegen*.

http://codereview.chromium.org/555164/diff/36/1014
File src/codegen.h (right):

http://codereview.chromium.org/555164/diff/36/1014#newcode356
src/codegen.h:356: // if complex results must be written to address passed as
first argument
I find this comment a bit obscure. Also it should be with uppercase initial
letter and end with a period.

[SeRya, 2010-02-02 14:48:32]

http://codereview.chromium.org/555164/diff/36/1022
File src/codegen.cc (right):

http://codereview.chromium.org/555164/diff/36/1022#newcode482
src/codegen.cc:482: ASSERT(result_size_ <= 2);
On 2010/02/02 11:18:32, Søren Gjesse wrote:
> I suggest you collect the comments on how values are returned from C++ on the
> different platforms (normally one return value but in rare cases two) either

I moved that comments in CEntryStub::Generate (now incoming and outcoming
register values are describet at single place).
> here or some other place in codegen*.

http://codereview.chromium.org/555164/diff/36/1014
File src/codegen.h (right):

http://codereview.chromium.org/555164/diff/36/1014#newcode356
src/codegen.h:356: // if complex results must be written to address passed as
first argument
On 2010/02/02 11:18:32, Søren Gjesse wrote:
> I find this comment a bit obscure. Also it should be with uppercase initial
> letter and end with a period.

Removed the comment.

[William Hesse, 2010-02-03 06:18:35]

There is an error in the win32 build, due to the commented lines in codegen.cc.

http://codereview.chromium.org/555164/diff/11001/12011
File src/codegen.cc (right):

http://codereview.chromium.org/555164/diff/11001/12011#newcode485
src/codegen.cc:485: const indirect_result = result_size_ > 1;
This seems really wrong.  Either make the type (int) explicit, or make this a
constant bool.

http://codereview.chromium.org/555164/diff/11001/12011#newcode491
src/codegen.cc:491: | IndirectResultBits::encode(indirect_result > 1);
Shouldn't this just be indirect_result, not indirect_result > 1?  The second
expression is never true.

In all cases except _WIN64, this is encoding 0, which probably doesn't affect
the bitwise or, so maybe the entire return statement should be in an #ifdef, and
be optimized in all cases except WIN64 to just return
ExitFrameModeBits::encode(mode).