Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(155)

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

Created:
3 years, 10 months ago by dcheng
Modified:
3 years, 6 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

Messages

Total messages: 64 (38 generated)
dcheng
Just to get the ball rolling again... anyone have further thoughts or comments on this ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-09 00:01:24 UTC) #26
danakj
SeemsGood. Oh and cc/ LGTM too.
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 > ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-14 19:13:46 UTC) #41
liberato (no reviews please)
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 ...
3 years, 10 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: ...
3 years, 10 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__(()).
3 years, 10 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 ...
3 years, 10 months ago (2017-02-14 23:01:50 UTC) #47
Mark Mentovai
Never mind, I was looking at an old patch set.
3 years, 10 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 ...
3 years, 10 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 ...
3 years, 10 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: > > ...
3 years, 10 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 ...
3 years, 10 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 ...
3 years, 10 months ago (2017-02-15 15:27:25 UTC) #53
jochen (gone - plz use gerrit)
lgtm from my side
3 years, 10 months ago (2017-02-20 09:53:57 UTC) #54
Julia Tuttle
net/ lgtm but maybe someone from quic should look at it.
3 years, 10 months 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 ...
3 years, 10 months 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, ...
3 years, 10 months ago (2017-02-23 21:40:36 UTC) #57
Julia Tuttle
3 years, 8 months ago (2017-04-07 20:16:18 UTC) #63
Dropping this from my dashboard since it's been lying around a while.

Powered by Google App Engine
This is Rietveld 408576698