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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by dcheng (OOO Jun 25 - Jul 1)
Modified:
2 weeks 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 (OOO Jun 25 - Jul 1)
Just to get the ball rolling again... anyone have further thoughts or comments on this ...
4 months, 2 weeks 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 ...
4 months, 2 weeks 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 ...
4 months, 2 weeks ago (2017-02-08 21:29:47 UTC) #23
dcheng (OOO Jun 25 - Jul 1)
I also went and filled in some of the tests so they test alignments from ...
4 months, 2 weeks ago (2017-02-09 00:01:24 UTC) #26
danakj
SeemsGood. Oh and cc/ LGTM too.
4 months, 2 weeks ago (2017-02-09 16:16:23 UTC) #29
Nico (vacation Jun 30-Jul 11)
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 ...
4 months, 2 weeks ago (2017-02-10 19:56:53 UTC) #30
dcheng (OOO Jun 25 - Jul 1)
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 ...
4 months, 2 weeks ago (2017-02-10 20:06:06 UTC) #31
Nico (vacation Jun 30-Jul 11)
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 ...
4 months, 2 weeks ago (2017-02-10 20:37:58 UTC) #32
dcheng (OOO Jun 25 - Jul 1)
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 > ...
4 months, 2 weeks ago (2017-02-10 21:43:41 UTC) #33
dcheng (OOO Jun 25 - Jul 1)
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 ...
4 months, 2 weeks ago (2017-02-10 21:46:21 UTC) #34
dcheng (OOO Jun 25 - Jul 1)
+jochen for //components + //content +liberato for //media +juliatuttle for //net +mark for //third_party/crashpad (I'm ...
4 months, 2 weeks 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 ...
4 months, 2 weeks ago (2017-02-14 20:02:17 UTC) #42
dcheng (OOO Jun 25 - Jul 1)
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: ...
4 months, 2 weeks ago (2017-02-14 20:06:53 UTC) #43
dcheng (OOO Jun 25 - Jul 1)
Note: updated the CL description to include details about the problems mixing alignas() with __attribute__(()).
4 months, 2 weeks ago (2017-02-14 22:59:17 UTC) #45
Mark Mentovai
LGTM in crashpad > - crashpad code marks some structs as aligned and packed. The ...
4 months, 2 weeks ago (2017-02-14 23:01:50 UTC) #47
Mark Mentovai
Never mind, I was looking at an old patch set.
4 months, 2 weeks 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 ...
4 months, 2 weeks ago (2017-02-14 23:07:33 UTC) #49
Nico (vacation Jun 30-Jul 11)
The alignof part lgtm for sure. I abstractly dislike alignas due to the quirks you ...
4 months, 2 weeks ago (2017-02-14 23:18:37 UTC) #50
dcheng (OOO Jun 25 - Jul 1)
On 2017/02/14 23:07:33, Mark Mentovai wrote: > On 2017/02/14 23:03:14, Mark Mentovai wrote: > > ...
4 months, 2 weeks ago (2017-02-14 23:34:36 UTC) #51
Nico (vacation Jun 30-Jul 11)
On 2017/02/14 23:34:36, dcheng wrote: > On 2017/02/14 23:07:33, Mark Mentovai wrote: > > On ...
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-15 15:27:25 UTC) #53
jochen
lgtm from my side
4 months, 1 week ago (2017-02-20 09:53:57 UTC) #54
Julia Tuttle
net/ lgtm but maybe someone from quic should look at it.
4 months ago (2017-02-23 20:19:09 UTC) #55
dcheng (OOO Jun 25 - Jul 1)
On 2017/02/15 15:27:25, Mark Mentovai wrote: > On 2017/02/15 15:09:55, Nico wrote: > > On ...
4 months ago (2017-02-23 21:38:03 UTC) #56
Nico (vacation Jun 30-Jul 11)
I lg'd above already. I personally wouldn't land the ALIGNAS part until it works better, ...
4 months ago (2017-02-23 21:40:36 UTC) #57
Julia Tuttle
2 months, 3 weeks 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 23e94e589