Make sure that ScratchBuffer::Allocate() always return 8-byte aligned address.

Make sure that ScratchBuffer::Allocate() always return 8-byte aligned addresses.

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268405

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268567

[yzshen1, 2014-05-05 22:53:25]

Hi, Darin.

Would you please take a look? Thanks!

[darin (slow to review), 2014-05-05 22:55:52]

LGTM

https://codereview.chromium.org/265403003/diff/1/mojo/public/cpp/bindings/lib...
File mojo/public/cpp/bindings/lib/scratch_buffer.cc (right):

https://codereview.chromium.org/265403003/diff/1/mojo/public/cpp/bindings/lib...
mojo/public/cpp/bindings/lib/scratch_buffer.cc:88: segment->cursor =
reinterpret_cast<char*>(segment) + padded_segment_size;
good catch!

[darin (slow to review), 2014-05-05 22:55:52]

LGTM

[yzshen1, 2014-05-05 22:57:46]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-05-05 22:59:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/265403003/1

[commit-bot: I haz the power, 2014-05-06 03:50:44]

Change committed as 268405

[Mike West, 2014-05-06 07:03:42]

A revert of this CL has been created in
https://codereview.chromium.org/262093011/ by mkwst@chromium.org.

The reason for reverting is: This broke mojo unit tests under linux dbg: see
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...
for detail..

[yzshen1, 2014-05-06 07:23:08]

Hi, Darin.

Would you please take another look at patchset #2?

Thanks!

[darin (slow to review), 2014-05-06 07:32:54]

LGTM

https://codereview.chromium.org/265403003/diff/20001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/scratch_buffer.cc (right):

https://codereview.chromium.org/265403003/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/scratch_buffer.cc:28: fixed_.cursor =
internal::AlignPointer(fixed_data_);
maybe we should align fixed_.end too?

doesn't this mean that we waste a few bytes of fixed_data_ on 32-bit systems?
maybe if we moved the overflow_ pointer to be listed before fixed_data_ that
would avoid that minor issue?

[darin (slow to review), 2014-05-06 07:35:28]

https://codereview.chromium.org/265403003/diff/20001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/scratch_buffer.cc (right):

https://codereview.chromium.org/265403003/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/scratch_buffer.cc:28: fixed_.cursor =
internal::AlignPointer(fixed_data_);
On 2014/05/06 07:32:55, darin wrote:
> maybe we should align fixed_.end too?

nevermind. that would increase fixed_.end in some cases, which would be bad.

[yzshen1, 2014-05-06 08:06:43]

Thanks, Darin!

https://codereview.chromium.org/265403003/diff/20001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/scratch_buffer.cc (right):

https://codereview.chromium.org/265403003/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/scratch_buffer.cc:28: fixed_.cursor =
internal::AlignPointer(fixed_data_);
On 2014/05/06 07:32:55, darin wrote:
> maybe we should align fixed_.end too?
> 
> doesn't this mean that we waste a few bytes of fixed_data_ on 32-bit systems?
> maybe if we moved the overflow_ pointer to be listed before fixed_data_ that
> would avoid that minor issue?

On 32-bit systems, it is possible that pointers are 4-byte aligned, therefore,
this whole object may be 4-byte aligned, right? In that case, no matter how we
reorder the fields, we cannot guarantee that |fixed_data_| is 8-byte aligned. (I
probably need to double-check the differences between platforms/compilers.)

[darin (slow to review), 2014-05-06 08:24:04]

That's a good point. Although I thought it was common for compilers to
align structures to 8-bytes. Hmm.... maybe not worth worrying about. The
current path LGTM.


On Tue, May 6, 2014 at 1:06 AM, <yzshen@chromium.org> wrote:

> Thanks, Darin!
>
>
>
> https://codereview.chromium.org/265403003/diff/20001/mojo/
> public/cpp/bindings/lib/scratch_buffer.cc
> File mojo/public/cpp/bindings/lib/scratch_buffer.cc (right):
>
> https://codereview.chromium.org/265403003/diff/20001/mojo/
> public/cpp/bindings/lib/scratch_buffer.cc#newcode28
> mojo/public/cpp/bindings/lib/scratch_buffer.cc:28: fixed_.cursor =
> internal::AlignPointer(fixed_data_);
> On 2014/05/06 07:32:55, darin wrote:
>
>> maybe we should align fixed_.end too?
>>
>
>  doesn't this mean that we waste a few bytes of fixed_data_ on 32-bit
>>
> systems?
>
>> maybe if we moved the overflow_ pointer to be listed before
>>
> fixed_data_ that
>
>> would avoid that minor issue?
>>
>
> On 32-bit systems, it is possible that pointers are 4-byte aligned,
> therefore, this whole object may be 4-byte aligned, right? In that case,
> no matter how we reorder the fields, we cannot guarantee that
> |fixed_data_| is 8-byte aligned. (I probably need to double-check the
> differences between platforms/compilers.)
>
> https://codereview.chromium.org/265403003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-05-06 17:17:52]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-05-06 17:18:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/265403003/20001

[commit-bot: I haz the power, 2014-05-06 18:25:51]

Change committed as 268567