Removes open arrays (e.g. data[0]) from raw objects.

Removes open arrays (e.g. data[0]) from raw objects.

Replaces them with data() methods as in RawArray.

R=asiva@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37450

[Florian Schneider, 2014-06-16 08:27:26]

dbc: What is the motivation for this?

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h#newc...
runtime/vm/raw_object.h:563: return
reinterpret_cast<RawAbstractType**>(address_of_length + kWordSize);
Can't you just use sizeof(RawTypeArguments::length_)?

[zra, 2014-06-16 14:22:53]

The motivation is to change intptr_t in raw objects to int32_t or int64_t. This
can throw off alignment on 64-bit making the open arrays start in the wrong
place. This change makes sure that the data following the objects start right
after the last field, as before, even if we were to change the size of the
fields. This is what is already done for RawArray.

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h#newc...
runtime/vm/raw_object.h:563: return
reinterpret_cast<RawAbstractType**>(address_of_length + kWordSize);
On 2014/06/16 08:27:26, Florian Schneider wrote:
> Can't you just use sizeof(RawTypeArguments::length_)?

Not sure what you mean.

[Florian Schneider, 2014-06-16 14:46:25]

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h#newc...
runtime/vm/raw_object.h:563: return
reinterpret_cast<RawAbstractType**>(address_of_length + kWordSize);
On 2014/06/16 14:22:52, zra wrote:
> On 2014/06/16 08:27:26, Florian Schneider wrote:
> > Can't you just use sizeof(RawTypeArguments::length_)?
> 
> Not sure what you mean.

return reinterpret_cast<RawAbstractType**>(address_of_length +
sizeof(RawTypeArguments::length_));

[zra, 2014-06-16 17:00:15]

PTAL

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/1/runtime/vm/raw_object.h#newc...
runtime/vm/raw_object.h:563: return
reinterpret_cast<RawAbstractType**>(address_of_length + kWordSize);
On 2014/06/16 14:46:24, Florian Schneider wrote:
> On 2014/06/16 14:22:52, zra wrote:
> > On 2014/06/16 08:27:26, Florian Schneider wrote:
> > > Can't you just use sizeof(RawTypeArguments::length_)?
> > 
> > Not sure what you mean.
> 
> return reinterpret_cast<RawAbstractType**>(address_of_length +
> sizeof(RawTypeArguments::length_));

Yah, that makes sense. Done here and elsewhere.

[zra, 2014-06-16 19:29:28]

+srdjan for restoring RawFunction snapshot change.

[srdjan, 2014-06-16 19:41:10]

LGTM for snapshot change

[Ivan Posva, 2014-06-16 23:02:13]

https://codereview.chromium.org/333773006/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/60001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:862: uword address_of_lazy =
reinterpret_cast<uword>(&lazy_deopt_pc_offset_);
As discussed this pattern looks a bit fragile from the point of keeping size and
alignment in sync with expectations.

[zra, 2014-06-17 17:24:08]

https://codereview.chromium.org/333773006/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/60001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:862: uword address_of_lazy =
reinterpret_cast<uword>(&lazy_deopt_pc_offset_);
On 2014/06/16 23:02:13, Ivan Posva wrote:
> As discussed this pattern looks a bit fragile from the point of keeping size
and
> alignment in sync with expectations.

Changed to:

const uword result = reinterpret_cast<uword>(this) + sizeof(*this);
ASSERT(Utils::IsAligned(result, sizeof(int32_t)));
return reinterpret_cast<int32_t*>(result);

With the appropriate types substituted for int32_t in other places.

[siva, 2014-06-17 22:29:50]

lgtm

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/object.h#newc...
runtime/vm/object.h:3823: return data_offset() + kWordSize * context_index;
why not OFFSET_OF_RETURNED_VALUE(RawContext, data) + context_index * kWordSize ?

and drop data_offset().

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/object.h#newc...
runtime/vm/object.h:5958: static intptr_t data_offset() { return
sizeof(RawArray); }
why not return OFFSET_OF_RETURNED_VALUE(RawArray, data) for consistency?

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:563: return reinterpret_cast<RawAbstractType**>(result);
why not make a macro and use that everywhere?
e.g
#define OPEN_ARRAY_START(type)                                        \
  const uword result = reinterpret_cast<uword>(this) + sizeof(*this); \
  ASSERT(Utils::IsAligned(result, sizeof(type)));                     \
  return reinterpret_cast<type*>(result);                             \

and here:
RawAbstractType** type() { OPEN_ARRAY_START(RawAbstractType*); }

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:866: }
int32_t* data() { OPEN_ARRAY_START(int32_t); }

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:1070: // Variable length data follows here.
This comment should be above data() for consistency?

[zra, 2014-06-17 23:17:58]

Since alignment might be different from sizeof(type), the OPEN_ARRAY_START macro
takes two arguments, one for the type and one for the alignment.

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/object.h#newc...
runtime/vm/object.h:3823: return data_offset() + kWordSize * context_index;
On 2014/06/17 22:29:50, siva wrote:
> why not OFFSET_OF_RETURNED_VALUE(RawContext, data) + context_index * kWordSize
?
> 
> and drop data_offset().

Done.

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/object.h#newc...
runtime/vm/object.h:5958: static intptr_t data_offset() { return
sizeof(RawArray); }
On 2014/06/17 22:29:50, siva wrote:
> why not return OFFSET_OF_RETURNED_VALUE(RawArray, data) for consistency?

Done.

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:563: return reinterpret_cast<RawAbstractType**>(result);
On 2014/06/17 22:29:50, siva wrote:
> why not make a macro and use that everywhere?
> e.g
> #define OPEN_ARRAY_START(type)                                        \
>   const uword result = reinterpret_cast<uword>(this) + sizeof(*this); \
>   ASSERT(Utils::IsAligned(result, sizeof(type)));                     \
>   return reinterpret_cast<type*>(result);                             \
> 
> and here:
> RawAbstractType** type() { OPEN_ARRAY_START(RawAbstractType*); }

Done.

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:866: }
On 2014/06/17 22:29:50, siva wrote:
> int32_t* data() { OPEN_ARRAY_START(int32_t); }

Done.

https://codereview.chromium.org/333773006/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:1070: // Variable length data follows here.
On 2014/06/17 22:29:50, siva wrote:
> This comment should be above data() for consistency?

Done.

[zra, 2014-06-18 15:58:35]

Committed patchset #7 manually as r37450 (presubmit successful).