Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

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

[dcheng, 2017-02-02 19:03:09]

Description was changed from

==========
Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

BUG=688061
==========

to

==========
Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

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

[dcheng, 2017-02-02 19:05:29]

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

[commit-bot: I haz the power, 2017-02-02 19:05:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-02-02 19:10:01]

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

[commit-bot: I haz the power, 2017-02-02 19:10:17]

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-02-02 19:39:11]

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

[commit-bot: I haz the power, 2017-02-02 19:39:12]

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

[dcheng, 2017-02-06 03:22:40]

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

[commit-bot: I haz the power, 2017-02-06 03:22:47]

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-02-06 03:52:12]

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

[commit-bot: I haz the power, 2017-02-06 03:52:13]

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

[dcheng, 2017-02-06 04:41:22]

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

[commit-bot: I haz the power, 2017-02-06 04:41:43]

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-02-06 05:08:46]

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

[commit-bot: I haz the power, 2017-02-06 05:08:47]

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

[dcheng, 2017-02-06 09:49:02]

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

[commit-bot: I haz the power, 2017-02-06 09:49:13]

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-02-06 12:10:48]

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

[commit-bot: I haz the power, 2017-02-06 12:10:49]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-08 18:00:37]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org, jbroman@chromium.org, thakis@chromium.org

[dcheng, 2017-02-08 18:00:38]

Just to get the ball rolling again... anyone have further thoughts or comments
on this beyond jbroman's last reply on cxx@chromium.org?

[jbroman, 2017-02-08 18:21:35]

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#n...
base/lazy_instance.h:63: DCHECK_EQ(reinterpret_cast<uintptr_t>(instance) &
(alignof(Type) - 1), 0u)
I'm not sure why this isn't just reinterpret_cast<uintptr_t>(instance) %
alignof(Type), which seems clearer than using the bitwise trick, but it's not
worth changing.

[danakj, 2017-02-08 21:29:47]

base/ LGTM

https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance_uni...
File base/lazy_instance_unittest.cc (right):

https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance_uni...
base/lazy_instance_unittest.cc:169: // GCC doesn't like alignment >64 on ARM.
Honest question is it worth testing this if nothing is using it what is the
point?

https://codereview.chromium.org/2670873002/diff/100001/base/memory/aligned_me...
File base/memory/aligned_memory_unittest.cc (right):

https://codereview.chromium.org/2670873002/diff/100001/base/memory/aligned_me...
base/memory/aligned_memory_unittest.cc:22: // GCC doesn't like alignment >64 on
ARM.
Same Q. Should we test 64 instead?

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

https://codereview.chromium.org/2670873002/diff/100001/base/memory/singleton_...
base/memory/singleton_unittest.cc:281: // GCC doesn't like alignment >64 on ARM.
Same?

[dcheng, 2017-02-09 00:00:02]

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

[commit-bot: I haz the power, 2017-02-09 00:00:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-02-09 00:01:24]

I also went and filled in some of the tests so they test alignments from 8-64.

https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance_uni...
File base/lazy_instance_unittest.cc (right):

https://codereview.chromium.org/2670873002/diff/100001/base/lazy_instance_uni...
base/lazy_instance_unittest.cc:169: // GCC doesn't like alignment >64 on ARM.
On 2017/02/08 21:29:47, danakj (slow) wrote:
> Honest question is it worth testing this if nothing is using it what is the
> point?

I don't feel strongly about this; I'm happy to just remove the ifdef'd tests,
because it's really annoying to read all the conditionals.

https://codereview.chromium.org/2670873002/diff/100001/base/memory/aligned_me...
File base/memory/aligned_memory_unittest.cc (right):

https://codereview.chromium.org/2670873002/diff/100001/base/memory/aligned_me...
base/memory/aligned_memory_unittest.cc:22: // GCC doesn't like alignment >64 on
ARM.
On 2017/02/08 21:29:47, danakj (slow) wrote:
> Same Q. Should we test 64 instead?

Done.

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

https://codereview.chromium.org/2670873002/diff/100001/base/memory/singleton_...
base/memory/singleton_unittest.cc:281: // GCC doesn't like alignment >64 on ARM.
On 2017/02/08 21:29:47, danakj (slow) wrote:
> Same?

Done.

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

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

[commit-bot: I haz the power, 2017-02-09 07:07:14]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-02-09 16:16:23]

SeemsGood. Oh and cc/ LGTM too.

