Use C++11 alignment primitives

Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced,
and a note is added to the definition about the problems with alignas.
This came from https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with alignas(type) char[size].

std::aligned_storage has some limitations. The style guide is updated to note these and mark it disallowed. It is also updated for alignas and alignof.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2932053002
Cr-Commit-Position: refs/heads/master@{#479169}
Committed: https://chromium.googlesource.com/chromium/src/+/16289b3ef759d892a4058671e0657c016101be23

[brettw, 2017-06-09 05:27:29]

Description was changed from

==========
Alignas

BUG=
==========

to

==========
Alignas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[brettw, 2017-06-09 05:27:43]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 05:27:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 05:38:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 05:39:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2017-06-09 19:10:39]

Linux Chrome

[brettw, 2017-06-09 19:10:39]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 19:11:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 19:33:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 19:33:55]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[brettw, 2017-06-09 19:35:56]

alignas

[brettw, 2017-06-09 19:38:27]

Format

[brettw, 2017-06-09 19:42:02]

Description was changed from

==========
Alignas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use C++11 alignment primitives

Removes the compiler-specific base #defines ALIGNAS and ALIGNOF and replace uses
with alignas and alignof.

Remove base::AlignedMemory and replace with std::aligned_storage.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[brettw, 2017-06-09 19:42:41]

brettw@chromium.org changed reviewers:
+ danakj@chromium.org

[brettw, 2017-06-09 19:42:42]

scottmg: third_party/crashpad
danakj: everything else (it's mostly base and cc)

[brettw, 2017-06-09 19:42:50]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 19:43:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-06-09 19:49:00]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2017-06-09 19:49:01]

https://codereview.chromium.org/2932053002/diff/60001/third_party/crashpad/cr...
File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right):

https://codereview.chromium.org/2932053002/diff/60001/third_party/crashpad/cr...
third_party/crashpad/crashpad/minidump/minidump_extensions.h:105: struct
alignas(4) PACKED MinidumpUTF8String {
There's a bunch more ALIGNAS in this file.

I can do this upstream and roll if you want instead. Is there a bug for this
work?

[Mark Mentovai, 2017-06-09 20:03:15]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2017-06-09 20:03:16]

Note a previous attempt to do this months ago,
https://codereview.chromium.org/2670873002/.

[Mark Mentovai, 2017-06-09 20:03:24]

mark@chromium.org changed reviewers:
- mark@chromium.org

[commit-bot: I haz the power, 2017-06-09 20:09:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 20:09:06]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[brettw, 2017-06-09 20:29:23]

:( I wasn't aware of the previous attempt. I'll trim this down (at least we can
delete base::AlignedMemory)

[danakj, 2017-06-09 20:34:32]

On Fri, Jun 9, 2017 at 4:29 PM, <brettw@chromium.org> wrote:

> :( I wasn't aware of the previous attempt. I'll trim this down (at least
> we can
> delete base::AlignedMemory)
>

I'll wait to review for that then


>
> https://codereview.chromium.org/2932053002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-06-09 20:41:59]

On Fri, Jun 9, 2017 at 4:29 PM, <brettw@chromium.org> wrote:

> :( I wasn't aware of the previous attempt. I'll trim this down (at least
> we can
> delete base::AlignedMemory)
>

I'll wait to review for that then


>
> https://codereview.chromium.org/2932053002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[brettw, 2017-06-10 15:29:56]

Put back ALIGNAS

[brettw, 2017-06-10 15:31:08]

Description was changed from

==========
Use C++11 alignment primitives

Removes the compiler-specific base #defines ALIGNAS and ALIGNOF and replace uses
with alignas and alignof.

Remove base::AlignedMemory and replace with std::aligned_storage.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with 
alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced, and a
note is added to the definition about the problems with alignas. This came from
https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with std::aligned_storage.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[brettw, 2017-06-10 15:32:07]

Okay, I put back the ALIGNAS definition and kept using it in a few cases where
it was combined with other things.

[brettw, 2017-06-10 15:32:13]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-10 15:32:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2017-06-10 17:27:45]

brettw@chromium.org changed reviewers:
+ dcheng@chromium.org

[brettw, 2017-06-10 17:27:46]

+dcheng FYI who wrote the last patch.

[commit-bot: I haz the power, 2017-06-10 17:31:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-10 17:31:50]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-06-10 17:43:17]

Note that the reason I never landed the previous patch is thakis@ felt there
were too many gotchas around compiler support for mixing alignas with other
attributes.

[Nico, 2017-06-10 17:57:57]

Description was changed from

==========
Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with 
alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced, and a
note is added to the definition about the problems with alignas. This came from
https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with std::aligned_storage.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with
alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced,
and a note is added to the definition about the problems with alignas.
This came from https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with std::aligned_storage.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Nico, 2017-06-10 17:58:23]

