Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2670873002: Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 2 weeks ago by dcheng
Modified:
2 months ago
CC:
asvitkine+watch_chromium.org, cbentzel+watch_chromium.org, cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, feature-media-reviews_chromium.org, gavinp+memory_chromium.org, jam, jbroman+cpp_chromium.org, jln+watch_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, net-reviews_chromium.org, pfeldman, posciak+watch_chromium.org, tracing+reviews_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, xjz+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas. Note that compilers have trouble mixing __attribute__((...)) syntax with alignas(...) syntax. // doesn't work in clang or gcc struct alignas(16) __attribute__((packed)) S { char c; }; // works in clang but not gcc struct __attribute__((packed)) alignas(16) S2 { char c; }; // works in clang and gcc struct alignas(16) S3 { char c; } __attribute__((packed)); Note that there are also some attributes that must be specified *before* a class definition: visibility (used for exporting functions/classes) is one of these attributes. This means that it is not possible to have use alignas() with a class that is marked as exported. In the Chromium codebase, there were two locations that encountered these issues: - media code marks some structs as aligned and exported. However, since these structs are simply PODs and the full definition is in the header, marking them as exported is unnecessary. The fix is to remove the exported attribute. - crashpad code marks some structs as aligned and packed. The fix is to simply move the packed attribute after the definition, verifying that clang/gcc still pack the structs the same way. If there are future instances of structs/classes that need to be aligned and exported, the plan is to move to C++11 attribute syntax for visibility attributes, which does work with alignas(). BUG=688061 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Patch Set 4 : crashpad #

Patch Set 5 : Fix Mac, maybe #

Patch Set 6 : #if / #endif all the things #

Total comments: 7

Patch Set 7 : Tests #

Total comments: 3

Patch Set 8 : rebase #

Total comments: 2

