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

[yan...@chromium.org]

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#newcode112
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#newcode176
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.

http://codereview.chromium.org/10263002/

[yan...@chromium.org]

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.

http://codereview.chromium.org/10263002/

[yan...@chromium.org]

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.

http://codereview.chromium.org/10263002/

[peter...@gmail.com]

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.

http://codereview.chromium.org/10263002/

[peter...@gmail.com]

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#newcode112
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#newcode176
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.

http://codereview.chromium.org/10263002/

[peter...@gmail.com]

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.

http://codereview.chromium.org/10263002/

[peter...@gmail.com]

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

http://codereview.chromium.org/10263002/

[peter...@gmail.com]

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

http://codereview.chromium.org/10263002/

[peter...@gmail.com]

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).

http://codereview.chromium.org/10263002/

[peter...@gmail.com]

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

http://codereview.chromium.org/10263002/