On 2017/06/10 17:43:17, dcheng wrote:
> Note that the reason I never landed the previous patch is thakis@ felt there
> were too many gotchas around compiler support for mixing alignas with other
> attributes.

Yup, patch set 5 retains ALIGNAS for that reason.

This patch lgtm.

[dcheng, 2017-06-10 18:15:33]

On 2017/06/10 17:58:23, Nico (vacation Jun 3-11) wrote:
> On 2017/06/10 17:43:17, dcheng wrote:
> > Note that the reason I never landed the previous patch is thakis@ felt there
> > were too many gotchas around compiler support for mixing alignas with other
> > attributes.
> 
> Yup, patch set 5 retains ALIGNAS for that reason.
> 
> This patch lgtm.

Should we update the c++11 style guide too? I didn't want to split up the
alignment directives originally, but I guess they serve pretty different
purposes anyway.

[brettw, 2017-06-11 16:02:11]

There seems to be some problem on Windows with aligned_storage I need to track
down :(

[danakj, 2017-06-12 18:32:10]

cc and base LGTM

https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific....
base/compiler_specific.h:129: // these attributes. This means that it is not
possible to have use alignas()
typo: "to have use"

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
File base/containers/stack_container.h (right):

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
base/containers/stack_container.h:57: typename
std::aligned_storage<sizeof(T[stack_capacity]), alignof(T)>::type
Since you're having issues maybe we can/should split the removal of ALIGNOF()
from the removal of base::AlignedMemory?

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
File base/containers/stack_container_unittest.cc (right):

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
base/containers/stack_container_unittest.cc:112: typename
std::aligned_storage<alignment, alignment>::type data_;
C++14 adds this (http://en.cppreference.com/w/cpp/types/aligned_storage):

template< std::size_t Len, std::size_t Align = /*default-alignment*/ >
using aligned_storage_t = typename aligned_storage<Len, Align>::type;

So we don't have to do this typename ::type dance. WDYT of adding a
base::aligned_storage_t to template_util in the meantime?

[brettw, 2017-06-12 19:34:47]

Exciting times: std::aligned_storage won't work for anything larger than 8 in
MS's STL implementation.

It's kind of elaborately written to avoid alignas (which would work) as well as
their attribute, but instead construct a type of the corresponding size. I would
speculate this was a relatively old implementation aiming to be cross-platform
or work on older compilers.

I will remove this and update the style guide to mark it as "avoid".

[brettw, 2017-06-12 21:41:33]

No aligned_storage

[brettw, 2017-06-12 21:54:32]

Fixes

[brettw, 2017-06-12 21:55:23]

Description was changed from

==========
Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with
alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced,
and a note is added to the definition about the problems with alignas.
This came from https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with std::aligned_storage.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with
alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced,
and a note is added to the definition about the problems with alignas.
This came from https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with alignas(type) char[size].

std::aligned_storage has some limitations. The style guide is updated to note
these and mark it disallowed. It is also updated for alignas and alignof.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[brettw, 2017-06-12 21:56:03]

Okay, the new version has no aligned_storage. This should be pretty trivial.

The main difference here is the modifications to the style guide. C++ folks
should check these.

[brettw, 2017-06-12 21:56:11]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 21:56:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 22:00:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 22:00:50]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[brettw, 2017-06-12 22:17:13]

Merge

[brettw, 2017-06-12 22:17:29]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 22:18:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-06-12 22:22:05]

https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_...
File base/memory/singleton_unittest.cc (right):

https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_...
base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4);
Is this legit? I realize AlignedData has only one member, but couldn't that
member be aligned differently than the AlignedData struct itself, with some
padding? Or is that not possible.

[brettw, 2017-06-12 22:22:15]

https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific....
base/compiler_specific.h:129: // these attributes. This means that it is not
possible to have use alignas()
On 2017/06/12 18:32:10, danakj wrote:
> typo: "to have use"