[Nico, 2017-02-10 19:56:53]

https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right):

https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED;
doesn't putting attributes at the end usually mean something else? (i.e. that
they apply to the var decl that may follow the struct as in `struct foo {}
foo_inst;`)?

[dcheng, 2017-02-10 20:06:06]

https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right):

https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED;
On 2017/02/10 19:56:53, Nico wrote:
> doesn't putting attributes at the end usually mean something else? (i.e. that
> they apply to the var decl that may follow the struct as in `struct foo {}
> foo_inst;`)?

A non-rigorous test seems to show it's the same.

dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ cat test.cc 
#include <stdio.h>

struct __attribute__((packed)) C {
  char c;
  int i;
};

struct C2 {
  char c;
  int i;
} __attribute__((packed));

struct C3 {
  char c;
  int i;
};

int main() {
  printf("sizeof(C): %lu\n", sizeof(C));
  printf("sizeof(C2): %lu\n", sizeof(C2));
  printf("sizeof(C3): %lu\n", sizeof(C3));
}
dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$
../../../../third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11
test.cc
dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ ./a.out
sizeof(C): 5
sizeof(C2): 5
sizeof(C3): 8

[Nico, 2017-02-10 20:37:58]

https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right):

https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED;
On 2017/02/10 20:06:06, dcheng wrote:
> On 2017/02/10 19:56:53, Nico wrote:
> > doesn't putting attributes at the end usually mean something else? (i.e.
that
> > they apply to the var decl that may follow the struct as in `struct foo {}
> > foo_inst;`)?
> 
> A non-rigorous test seems to show it's the same.
> 
> dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ cat test.cc 
> #include <stdio.h>
> 
> struct __attribute__((packed)) C {
>   char c;
>   int i;
> };
> 
> struct C2 {
>   char c;
>   int i;
> } __attribute__((packed));
> 
> struct C3 {
>   char c;
>   int i;
> };
> 
> int main() {
>   printf("sizeof(C): %lu\n", sizeof(C));
>   printf("sizeof(C2): %lu\n", sizeof(C2));
>   printf("sizeof(C3): %lu\n", sizeof(C3));
> }
> dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$
> ../../../../third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11
> test.cc
> dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ ./a.out
> sizeof(C): 5
> sizeof(C2): 5
> sizeof(C3): 8

