Account Options

  1. Sign in
The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Google Groups Home
« Groups Home
v8-dev
Home
+ new post
About this group
Join this group
Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Options
There are currently too many topics in this group that display first. To make this topic appear first, remove this option from another topic.
There was an error processing your request. Please try again.
flag
   10 messages - Collapse all  -  Translate all to Translated (View all originals)
The group you are posting to is a Usenet group. Messages posted to this group will make your email address visible to anyone on the Internet.
Your reply message has not been sent.
Your post was successful
 
From:
To:
Cc:
Followup To:
Add Cc | Add Followup-to | Edit Subject
Subject:
Validation:
For verification purposes please type the characters you see in the picture below or the numbers you hear by clicking the accessibility icon. Listen and type the numbers you hear
 
yang...@chromium.org  
View profile  
 More options May 2 2012, 7:38 am
From: yang...@chromium.org
Date: Wed, 02 May 2012 11:38:34 +0000
Local: Wed, May 2 2012 7:38 am
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author
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#newcod...
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...
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...
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#new...
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#new...
src/liveedit.cc:1534: Memory::int_at(padding_pointer) -= shortage_bytes
/ kPointerSize * 2;
Same here.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
yang...@chromium.org  
View profile  
 More options May 3 2012, 10:00 am
From: yang...@chromium.org
Date: Thu, 03 May 2012 14:00:46 +0000
Local: Thurs, May 3 2012 10:00 am
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author
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#newcod...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
yang...@chromium.org  
View profile  
 More options May 3 2012, 1:33 pm
From: yang...@chromium.org
Date: Thu, 03 May 2012 17:33:56 +0000
Local: Thurs, May 3 2012 1:33 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
peter.ry...@gmail.com  
View profile  
 More options May 2 2012, 7:58 pm
From: peter.ry...@gmail.com
Date: Wed, 02 May 2012 23:58:20 +0000
Local: Wed, May 2 2012 7:58 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author

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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
peter.ry...@gmail.com  
View profile  
 More options May 2 2012, 7:46 pm
From: peter.ry...@gmail.com
Date: Wed, 02 May 2012 23:46:33 +0000
Local: Wed, May 2 2012 7:46 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author

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#newcod...
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...
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...
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#new...
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#new...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
peter.ry...@gmail.com  
View profile  
 More options May 3 2012, 1:07 pm
From: peter.ry...@gmail.com
Date: Thu, 03 May 2012 17:07:11 +0000
Local: Thurs, May 3 2012 1:07 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author

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#newcod...
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
peter.ry...@gmail.com  
View profile  
 More options May 3 2012, 1:31 pm
From: peter.ry...@gmail.com
Date: Thu, 03 May 2012 17:31:18 +0000
Local: Thurs, May 3 2012 1:31 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author
I also added missing guard variable definition on other platforms and  
slightly
improved test output.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
peter.ry...@gmail.com  
View profile  
 More options May 2 2012, 8:55 pm
From: peter.ry...@gmail.com
Date: Thu, 03 May 2012 00:55:10 +0000
Local: Wed, May 2 2012 8:55 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author
It also fixes an obvious bug with checking stub major key.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
peter.ry...@gmail.com  
View profile  
 More options May 2 2012, 8:00 pm
From: peter.ry...@gmail.com
Date: Thu, 03 May 2012 00:00:35 +0000
Local: Wed, May 2 2012 8:00 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
peter.ry...@gmail.com  
View profile  
 More options May 3 2012, 2:25 pm
From: peter.ry...@gmail.com
Date: Thu, 03 May 2012 18:25:19 +0000
Local: Thurs, May 3 2012 2:25 pm
Subject: Re: Fix issue 825 (LiveEdit vs. function with no locals) in core and for ia32. (issue 10263002)
Print | Individual message | Show original | Report this message | Find messages by this author
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/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
End of messages
« Back to Discussions « Newer topic     Older topic »

Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
©2013 Google