I did this but haven't uploaded yet since I assume there will be another issue
and I'll have to do a new patchset.

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
File base/containers/stack_container_unittest.cc (right):

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
base/containers/stack_container_unittest.cc:112: typename
std::aligned_storage<alignment, alignment>::type data_;
I just replaced these all with char arrays. This seems clearer to me anyway, and
the code is shorter.

[danakj, 2017-06-12 22:23:10]

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
File base/containers/stack_container_unittest.cc (right):

https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_c...
base/containers/stack_container_unittest.cc:112: typename
std::aligned_storage<alignment, alignment>::type data_;
On 2017/06/12 22:22:15, brettw wrote:
> I just replaced these all with char arrays. This seems clearer to me anyway,
and
> the code is shorter.

Me too, that is A+

[brettw, 2017-06-12 22:28:50]

https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_...
File base/memory/singleton_unittest.cc (right):

https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_...
base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4);
On 2017/06/12 22:22:05, danakj wrote:
> Is this legit? I realize AlignedData has only one member, but couldn't that
> member be aligned differently than the AlignedData struct itself, with some
> padding? Or is that not possible.

I wondered this also. I had even changed it at one point to reference the data
inside here. But even if there was padding the outer structure would always have
to be aligned at the same interval as the inner one to guarantee that the inner
one is aligned properly. And I don't think it makes sense to have a struct
aligned at "8-byte offsets starting at 4" (if this makes sense). So I think the
only reasonable thing is for the compilers to align the outer struct the same.

[commit-bot: I haz the power, 2017-06-12 22:30:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 22:30:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2017-06-12 22:44:45]

Fixes

[brettw, 2017-06-12 22:44:46]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 22:45:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-06-12 23:33:27]

On Mon, Jun 12, 2017 at 6:28 PM, <brettw@chromium.org> wrote:

>
> https://codereview.chromium.org/2932053002/diff/120001/
> base/memory/singleton_unittest.cc
> File base/memory/singleton_unittest.cc (right):
>
> https://codereview.chromium.org/2932053002/diff/120001/
> base/memory/singleton_unittest.cc#newcode292
> base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4);
> On 2017/06/12 22:22:05, danakj wrote:
> > Is this legit? I realize AlignedData has only one member, but couldn't
> that
> > member be aligned differently than the AlignedData struct itself, with
> some
> > padding? Or is that not possible.
>
> I wondered this also. I had even changed it at one point to reference
> the data inside here. But even if there was padding the outer structure
> would always have to be aligned at the same interval as the inner one to
> guarantee that the inner one is aligned properly. And I don't think it
> makes sense to have a struct aligned at "8-byte offsets starting at 4"
> (if this makes sense). So I think the only reasonable thing is for the
> compilers to align the outer struct the same.
>

If that's what the spec says it has to do then ok, otherwise I think we
should check the alignment of the field. That would also be more clear IMO
as testing that the aligned field is aligned, rather than sideeffects.


>
> https://codereview.chromium.org/2932053002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-06-12 23:33:27]

On Mon, Jun 12, 2017 at 6:28 PM, <brettw@chromium.org> wrote:

>
> https://codereview.chromium.org/2932053002/diff/120001/
> base/memory/singleton_unittest.cc
> File base/memory/singleton_unittest.cc (right):
>
> https://codereview.chromium.org/2932053002/diff/120001/
> base/memory/singleton_unittest.cc#newcode292
> base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4);
> On 2017/06/12 22:22:05, danakj wrote:
> > Is this legit? I realize AlignedData has only one member, but couldn't
> that
> > member be aligned differently than the AlignedData struct itself, with
> some
> > padding? Or is that not possible.
>
> I wondered this also. I had even changed it at one point to reference
> the data inside here. But even if there was padding the outer structure
> would always have to be aligned at the same interval as the inner one to
> guarantee that the inner one is aligned properly. And I don't think it
> makes sense to have a struct aligned at "8-byte offsets starting at 4"
> (if this makes sense). So I think the only reasonable thing is for the
> compilers to align the outer struct the same.
>

If that's what the spec says it has to do then ok, otherwise I think we
should check the alignment of the field. That would also be more clear IMO
as testing that the aligned field is aligned, rather than sideeffects.


>
> https://codereview.chromium.org/2932053002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2017-06-12 23:33:56]

LGTM otherwise, incl styleguide

[commit-bot: I haz the power, 2017-06-13 02:04:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 02:04:59]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-06-13 04:43:57]