Hm, I guess I was thinking of
http://llvm-cs.pcc.me.uk/tools/clang/lib/Sema/SemaDecl.cpp#4165 (i.e. the
attribute in front of `struct`). In this case, clang emits a fixit that suggests
putting the attribut right after `struct`
(http://llvm-cs.pcc.me.uk/tools/clang/test/SemaCXX/attr-declspec-ignored.cpp#6).
 It seems unfortunate that this doesn't compose :-/ Can you at least summarize
the problem from that thread in the Cl description?

[dcheng, 2017-02-10 21:43:41]

On 2017/02/10 20:37:58, Nico wrote:
>
https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
> File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right):
> 
>
https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
> third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED;
> On 2017/02/10 20:06:06, dcheng wrote:
> > On 2017/02/10 19:56:53, Nico wrote:
> > > doesn't putting attributes at the end usually mean something else? (i.e.
> that
> > > they apply to the var decl that may follow the struct as in `struct foo {}
> > > foo_inst;`)?
> > 
> > A non-rigorous test seems to show it's the same.
> > 
> > dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ cat test.cc 
> > #include <stdio.h>
> > 
> > struct __attribute__((packed)) C {
> >   char c;
> >   int i;
> > };
> > 
> > struct C2 {
> >   char c;
> >   int i;
> > } __attribute__((packed));
> > 
> > struct C3 {
> >   char c;
> >   int i;
> > };
> > 
> > int main() {
> >   printf("sizeof(C): %lu\n", sizeof(C));
> >   printf("sizeof(C2): %lu\n", sizeof(C2));
> >   printf("sizeof(C3): %lu\n", sizeof(C3));
> > }
> > dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$
> > ../../../../third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11
> > test.cc
> > dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ ./a.out
> > sizeof(C): 5
> > sizeof(C2): 5
> > sizeof(C3): 8
> 
> Hm, I guess I was thinking of
> http://llvm-cs.pcc.me.uk/tools/clang/lib/Sema/SemaDecl.cpp#4165 (i.e. the
> attribute in front of `struct`). In this case, clang emits a fixit that
suggests
> putting the attribut right after `struct`
>
(http://llvm-cs.pcc.me.uk/tools/clang/test/SemaCXX/attr-declspec-ignored.cpp#6).
>  It seems unfortunate that this doesn't compose :-/ Can you at least summarize
> the problem from that thread in the Cl description?

OK, sure. FWIW, there's another similar variant of this problem already with
__attribute__((visibility("default"))) and co with gcc already discussed on the
thread: after the struct works fine for clang, but not for gcc. So I think it's
"semi-known".

mcgrathr@ also highlighted a potential workaround using C++11 attribute syntax;
there was limited support for using that to get around issues with exported
symbols.

[dcheng, 2017-02-10 21:46:21]

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/c...
> > File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right):
> > 
> >
>
https://codereview.chromium.org/2670873002/diff/120001/third_party/crashpad/c...
> > third_party/crashpad/crashpad/minidump/minidump_extensions.h:252: } PACKED;
> > On 2017/02/10 20:06:06, dcheng wrote:
> > > On 2017/02/10 19:56:53, Nico wrote:
> > > > doesn't putting attributes at the end usually mean something else? (i.e.
> > that
> > > > they apply to the var decl that may follow the struct as in `struct foo
{}
> > > > foo_inst;`)?
> > > 
> > > A non-rigorous test seems to show it's the same.
> > > 
> > > dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ cat test.cc 
> > > #include <stdio.h>
> > > 
> > > struct __attribute__((packed)) C {
> > >   char c;
> > >   int i;
> > > };
> > > 
> > > struct C2 {
> > >   char c;
> > >   int i;
> > > } __attribute__((packed));
> > > 
> > > struct C3 {
> > >   char c;
> > >   int i;
> > > };
> > > 
> > > int main() {
> > >   printf("sizeof(C): %lu\n", sizeof(C));
> > >   printf("sizeof(C2): %lu\n", sizeof(C2));
> > >   printf("sizeof(C3): %lu\n", sizeof(C3));
> > > }
> > > dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$
> > > ../../../../third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11
> > > test.cc
> > > dcheng@nausicaa:~/src/chrome/src/tools/clang/plugins/tests$ ./a.out
> > > sizeof(C): 5
> > > sizeof(C2): 5
> > > sizeof(C3): 8
> > 
> > Hm, I guess I was thinking of
> > http://llvm-cs.pcc.me.uk/tools/clang/lib/Sema/SemaDecl.cpp#4165 (i.e. the
> > attribute in front of `struct`). In this case, clang emits a fixit that
> suggests
> > putting the attribut right after `struct`
> >
>
(http://llvm-cs.pcc.me.uk/tools/clang/test/SemaCXX/attr-declspec-ignored.cpp#6).
> >  It seems unfortunate that this doesn't compose :-/ Can you at least
summarize
> > the problem from that thread in the Cl description?
> 
> OK, sure. FWIW, there's another similar variant of this problem already with
> __attribute__((visibility("default"))) and co with gcc already discussed on
the
> thread: after the struct works fine for clang, but not for gcc. So I think
it's
> "semi-known".
> 
> mcgrathr@ also highlighted a potential workaround using C++11 attribute
syntax;
> there was limited support for using that to get around issues with exported
> symbols.

Oh wait, that's the visibility attribute, which I already mentioned. So it's
already mentioned as one of the downsides on the thread.

[dcheng, 2017-02-14 10:39:52]

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

[commit-bot: I haz the power, 2017-02-14 10:40:07]

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-02-14 13:00:35]

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

[commit-bot: I haz the power, 2017-02-14 13:00:36]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-14 19:12:41]

Description was changed from

==========
Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

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

to

==========
Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

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

[dcheng, 2017-02-14 19:12:41]

dcheng@chromium.org changed reviewers:
+ jochen@chromium.org, jschuh@chromium.org, juliatuttle@chromium.org,
liberato@chromium.org, mark@chromium.org

[dcheng, 2017-02-14 19:13:46]

+jochen for //components + //content
+liberato for //media
+juliatuttle for //net
+mark for //third_party/crashpad

