Wrap external pointers more carefully.

Wrap external pointers more carefully.

On 32-bit platforms any pointer with 0 as LSB can be wrapped into Smi.  However, on 64-bit
platforms it's currently not the case as x64 Smis must have 0s in lower 32 bit word.
Even worse, macroassembler Move instruction will try to fetch integer value from Smi
and will shift by 32 bits to the right rendering stored pointer incorrect.

BUG=v8:1037
Committed: http://code.google.com/p/v8/source/detail?r=6301

[antonm, 2011-01-12 16:40:03]

Lasse,

may you have a look?

[Lasse Reichstein, 2011-01-13 08:35:18]

We need to distinguish between actual smis and smi-tagged values. You can have
smi-tagged values with bits set in the 1..31 range, but not smis. 

Have functions that convert an aligned pointer to a smi-tagged value *as an
intptr_t*, not a smi. 

Then use Move with intptr_t (and RelocInfo::NONE) to store and load it, not Move
of Smi*.

We need all the bits of a 64-bit value to store the pointer, so we can't
restrict ourselves to the Smi value range.

http://codereview.chromium.org/6119009/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6119009/diff/1/include/v8.h#newcode3437
include/v8.h:3437: reinterpret_cast<intptr_t>(smi) >> kSmiShiftSize);
I don't think this will work in 64-bit mode (kShiftSize is 31 there, so you lose
a LOT of bits). You need to use all the non-tag bits.
On the other hand, I guess you really meant kSmiTagSize.

We should just have a function (per pointer size) that embeds either an
arbitrary pointer, or just an aligned pointer, in a smi-tagged value (bit zero
is 0).

If the pointer is arbitrary, you need to throw away at least one bit. Probably
the most significant one (but you can't know that that is safe, e.g., in Windows
apps running with 3Gb user space). If it's aligned (like a stack pointer or
similar), we know that the low bit is zero, and we can cast it directly to Smi
and back.

E.g. put functions into SmiConstants<4> and SmiConstants<8> above.

http://codereview.chromium.org/6119009/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6119009/diff/1/src/api.cc#newcode3271
src/api.cc:3271: const intptr_t mask = (intptr_t(1) << i::kSmiTagSize) - 1;
Just use kSmiTagMask.

http://codereview.chromium.org/6119009/diff/1/src/api.cc#newcode3272
src/api.cc:3272: return ((address & mask) == 0) && i::Smi::IsValid(address >>
i::kSmiTagSize);
Almost no addresses will be valid in 64-bit mode. Only those in the low 31-bit
of the address space will work.

Let's just embed the pointer as a smi-tagged, but non-smi, value, with bits in
the lower half as well.
As long as we only read it again using GetPointerFromInternalField, or in custom
generated code that knows what it's doing, it should be safe.

[antonm, 2011-01-13 12:44:55]

Lasse,

may you have another look?

[Lasse Reichstein, 2011-01-13 12:57:57]

LGTM

http://codereview.chromium.org/6119009/diff/6001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6119009/diff/6001/include/v8.h#newcode3356
include/v8.h:3356: template <size_t ptr_size> struct SmiTraits;
I think I would prefer "SmiTagging", but it's better than SmiConstants.

http://codereview.chromium.org/6119009/diff/6001/include/v8.h#newcode3386
include/v8.h:3386: // alignment.  Currently we require 8 bytes alignment (which
Drop "better" alignment (the text should be stand-alone, so it should not be
implicitly relative to the 32-bit case).
Just say something like "To maximize the range of pointers that can be encoded
in the available 32 bit, we require them to be 8-byte aligned".

http://codereview.chromium.org/6119009/diff/6001/include/v8.h#newcode3393
include/v8.h:3393: intptr_t(0xffffffff) << kPointerAlignment;
Shouldn't this be inverted to mask the bits that must be zero?

http://codereview.chromium.org/6119009/diff/6001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6119009/diff/6001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:829: *value = 42;
Try storing both values that can and can't be stored as smis.

[antonm, 2011-01-13 15:49:15]

Thanks a lot for review, Lasse.  Submitting.

http://codereview.chromium.org/6119009/diff/6001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6119009/diff/6001/include/v8.h#newcode3356
include/v8.h:3356: template <size_t ptr_size> struct SmiTraits;
On 2011/01/13 12:57:57, Lasse Reichstein wrote:
> I think I would prefer "SmiTagging", but it's better than SmiConstants.

Done.

http://codereview.chromium.org/6119009/diff/6001/include/v8.h#newcode3386
include/v8.h:3386: // alignment.  Currently we require 8 bytes alignment (which
On 2011/01/13 12:57:57, Lasse Reichstein wrote:
> Drop "better" alignment (the text should be stand-alone, so it should not be
> implicitly relative to the 32-bit case).
> Just say something like "To maximize the range of pointers that can be encoded
> in the available 32 bit, we require them to be 8-byte aligned".

Done.

http://codereview.chromium.org/6119009/diff/6001/include/v8.h#newcode3393
include/v8.h:3393: intptr_t(0xffffffff) << kPointerAlignment;
On 2011/01/13 12:57:57, Lasse Reichstein wrote:
> Shouldn't this be inverted to mask the bits that must be zero?

Thanks a lot, of course it should.

http://codereview.chromium.org/6119009/diff/6001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6119009/diff/6001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:829: *value = 42;
On 2011/01/13 12:57:57, Lasse Reichstein wrote:
> Try storing both values that can and can't be stored as smis.

Done.