On 2017/06/12 23:33:27, danakj wrote:
> On Mon, Jun 12, 2017 at 6:28 PM, <mailto:brettw@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2932053002/diff/120001/
> > base/memory/singleton_unittest.cc
> > File base/memory/singleton_unittest.cc (right):
> >
> > https://codereview.chromium.org/2932053002/diff/120001/
> > base/memory/singleton_unittest.cc#newcode292
> > base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4);
> > On 2017/06/12 22:22:05, danakj wrote:
> > > Is this legit? I realize AlignedData has only one member, but couldn't
> > that
> > > member be aligned differently than the AlignedData struct itself, with
> > some
> > > padding? Or is that not possible.
> >
> > I wondered this also. I had even changed it at one point to reference
> > the data inside here. But even if there was padding the outer structure
> > would always have to be aligned at the same interval as the inner one to
> > guarantee that the inner one is aligned properly. And I don't think it
> > makes sense to have a struct aligned at "8-byte offsets starting at 4"
> > (if this makes sense). So I think the only reasonable thing is for the
> > compilers to align the outer struct the same.
> >
> 
> If that's what the spec says it has to do then ok, otherwise I think we
> should check the alignment of the field. That would also be more clear IMO
> as testing that the aligned field is aligned, rather than sideeffects.

From Stack Overflow:

From the C99 standard section 6.7.2.1 bullet point 13: Within a structure
object, the non-bit-field members and the units in which bit-fields reside have
addresses that increase in the order in which they are declared. A pointer to a
structure object, suitably converted, points to its initial member (or if that
member is a bit-field, then to the unit in which it resides), and vice versa.
There may be unnamed padding within a structure object, but not at its
beginning.

So I think this is well-defined (I don't think C++ would diverge here).

[brettw, 2017-06-13 04:44:05]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-13 04:44:05]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2932053002/#ps160001
(title: "Fixes")

[commit-bot: I haz the power, 2017-06-13 04:44:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 04:53:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 04:53:29]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[(unused - use chromium), 2017-06-13 13:13:33]

thakis@google.com changed reviewers:
+ mark@chromium.org, thakis@google.com

[(unused - use chromium), 2017-06-13 13:13:34]

mark, can you lg-stamp crashpad?

[danakj, 2017-06-13 14:30:52]

On Tue, Jun 13, 2017 at 12:43 AM, <brettw@chromium.org> wrote:

