Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32.

Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32.

Committed: https://code.google.com/p/v8/source/detail?r=11502

[Peter Rybin, 2012-04-29 01:09:56]

Hi Yang

This is a long-awaited fix for LiveEdit.

This change has some ad-hoc design doc in the issue:
http://code.google.com/p/v8/issues/detail?id=825

Plus a put a number of comments along the code itself.

Peter

[Yang, 2012-05-02 11:38:34]

Integers are not always converted to Smis by a left-shift by one. While we can
assume that on platform-dependent code (ia32, arm and mips), we avoid using that
assumption on platform-independent code because on x64 a Smi is converted by a
left-shift by 32. Code that intend to store integers are Smis should therefore
use Smi::FromInt() and Smi::cast()->value().

http://codereview.chromium.org/10263002/diff/1006/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/10263002/diff/1006/src/debug.cc#newcode2234
src/debug.cc:2234: const int Debug::FramePaddingLayout::kPaddingValue =
(kInitialSize + 1) * 2;
Use Smi::FromInt to get tagged smi values since x64 tags differently. Why does
the value need to be larger than kInitialSize (smi-tagged)?

http://codereview.chromium.org/10263002/diff/1006/src/debug.h
File src/debug.h (right):

http://codereview.chromium.org/10263002/diff/1006/src/debug.h#newcode487
src/debug.h:487: struct FramePaddingLayout {
we would typically define this as a class deriving from AllStatic (e.g. class
ArgumentsAdaptorFrameConstants : public AllStatic)

http://codereview.chromium.org/10263002/diff/1006/src/debug.h#newcode493
src/debug.h:493: static const int kFrameBaseSize = 4;
This coincides with Debug::kFrameDropperFrameSize, which is also 4. If they are
derived similarly, can't we consistently use only one of the two?

http://codereview.chromium.org/10263002/diff/1006/src/ia32/debug-ia32.cc
File src/ia32/debug-ia32.cc (right):

http://codereview.chromium.org/10263002/diff/1006/src/ia32/debug-ia32.cc#newc...
src/ia32/debug-ia32.cc:112: __ push(Immediate(2 *
Debug::FramePaddingLayout::kInitialSize));
I assume multiplying by 2 is for Smi-tagging? Use
Smi::FromInt(Debug::FramePaddingLayout::kInitialSize) instead.

http://codereview.chromium.org/10263002/diff/1006/src/ia32/debug-ia32.cc#newc...
src/ia32/debug-ia32.cc:176: __ lea(esp, Operand(esp, unused_reg,
times_half_pointer_size, 0));
This is based on the implication that Smis are represented by left shift by 1
bit. Please add a STATIC_ASSERT(kSmiTagSize == 1) or a comment to state this
implication.

http://codereview.chromium.org/10263002/diff/1006/src/liveedit.cc
File src/liveedit.cc (right):

http://codereview.chromium.org/10263002/diff/1006/src/liveedit.cc#newcode1530
src/liveedit.cc:1530: if (Memory::int_at(padding_pointer) / 2 * kPointerSize <
shortage_bytes) {
I suppose this is a Smi-untagging.

Use Smi::cast(Memory::Object_at())->value() instead would be cleaner, especially
since x64 smi-tags differently.

http://codereview.chromium.org/10263002/diff/1006/src/liveedit.cc#newcode1534
src/liveedit.cc:1534: Memory::int_at(padding_pointer) -= shortage_bytes /
kPointerSize * 2;
Same here.

[Peter Rybin, 2012-05-02 23:46:33]

http://codereview.chromium.org/10263002/diff/1006/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/10263002/diff/1006/src/debug.cc#newcode2234
src/debug.cc:2234: const int Debug::FramePaddingLayout::kPaddingValue =
(kInitialSize + 1) * 2;
On 2012/05/02 11:38:34, Yang wrote:
> Use Smi::FromInt to get tagged smi values since x64 tags differently.
Done

> Why does
> the value need to be larger than kInitialSize (smi-tagged)?

This is how liveedit part works. It reads from the frame base and first that it
sees are padding words. It iterates until they end and this is counter.
Therefore padding words mustn't look like any possible value of counter word.

http://codereview.chromium.org/10263002/diff/1006/src/ia32/debug-ia32.cc
File src/ia32/debug-ia32.cc (right):

http://codereview.chromium.org/10263002/diff/1006/src/ia32/debug-ia32.cc#newc...
src/ia32/debug-ia32.cc:112: __ push(Immediate(2 *
Debug::FramePaddingLayout::kInitialSize));
On 2012/05/02 11:38:34, Yang wrote:
> I assume multiplying by 2 is for Smi-tagging? Use
> Smi::FromInt(Debug::FramePaddingLayout::kInitialSize) instead.

Done.

http://codereview.chromium.org/10263002/diff/1006/src/ia32/debug-ia32.cc#newc...
src/ia32/debug-ia32.cc:176: __ lea(esp, Operand(esp, unused_reg,
times_half_pointer_size, 0));
On 2012/05/02 11:38:34, Yang wrote:
> This is based on the implication that Smis are represented by left shift by 1
> bit. Please add a STATIC_ASSERT(kSmiTagSize == 1) or a comment to state this
> implication.

Done.

http://codereview.chromium.org/10263002/diff/1006/src/liveedit.cc
File src/liveedit.cc (right):

http://codereview.chromium.org/10263002/diff/1006/src/liveedit.cc#newcode1530
src/liveedit.cc:1530: if (Memory::int_at(padding_pointer) / 2 * kPointerSize <
shortage_bytes) {
On 2012/05/02 11:38:34, Yang wrote:
> I suppose this is a Smi-untagging.
> 
> Use Smi::cast(Memory::Object_at())->value() instead would be cleaner,
especially
> since x64 smi-tags differently.

Done.

http://codereview.chromium.org/10263002/diff/1006/src/liveedit.cc#newcode1534
src/liveedit.cc:1534: Memory::int_at(padding_pointer) -= shortage_bytes /
kPointerSize * 2;
On 2012/05/02 11:38:34, Yang wrote:
> Same here.

Done.

[Peter Rybin, 2012-05-02 23:58:20]

http://codereview.chromium.org/10263002/diff/1006/src/debug.h
File src/debug.h (right):

http://codereview.chromium.org/10263002/diff/1006/src/debug.h#newcode487
src/debug.h:487: struct FramePaddingLayout {
On 2012/05/02 11:38:34, Yang wrote:
> we would typically define this as a class deriving from AllStatic (e.g. class
> ArgumentsAdaptorFrameConstants : public AllStatic)

Done.

http://codereview.chromium.org/10263002/diff/1006/src/debug.h#newcode493
src/debug.h:493: static const int kFrameBaseSize = 4;
On 2012/05/02 11:38:34, Yang wrote:
> This coincides with Debug::kFrameDropperFrameSize, which is also 4. If they
are
> derived similarly, can't we consistently use only one of the two?

I'm not sure. Both are derived from the fact that the minimum size of
InternalFrame is 4 words. But they are layouts of different frames. So I would
say it's still more of coincidence here.

[Peter Rybin, 2012-05-03 00:00:35]

On 2012/05/02 11:38:34, Yang wrote:
> Integers are not always converted to Smis by a left-shift by one. While we can
> assume that on platform-dependent code (ia32, arm and mips), we avoid using
that
> assumption on platform-independent code because on x64 a Smi is converted by a
> left-shift by 32. Code that intend to store integers are Smis should therefore
> use Smi::FromInt() and Smi::cast()->value().

Thanks a lot for the very accurate review. I hope I addressed all your comments.
Plus add added a test (currently disabled because if issue 915).

[Peter Rybin, 2012-05-03 00:55:10]

It also fixes an obvious bug with checking stub major key.

[Yang, 2012-05-03 14:00:46]

LGTM with one comment. And please update the copyright headers to 2012.

http://codereview.chromium.org/10263002/diff/18001/src/debug.h
File src/debug.h (right):

http://codereview.chromium.org/10263002/diff/18001/src/debug.h#newcode487
src/debug.h:487: struct FramePaddingLayout : public AllStatic {
Even though not actually necessary, please make this a class (instead of struct)
for consistency's sake.

[Peter Rybin, 2012-05-03 17:07:11]

http://codereview.chromium.org/10263002/diff/18001/src/debug.h
File src/debug.h (right):

http://codereview.chromium.org/10263002/diff/18001/src/debug.h#newcode487
src/debug.h:487: struct FramePaddingLayout : public AllStatic {
On 2012/05/03 14:00:47, Yang wrote:
> Even though not actually necessary, please make this a class (instead of
struct)
> for consistency's sake.

Done.

[Peter Rybin, 2012-05-03 17:31:18]

I also added missing guard variable definition on other platforms and slightly
improved test output.

[Yang, 2012-05-03 17:33:56]

On 2012/05/03 17:31:18, Peter Rybin wrote:
> I also added missing guard variable definition on other platforms and slightly
> improved test output.

LGTM.

[Peter Rybin, 2012-05-03 18:25:18]

I must have failed to properly set mjsunit.status record for my test.
I'm going to fix it now.