(I'm going to be submitting the CL this evening, so I'll TBR any owners that
haven't replied at that point. Thanks!)

[liberato (no reviews please), 2017-02-14 20:02:17]

media lgtm % two missing MEDIA_EXPORTs.

https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_param...
File media/base/audio_parameters.h (right):

https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_param...
media/base/audio_parameters.h:35: struct alignas(PARAMETERS_ALIGNMENT)
AudioInputBufferParameters {
MEDIA_EXPORT is missing, here and @42.

[dcheng, 2017-02-14 20:06:53]

https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_param...
File media/base/audio_parameters.h (right):

https://codereview.chromium.org/2670873002/diff/140001/media/base/audio_param...
media/base/audio_parameters.h:35: struct alignas(PARAMETERS_ALIGNMENT)
AudioInputBufferParameters {
On 2017/02/14 20:02:17, liberato wrote:
> MEDIA_EXPORT is missing, here and @42.

The export aren't needed (see the green build), since these are just PODs and
the entire definition is in the header.

This probably got lost in the thread, but it's problematic to mix
__attribute__(...) syntax with alignas(...). Since we don't actually need to
export these sybols, we can sidestep it by removing the EXPORT macros.

If this is needed at some point in the future, we'll probably switch the export
macros over to using C++11 attribute syntax, which will work with alignas.

[dcheng, 2017-02-14 22:58:38]

Description was changed from

==========
Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

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

to

==========
Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

Note that compilers have trouble mixing __attribute__((...))
syntax with alignas(...) directives.

  // 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.

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

[dcheng, 2017-02-14 22:59:17]

Note: updated the CL description to include details about the problems mixing
alignas() with __attribute__(()).

[dcheng, 2017-02-14 23:00:19]

Description was changed from

==========
Remove base's ALIGNOF/ALIGNAS in favor of alignof/alignas.

Note that compilers have trouble mixing __attribute__((...))
syntax with alignas(...) directives.

  // 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.

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

to

==========
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
==========

[Mark Mentovai, 2017-02-14 23:01:50]

LGTM in crashpad

> - 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.

I’m aware of ordering sensitivities. It looks like Crashpad already ordered
alignment and packing this way, so what was there to “fix”?

[Mark Mentovai, 2017-02-14 23:03:14]

Never mind, I was looking at an old patch set.

[Mark Mentovai, 2017-02-14 23:07:33]

On 2017/02/14 23:03:14, Mark Mentovai wrote:
> Never mind, I was looking at an old patch set.

OK, so on the current patch set in Crashpad, I’m not sure that what you’ve done
will do the right thing for MSVC.

I’m also not sure that the existing thing is right either, because it never pops
the pack, but that’s a less bad problem than possibly missing out on packing the
first structure in that file. Although fortunately, it appears that the first
one will pack properly without any help.

I don’t mean for this to hold your change up, I’m just wondering if you know
more about how MSVC deals with this and if you’ve tested with it, since you
mentioned both clang and GCC in the description, but I don’t see anything about
MSVC in this change or its review thread.

[Nico, 2017-02-14 23:18:37]

The alignof part lgtm for sure.

I abstractly dislike alignas due to the quirks you mention (gcc bug seems to be
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69585), but it seems it's fairly
rare in practice, so if you think it's better and Mento is happy with breakpad
(see his latest comments), then I suppose that part lgtm as well. But to me it
kind of feels like alignas isn't supported well enough to use yet.

[dcheng, 2017-02-14 23:34:36]

On 2017/02/14 23:07:33, Mark Mentovai wrote:
> On 2017/02/14 23:03:14, Mark Mentovai wrote:
> > Never mind, I was looking at an old patch set.
> 
> OK, so on the current patch set in Crashpad, I’m not sure that what you’ve
done
> will do the right thing for MSVC.
> 
> I’m also not sure that the existing thing is right either, because it never
pops
> the pack, but that’s a less bad problem than possibly missing out on packing
the
> first structure in that file. Although fortunately, it appears that the first
> one will pack properly without any help.
> 
> I don’t mean for this to hold your change up, I’m just wondering if you know
> more about how MSVC deals with this and if you’ve tested with it, since you
> mentioned both clang and GCC in the description, but I don’t see anything
about
> MSVC in this change or its review thread.

Ugh, I think you're right. I made the (incorrect) assumption that this was
working correctly for MSVC and didn't go back to verify after I made the
clang/gcc changes. I suspect this will probably make this change impossible,
unless we're willing to use C++11-style atttributes...

(I'll investigate and update the thread with the conclusions)

[Nico, 2017-02-15 15:09:55]

On 2017/02/14 23:34:36, dcheng wrote:
> On 2017/02/14 23:07:33, Mark Mentovai wrote:
> > On 2017/02/14 23:03:14, Mark Mentovai wrote:
> > > Never mind, I was looking at an old patch set.
> > 
> > OK, so on the current patch set in Crashpad, I’m not sure that what you’ve
> done
> > will do the right thing for MSVC.
> > 
> > I’m also not sure that the existing thing is right either, because it never
> pops
> > the pack, but that’s a less bad problem than possibly missing out on packing
> the
> > first structure in that file. Although fortunately, it appears that the
first
> > one will pack properly without any help.
> > 
> > I don’t mean for this to hold your change up, I’m just wondering if you know
> > more about how MSVC deals with this and if you’ve tested with it, since you
> > mentioned both clang and GCC in the description, but I don’t see anything
> about
> > MSVC in this change or its review thread.
> 
> Ugh, I think you're right. I made the (incorrect) assumption that this was
> working correctly for MSVC and didn't go back to verify after I made the
> clang/gcc changes. I suspect this will probably make this change impossible,
> unless we're willing to use C++11-style atttributes...
> 
> (I'll investigate and update the thread with the conclusions)

Looks like all bots came back green -- I guess we're missing test coverage in
this area?

[Mark Mentovai, 2017-02-15 15:27:25]

On 2017/02/15 15:09:55, Nico wrote:
> On 2017/02/14 23:34:36, dcheng wrote:
> > On 2017/02/14 23:07:33, Mark Mentovai wrote:
> > > On 2017/02/14 23:03:14, Mark Mentovai wrote:
> > > > Never mind, I was looking at an old patch set.
> > > 
> > > OK, so on the current patch set in Crashpad, I’m not sure that what you’ve
> > done
> > > will do the right thing for MSVC.
> > > 
> > > I’m also not sure that the existing thing is right either, because it
never
> > pops
> > > the pack, but that’s a less bad problem than possibly missing out on
packing
> > the
> > > first structure in that file. Although fortunately, it appears that the
> first
> > > one will pack properly without any help.
> > > 
> > > I don’t mean for this to hold your change up, I’m just wondering if you
know
> > > more about how MSVC deals with this and if you’ve tested with it, since
you
> > > mentioned both clang and GCC in the description, but I don’t see anything
> > about
> > > MSVC in this change or its review thread.
> > 
> > Ugh, I think you're right. I made the (incorrect) assumption that this was
> > working correctly for MSVC and didn't go back to verify after I made the
> > clang/gcc changes. I suspect this will probably make this change impossible,
> > unless we're willing to use C++11-style atttributes...
> > 
> > (I'll investigate and update the thread with the conclusions)
> 
> Looks like all bots came back green -- I guess we're missing test coverage in
> this area?

MSVC interop may actually be fine. I was just wondering because I saw a lot of
GCC and clang talk, but no MSVC talk, and the alignas/packed combo is fiddly.

[jochen (gone - plz use gerrit), 2017-02-20 09:53:57]

lgtm from my side

[Julia Tuttle, 2017-02-23 20:19:09]

net/ lgtm but maybe someone from quic should look at it.

[dcheng, 2017-02-23 21:38:03]

On 2017/02/15 15:27:25, Mark Mentovai wrote:
> On 2017/02/15 15:09:55, Nico wrote:
> > On 2017/02/14 23:34:36, dcheng wrote:
> > > On 2017/02/14 23:07:33, Mark Mentovai wrote:
> > > > On 2017/02/14 23:03:14, Mark Mentovai wrote:
> > > > > Never mind, I was looking at an old patch set.
> > > > 
> > > > OK, so on the current patch set in Crashpad, I’m not sure that what
you’ve
> > > done
> > > > will do the right thing for MSVC.
> > > > 
> > > > I’m also not sure that the existing thing is right either, because it
> never
> > > pops
> > > > the pack, but that’s a less bad problem than possibly missing out on
> packing
> > > the
> > > > first structure in that file. Although fortunately, it appears that the
> > first
> > > > one will pack properly without any help.
> > > > 
> > > > I don’t mean for this to hold your change up, I’m just wondering if you
> know
> > > > more about how MSVC deals with this and if you’ve tested with it, since
> you
> > > > mentioned both clang and GCC in the description, but I don’t see
anything
> > > about
> > > > MSVC in this change or its review thread.
> > > 
> > > Ugh, I think you're right. I made the (incorrect) assumption that this was
> > > working correctly for MSVC and didn't go back to verify after I made the
> > > clang/gcc changes. I suspect this will probably make this change
impossible,
> > > unless we're willing to use C++11-style atttributes...
> > > 
> > > (I'll investigate and update the thread with the conclusions)
> > 
> > Looks like all bots came back green -- I guess we're missing test coverage
in
> > this area?
> 
> MSVC interop may actually be fine. I was just wondering because I saw a lot of
> GCC and clang talk, but no MSVC talk, and the alignas/packed combo is fiddly.

OK, so I had a few spare cycles. As it turns out, MSVC is OK, but that's because
it works completely differently from what I originally thought. I mistakenly
thought that the #pragma pack(push, 1) was nested inside #define PACKED, but
it's not: for MSVC, we just #define PACKED to be empty and then rely on the
#pragma packed(push, ...) outside the macro to do the actual packing.

thakis@: For consistency, I think it'd be preferable to use alignof/alignas or
ALIGNOF/ALIGNAS. Given that MSVC already has a completely different "attribute"
model for packed structs, are you OK with this landing?

[Nico, 2017-02-23 21:40:36]

I lg'd above already. I personally wouldn't land the ALIGNAS part until it
works better, but up to you.

On Thu, Feb 23, 2017 at 4:38 PM, <dcheng@chromium.org> wrote:

> On 2017/02/15 15:27:25, Mark Mentovai wrote:
> > On 2017/02/15 15:09:55, Nico wrote:
> > > On 2017/02/14 23:34:36, dcheng wrote:
> > > > On 2017/02/14 23:07:33, Mark Mentovai wrote:
> > > > > On 2017/02/14 23:03:14, Mark Mentovai wrote:
> > > > > > Never mind, I was looking at an old patch set.
> > > > >
> > > > > OK, so on the current patch set in Crashpad, I’m not sure that what
> you’ve
> > > > done
> > > > > will do the right thing for MSVC.
> > > > >
> > > > > I’m also not sure that the existing thing is right either, because
> it
> > never
> > > > pops
> > > > > the pack, but that’s a less bad problem than possibly missing out
> on
> > packing
> > > > the
> > > > > first structure in that file. Although fortunately, it appears
> that the
> > > first
> > > > > one will pack properly without any help.
> > > > >
> > > > > I don’t mean for this to hold your change up, I’m just wondering
> if you
> > know
> > > > > more about how MSVC deals with this and if you’ve tested with it,
> since
> > you
> > > > > mentioned both clang and GCC in the description, but I don’t see
> anything
> > > > about
> > > > > MSVC in this change or its review thread.
> > > >
> > > > Ugh, I think you're right. I made the (incorrect) assumption that
> this was
> > > > working correctly for MSVC and didn't go back to verify after I made
> the
> > > > clang/gcc changes. I suspect this will probably make this change
> impossible,
> > > > unless we're willing to use C++11-style atttributes...
> > > >
> > > > (I'll investigate and update the thread with the conclusions)
> > >
> > > Looks like all bots came back green -- I guess we're missing test
> coverage
> in
> > > this area?
> >
> > MSVC interop may actually be fine. I was just wondering because I saw a
> lot of
> > GCC and clang talk, but no MSVC talk, and the alignas/packed combo is
> fiddly.
>
> OK, so I had a few spare cycles. As it turns out, MSVC is OK, but that's
> because
> it works completely differently from what I originally thought. I
> mistakenly
> thought that the #pragma pack(push, 1) was nested inside #define PACKED,
> but
> it's not: for MSVC, we just #define PACKED to be empty and then rely on the
> #pragma packed(push, ...) outside the macro to do the actual packing.
>
> thakis@: For consistency, I think it'd be preferable to use
> alignof/alignas or
> ALIGNOF/ALIGNAS. Given that MSVC already has a completely different
> "attribute"
> model for packed structs, are you OK with this landing?
>
> https://codereview.chromium.org/2670873002/
>

-- 
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.

[dcheng, 2017-02-23 21:43:14]

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

[commit-bot: I haz the power, 2017-02-23 21:43:43]

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-02-24 00:36:02]

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

[commit-bot: I haz the power, 2017-02-24 00:36:03]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2017-04-07 20:16:18]

juliatuttle@chromium.org changed reviewers:
+ nicolas.baroiller@gmail.com
- thakis@chromium.org

[Julia Tuttle, 2017-04-07 20:16:18]

Dropping this from my dashboard since it's been lying around a while.

[Julia Tuttle, 2017-04-07 20:17:39]

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