> On 2017/06/12 23:33:27, danakj wrote:
> > On Mon, Jun 12, 2017 at 6:28 PM, <mailto:brettw@chromium.org> wrote:
> >
> > >
> > > https://codereview.chromium.org/2932053002/diff/120001/
> > > base/memory/singleton_unittest.cc
> > > File base/memory/singleton_unittest.cc (right):
> > >
> > > https://codereview.chromium.org/2932053002/diff/120001/
> > > base/memory/singleton_unittest.cc#newcode292
> > > base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4);
> > > On 2017/06/12 22:22:05, danakj wrote:
> > > > Is this legit? I realize AlignedData has only one member, but
> couldn't
> > > that
> > > > member be aligned differently than the AlignedData struct itself,
> with
> > > some
> > > > padding? Or is that not possible.
> > >
> > > I wondered this also. I had even changed it at one point to reference
> > > the data inside here. But even if there was padding the outer structure
> > > would always have to be aligned at the same interval as the inner one
> to
> > > guarantee that the inner one is aligned properly. And I don't think it
> > > makes sense to have a struct aligned at "8-byte offsets starting at 4"
> > > (if this makes sense). So I think the only reasonable thing is for the
> > > compilers to align the outer struct the same.
> > >
> >
> > If that's what the spec says it has to do then ok, otherwise I think we
> > should check the alignment of the field. That would also be more clear
> IMO
> > as testing that the aligned field is aligned, rather than sideeffects.
>
> From Stack Overflow:
>
> From the C99 standard section 6.7.2.1 bullet point 13: Within a structure
> object, the non-bit-field members and the units in which bit-fields reside
> have
> addresses that increase in the order in which they are declared. A pointer
> to a
> structure object, suitably converted, points to its initial member (or if
> that
> member is a bit-field, then to the unit in which it resides), and vice
> versa.
> There may be unnamed padding within a structure object, but not at its
> beginning.
>
> So I think this is well-defined (I don't think C++ would diverge here).
>

Cool, thanks!


>
> https://codereview.chromium.org/2932053002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-06-13 14:30:52]

On Tue, Jun 13, 2017 at 12:43 AM, <brettw@chromium.org> wrote:

> On 2017/06/12 23:33:27, danakj wrote:
> > On Mon, Jun 12, 2017 at 6:28 PM, <mailto:brettw@chromium.org> wrote:
> >
> > >
> > > https://codereview.chromium.org/2932053002/diff/120001/
> > > base/memory/singleton_unittest.cc
> > > File base/memory/singleton_unittest.cc (right):
> > >
> > > https://codereview.chromium.org/2932053002/diff/120001/
> > > base/memory/singleton_unittest.cc#newcode292
> > > base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4);
> > > On 2017/06/12 22:22:05, danakj wrote:
> > > > Is this legit? I realize AlignedData has only one member, but
> couldn't
> > > that
> > > > member be aligned differently than the AlignedData struct itself,
> with
> > > some
> > > > padding? Or is that not possible.
> > >
> > > I wondered this also. I had even changed it at one point to reference
> > > the data inside here. But even if there was padding the outer structure
> > > would always have to be aligned at the same interval as the inner one
> to
> > > guarantee that the inner one is aligned properly. And I don't think it
> > > makes sense to have a struct aligned at "8-byte offsets starting at 4"
> > > (if this makes sense). So I think the only reasonable thing is for the
> > > compilers to align the outer struct the same.
> > >
> >
> > If that's what the spec says it has to do then ok, otherwise I think we
> > should check the alignment of the field. That would also be more clear
> IMO
> > as testing that the aligned field is aligned, rather than sideeffects.
>
> From Stack Overflow:
>
> From the C99 standard section 6.7.2.1 bullet point 13: Within a structure
> object, the non-bit-field members and the units in which bit-fields reside
> have
> addresses that increase in the order in which they are declared. A pointer
> to a
> structure object, suitably converted, points to its initial member (or if
> that
> member is a bit-field, then to the unit in which it resides), and vice
> versa.
> There may be unnamed padding within a structure object, but not at its
> beginning.
>
> So I think this is well-defined (I don't think C++ would diverge here).
>

Cool, thanks!


>
> https://codereview.chromium.org/2932053002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Mark Mentovai, 2017-06-13 14:46:11]

LGTM in crashpad, and we’ll carry this upstream.

[Nico, 2017-06-13 14:49:37]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-06-13 14:49:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 14:59:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 14:59:44]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2017-06-13 18:08:25]

Merge

[brettw, 2017-06-13 18:08:29]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-13 18:08:29]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
mark@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2932053002/#ps180001
(title: "Merge")

[commit-bot: I haz the power, 2017-06-13 18:09:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 18:12:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 18:12:42]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2017-06-13 18:22:31]

Merge

[brettw, 2017-06-13 18:22:34]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-13 18:22:34]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
mark@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2932053002/#ps200001
(title: "Merge")

[commit-bot: I haz the power, 2017-06-13 18:23:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 18:37:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 18:37:49]

Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2017-06-13 19:08:44]

Fix merge

[brettw, 2017-06-13 19:08:46]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-13 19:08:46]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
mark@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2932053002/#ps220001
(title: "Fix merge")

[commit-bot: I haz the power, 2017-06-13 19:09:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 21:58:45]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1497380926084270,
"parent_rev": "c5b8d597dda99a31ae014519984d51c994f3f55d", "commit_rev":
"16289b3ef759d892a4058671e0657c016101be23"}

[commit-bot: I haz the power, 2017-06-13 21:58:57]

Description was changed from

==========
Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with
alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced,
and a note is added to the definition about the problems with alignas.
This came from https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with alignas(type) char[size].

std::aligned_storage has some limitations. The style guide is updated to note
these and mark it disallowed. It is also updated for alignas and alignof.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use C++11 alignment primitives

Removes the compiler-specific base #define ALIGNOF and replace uses with
alignof.

Replaces some uses of ALIGNAS with alignas. These are not all replaced,
and a note is added to the definition about the problems with alignas.
This came from https://codereview.chromium.org/2670873002/

Remove base::AlignedMemory and replace with alignas(type) char[size].

std::aligned_storage has some limitations. The style guide is updated to note
these and mark it disallowed. It is also updated for alignas and alignof.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2932053002
Cr-Commit-Position: refs/heads/master@{#479169}
Committed:
https://chromium.googlesource.com/chromium/src/+/16289b3ef759d892a4058671e065...
==========

[commit-bot: I haz the power, 2017-06-13 21:59:04]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/16289b3ef759d892a4058671e065...