Patch Set 9 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -174 lines) Patch
M base/compiler_specific.h View 1 chunk +0 lines, -19 lines 0 comments Download
M base/containers/stack_container.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M base/containers/stack_container_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M base/lazy_instance.h View 1 2 2 chunks +5 lines, -5 lines 0 comments Download
M base/lazy_instance_unittest.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M base/memory/aligned_memory.h View 2 chunks +17 lines, -43 lines 0 comments Download
M base/memory/aligned_memory_unittest.cc View 1 2 3 4 5 6 1 chunk +21 lines, -37 lines 0 comments Download
M base/memory/manual_constructor.h View 2 chunks +2 lines, -2 lines 0 comments Download
M base/memory/singleton.h View 1 chunk +2 lines, -2 lines 0 comments Download
M base/memory/singleton_unittest.cc View 1 2 3 4 5 6 1 chunk +9 lines, -6 lines 0 comments Download
M base/metrics/persistent_memory_allocator.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M base/process/launch_posix.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/base/contiguous_container.h View 1 chunk +1 line, -1 line 0 comments Download
M cc/base/contiguous_container_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/raster/texture_compressor_etc1.h View 1 chunk +4 lines, -9 lines 0 comments Download
M cc/raster/texture_compressor_etc1_sse.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/tracing/core/proto_zero_message.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/media/audio_input_sync_writer_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -3 lines 0 comments Download
M content/renderer/devtools/lock_free_circular_queue.h View 2 chunks +2 lines, -2 lines 0 comments Download
M media/base/audio_parameters.h View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M net/quic/platform/impl/quic_aligned_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M sandbox/linux/services/credentials.cc View 1 2 3 5 6 1 chunk +1 line, -1 line 0 comments Download
M styleguide/c++/c++11.html View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -8 lines 0 comments Download
M third_party/crashpad/crashpad/minidump/minidump_context.h View 1 2 3 2 chunks +1 line, -2 lines 0 comments Download
M third_party/crashpad/crashpad/minidump/minidump_extensions.h View 1 2 3 4 11 chunks +16 lines, -16 lines 0 comments Download
M third_party/crashpad/crashpad/util/stdlib/aligned_allocator.h View 1 2 3 3 chunks +2 lines, -3 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 64 (38 generated)
dcheng
Just to get the ball rolling again... anyone have further thoughts or comments on this ...
6 months, 1 week ago (2017-02-08 18:00:38 UTC) #21
jbroman
lgtm https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance.h File base/lazy_instance.h (right): https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance.h#newcode63 base/lazy_instance.h:63: DCHECK_EQ(reinterpret_cast<uintptr_t>(instance) & (alignof(Type) - 1), 0u) I'm not ...
6 months, 1 week ago (2017-02-08 18:21:35 UTC) #22
danakj
base/ LGTM https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance_unittest.cc File base/lazy_instance_unittest.cc (right): https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance_unittest.cc#newcode169 base/lazy_instance_unittest.cc:169: // GCC doesn't like alignment >64 on ...
6 months, 1 week ago (2017-02-08 21:29:47 UTC) #23
dcheng
I also went and filled in some of the tests so they test alignments from ...
6 months, 1 week ago (2017-02-09 00:01:24 UTC) #26
danakj
SeemsGood. Oh and cc/ LGTM too.
6 months, 1 week ago (2017-02-09 16:16:23 UTC) #29
Nico
https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right): https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h#newcode252 third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED; doesn't putting attributes at the end usually ...
6 months, 1 week ago (2017-02-10 19:56:53 UTC) #30
dcheng
https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right): https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h#newcode252 third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED; On 2017/02/10 19:56:53, Nico wrote: > doesn't ...
6 months, 1 week ago (2017-02-10 20:06:06 UTC) #31
Nico
https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right): https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h#newcode252 third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED; On 2017/02/10 20:06:06, dcheng wrote: > On ...
6 months, 1 week ago (2017-02-10 20:37:58 UTC) #32
dcheng
On 2017/02/10 20:37:58, Nico wrote: > https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h > File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right): > > https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h#newcode252 > ...
6 months, 1 week ago (2017-02-10 21:43:41 UTC) #33
dcheng
On 2017/02/10 21:43:41, dcheng wrote: > On 2017/02/10 20:37:58, Nico wrote: > > > https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/crashpad/minidump/minidump_extensions.h ...
6 months, 1 week ago (2017-02-10 21:46:21 UTC) #34
dcheng
+jochen for //components + //content +liberato for //media +juliatuttle for //net +mark for //third_party/crashpad (I'm ...
6 months ago (2017-02-14 19:13:46 UTC) #41
liberato
media lgtm % two missing MEDIA_EXPORTs. https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_parameters.h File media/base/audio_parameters.h (right): https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_parameters.h#newcode35 media/base/audio_parameters.h:35: struct alignas(PARAMETERS_ALIGNMENT) AudioInputBufferParameters ...
6 months ago (2017-02-14 20:02:17 UTC) #42
dcheng
https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_parameters.h File media/base/audio_parameters.h (right): https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_parameters.h#newcode35 media/base/audio_parameters.h:35: struct alignas(PARAMETERS_ALIGNMENT) AudioInputBufferParameters { On 2017/02/14 20:02:17, liberato wrote: ...
6 months ago (2017-02-14 20:06:53 UTC) #43
dcheng
Note: updated the CL description to include details about the problems mixing alignas() with __attribute__(()).
6 months ago (2017-02-14 22:59:17 UTC) #45
Mark Mentovai
LGTM in crashpad > - crashpad code marks some structs as aligned and packed. The ...
6 months ago (2017-02-14 23:01:50 UTC) #47
Mark Mentovai
Never mind, I was looking at an old patch set.
6 months ago (2017-02-14 23:03:14 UTC) #48
Mark Mentovai
On 2017/02/14 23:03:14, Mark Mentovai wrote: > Never mind, I was looking at an old ...
6 months ago (2017-02-14 23:07:33 UTC) #49
Nico
The alignof part lgtm for sure. I abstractly dislike alignas due to the quirks you ...
6 months ago (2017-02-14 23:18:37 UTC) #50
dcheng
On 2017/02/14 23:07:33, Mark Mentovai wrote: > On 2017/02/14 23:03:14, Mark Mentovai wrote: > > ...
6 months ago (2017-02-14 23:34:36 UTC) #51
Nico
On 2017/02/14 23:34:36, dcheng wrote: > On 2017/02/14 23:07:33, Mark Mentovai wrote: > > On ...
6 months ago (2017-02-15 15:09:55 UTC) #52
Mark Mentovai
On 2017/02/15 15:09:55, Nico wrote: > On 2017/02/14 23:34:36, dcheng wrote: > > On 2017/02/14 ...
6 months ago (2017-02-15 15:27:25 UTC) #53
jochen (gone - plz use gerrit)
lgtm from my side
5 months, 3 weeks ago (2017-02-20 09:53:57 UTC) #54
Julia Tuttle
net/ lgtm but maybe someone from quic should look at it.
5 months, 3 weeks ago (2017-02-23 20:19:09 UTC) #55
dcheng
On 2017/02/15 15:27:25, Mark Mentovai wrote: > On 2017/02/15 15:09:55, Nico wrote: > > On ...
5 months, 3 weeks ago (2017-02-23 21:38:03 UTC) #56
Nico
I lg'd above already. I personally wouldn't land the ALIGNAS part until it works better, ...
5 months, 3 weeks ago (2017-02-23 21:40:36 UTC) #57
Julia Tuttle
4 months, 1 week ago (2017-04-07 20:16:18 UTC) #63
Dropping this from my dashboard since it's been lying around a while.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b