Proposal to ban alignof/alignas for now, revisit when VS2015 is deployed

[Yuta Kitamura]

Hi,

I've ran several experiments to see if we can use alignof/alignas in Chromium/Blink:

https://codereview.chromium.org/1497963002/

Unfortunately, VS2013 doesn't support these, so apparently we can't use them yet.

MSDN says VS2015 will have those, though:

https://msdn.microsoft.com/en-us/library/dn956970.aspx

So I propose to ban these for now, with a note to revisit when VS2015 arrives.

--

As an aside, alignas(N) for N > 64 causes an error on GCC (see patch set 1 of the CL above). According to aligned_memory_unittest.cc, higher N tends to cause problems in other toolchains, too. See the triple nested #ifs in the file:

https://chromium.googlesource.com/chromium/src/+/master/base/memory/aligned_memory_unittest.cc

*sad face*

I wonder if we could ban the use of ALIGNAS(N) macro for N > 64, because higher N isn't very useful itself and we currently don't use such a high value actually. The macro is useful for N = 8 or N = 16 to get better performance by aligned read, but I can't think of cases where N >= 32 is needed for this use case. If we need something page-aligned (N = 4096), we should probably make use of other means (mmap etc.).

WDYT?

Thanks,

Yuta

[JF Bastien]

On Tue, Dec 8, 2015 at 7:26 AM, Yuta Kitamura <yu...@chromium.org> wrote:

Hi,

I've ran several experiments to see if we can use alignof/alignas in Chromium/Blink:

https://codereview.chromium.org/1497963002/

Unfortunately, VS2013 doesn't support these, so apparently we can't use them yet.

MSDN says VS2015 will have those, though:

https://msdn.microsoft.com/en-us/library/dn956970.aspx

So I propose to ban these for now, with a note to revisit when VS2015 arrives.

Unfortunate, but that sounds fine since the macros just work with __alignof / __declspec(align((#)).

As an aside, alignas(N) for N > 64 causes an error on GCC (see patch set 1 of the CL above). According to aligned_memory_unittest.cc, higher N tends to cause problems in other toolchains, too. See the triple nested #ifs in the file:

https://chromium.googlesource.com/chromium/src/+/master/base/memory/aligned_memory_unittest.cc

*sad face*

I wonder if we could ban the use of ALIGNAS(N) macro for N > 64, because higher N isn't very useful itself and we currently don't use such a high value actually. The macro is useful for N = 8 or N = 16 to get better performance by aligned read, but I can't think of cases where N >= 32 is needed for this use case. If we need something page-aligned (N = 4096), we should probably make use of other means (mmap etc.).

It's useful to avoid destructive interference on some HW Chrome likely doesn't care about. Some packages (such as TBB) are / were pessimistic at some point in time and chose 128 bytes as cacheline size because IIRC one Intel chip chose that value at some point in time. I'm hoping that these numbers can eventually be provided by the compiler (http://wg21.link/p0154r0) but that won't help before at least C++17.

The unit test has issues with stack alignment, which is still useful (sometimes even cross-thread) but not as widely has heap alignment IMO.

It may be worth limiting until toolchains are fixed.

[Yuta Kitamura]

On Tue, Dec 8, 2015 at 3:42 PM, JF Bastien <j...@chromium.org> wrote:

On Tue, Dec 8, 2015 at 7:26 AM, Yuta Kitamura <yu...@chromium.org> wrote:

Hi,

I've ran several experiments to see if we can use alignof/alignas in Chromium/Blink:

https://codereview.chromium.org/1497963002/

Unfortunately, VS2013 doesn't support these, so apparently we can't use them yet.

MSDN says VS2015 will have those, though:

https://msdn.microsoft.com/en-us/library/dn956970.aspx

So I propose to ban these for now, with a note to revisit when VS2015 arrives.

Unfortunate, but that sounds fine since the macros just work with __alignof / __declspec(align((#)).

Thanks, here's the CL to update the documentation for anyone in OWNERS to (dis)approve:

https://codereview.chromium.org/1509123002/

 

As an aside, alignas(N) for N > 64 causes an error on GCC (see patch set 1 of the CL above). According to aligned_memory_unittest.cc, higher N tends to cause problems in other toolchains, too. See the triple nested #ifs in the file:

https://chromium.googlesource.com/chromium/src/+/master/base/memory/aligned_memory_unittest.cc

*sad face*

I wonder if we could ban the use of ALIGNAS(N) macro for N > 64, because higher N isn't very useful itself and we currently don't use such a high value actually. The macro is useful for N = 8 or N = 16 to get better performance by aligned read, but I can't think of cases where N >= 32 is needed for this use case. If we need something page-aligned (N = 4096), we should probably make use of other means (mmap etc.).

It's useful to avoid destructive interference on some HW Chrome likely doesn't care about. Some packages (such as TBB) are / were pessimistic at some point in time and chose 128 bytes as cacheline size because IIRC one Intel chip chose that value at some point in time. I'm hoping that these numbers can eventually be provided by the compiler (http://wg21.link/p0154r0) but that won't help before at least C++17.

The unit test has issues with stack alignment, which is still useful (sometimes even cross-thread) but not as widely has heap alignment IMO.

It may be worth limiting until toolchains are fixed.

This is interesting. May be *too* interesting for Chromium's use case, though ;)

I don't feel strongly on this; once we enable alignas after VS 2015, this problem will come up, but it may not be too late to reevaluate at that point (gcc situation may change until then).

[Nico Weber]

On Tue, Dec 8, 2015 at 1:42 AM, JF Bastien <j...@chromium.org> wrote:

On Tue, Dec 8, 2015 at 7:26 AM, Yuta Kitamura <yu...@chromium.org> wrote:

Hi,

I've ran several experiments to see if we can use alignof/alignas in Chromium/Blink:

https://codereview.chromium.org/1497963002/

Unfortunately, VS2013 doesn't support these, so apparently we can't use them yet.

MSDN says VS2015 will have those, though:

https://msdn.microsoft.com/en-us/library/dn956970.aspx

So I propose to ban these for now, with a note to revisit when VS2015 arrives.

Unfortunate, but that sounds fine since the macros just work with __alignof / __declspec(align((#)).

Only tangentially related to this thread, but it looks like __declspec(align) and __attribute__((__aligned__)) have slightly different semantics: The former tells the compiler "try to align this type when you allocate memory for it yourself, i.e. for static and automatic variables, and if you did ensure alignment yourself you're free to exploit this". The latter tells the compiler "when you see an instance of this type anywhere, no matter if on stack or heap, you can assume that it will be aligned".

See the discussion in https://codereview.chromium.org/1498133002/ and the bug linked from there for details.

[Jeffrey Yasskin]

And, unfortunately, the alignas() macros don't affect heap alignment at all because operator new doesn't yet have an interface to accept its argument type's alignment: http://wg21.link/p0035r0

Jeffrey

[JF Bastien]

Ugh I'd forgotten about that one.

It sounds like we shouldn't just ban the usage, we should also have a pre-submit checker that validates usage (no stack, no heap). Of course that checker needs to be aware of alignas/__declspec(align(#))/__attribute__(aligned(#)).

[Daniel Cheng]

I wrote https://codereview.chromium.org/2670873002/ which removes the macros from //base/compiler_specific.h. I found a few issues while doing this:

1. GCC on ARM doesn't like alignas(N) if N > 64. There doesn't seem to be any non-test code affected by this, and it throws a compile error if it doesn't like the alignment. However, this seems strictly better than relying on runtime checks to detect unsupported configurations (see aligned_memory_unittest.cc [1]).

2. GCC/Clang are somewhat picky about mixing __attribute__(...) with alignas().

struct alignas(16) __attribute__((packed)) Foo { ... };  // Not OK

struct __attribute__((packed)) alignas(16) Foo { ... };  // OK in clang

struct alignas(16) Foo { ... }  __attribute__((packed));  // OK in clang and gcc

3. GCC on ARM doesn't permit __attribute__((visibility(...))) after a definition:

struct alignas(16) Foo { ... } __attribute__((visibility("default")));  // OK in clang, not OK in gcc

which means it's impossible to export a struct/class while specifying alignment.

For (2), there's a workaround. For (3), there's no workaround, but it seems like structs/classes that require alignment are generally POD objects. Thus, there's generally no need to export them anyway. Usage should be relatively rare, so I don't expect these issues to occur much in practice.Given this, does moving alignof/alignas to the allowed language features seem reasonable?

Daniel

[1] https://chromium.googlesource.com/chromium/src/+/65e3ef2c11512c69ebb750637e6b7dc31e74b864/base/memory/aligned_memory_unittest.cc

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABdywOd%2Bkr3YbAbhzDJ99aM5g9HdHaKdi54r%3DcHu6nMkHygNpA%40mail.gmail.com.

[dan...@chromium.org]

On Mon, Feb 6, 2017 at 11:50 AM, Daniel Cheng <dch...@chromium.org> wrote:

I wrote https://codereview.chromium.org/2670873002/ which removes the macros from //base/compiler_specific.h. I found a few issues while doing this:

1. GCC on ARM doesn't like alignas(N) if N > 64. There doesn't seem to be any non-test code affected by this, and it throws a compile error if it doesn't like the alignment. However, this seems strictly better than relying on runtime checks to detect unsupported configurations (see aligned_memory_unittest.cc [1]).

2. GCC/Clang are somewhat picky about mixing __attribute__(...) with alignas().

struct alignas(16) __attribute__((packed)) Foo { ... };  // Not OK

struct __attribute__((packed)) alignas(16) Foo { ... };  // OK in clang

struct alignas(16) Foo { ... }  __attribute__((packed));  // OK in clang and gcc

3. GCC on ARM doesn't permit __attribute__((visibility(...))) after a definition:

struct alignas(16) Foo { ... } __attribute__((visibility("default")));  // OK in clang, not OK in gcc

which means it's impossible to export a struct/class while specifying alignment.

For (2), there's a workaround. For (3), there's no workaround, but it seems like structs/classes that require alignment are generally POD objects. Thus, there's generally no need to export them anyway. Usage should be relatively rare, so I don't expect these issues to occur much in practice.Given this, does moving alignof/alignas to the allowed language features seem reasonable?

Not occuring much seems bad still if it will occur at all, given there's no workaround. These cases will have to redesign things to make the aligned thing POD or keep/invent something like AlignedMemory around for them. There doesn't seem to be a good motivation to allow these if we have to also keep AlignedMemory, unless we're implementing AlignedMemory with them when possible, and allowing them only there. Unless you think it is good and reasonable that cases that run into problems here are refactored to not require export and aligned on the same type?

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKp-W-9w7H5G-kbwN%2BZj8wxuVa0%3D8n1kObSK8_gXZr0%2BZw%40mail.gmail.com.

[Roland McGrath]

On Mon, Feb 6, 2017 at 8:50 AM, Daniel Cheng <dch...@chromium.org> wrote:

3. GCC on ARM doesn't permit __attribute__((visibility(...))) after a definition:

struct alignas(16) Foo { ... } __attribute__((visibility("default")));  // OK in clang, not OK in gcc

which means it's impossible to export a struct/class while specifying alignment.

struct alignas(16) [[gnu::visibility("default")]] Foo { ... };

[Daniel Cheng]

On Mon, Feb 6, 2017 at 11:32 AM <dan...@chromium.org> wrote:

On Mon, Feb 6, 2017 at 11:50 AM, Daniel Cheng <dch...@chromium.org> wrote:

I wrote https://codereview.chromium.org/2670873002/ which removes the macros from //base/compiler_specific.h. I found a few issues while doing this:

1. GCC on ARM doesn't like alignas(N) if N > 64. There doesn't seem to be any non-test code affected by this, and it throws a compile error if it doesn't like the alignment. However, this seems strictly better than relying on runtime checks to detect unsupported configurations (see aligned_memory_unittest.cc [1]).

2. GCC/Clang are somewhat picky about mixing __attribute__(...) with alignas().

struct alignas(16) __attribute__((packed)) Foo { ... };  // Not OK

struct __attribute__((packed)) alignas(16) Foo { ... };  // OK in clang

struct alignas(16) Foo { ... }  __attribute__((packed));  // OK in clang and gcc

3. GCC on ARM doesn't permit __attribute__((visibility(...))) after a definition:

struct alignas(16) Foo { ... } __attribute__((visibility("default")));  // OK in clang, not OK in gcc

which means it's impossible to export a struct/class while specifying alignment.

For (2), there's a workaround. For (3), there's no workaround, but it seems like structs/classes that require alignment are generally POD objects. Thus, there's generally no need to export them anyway. Usage should be relatively rare, so I don't expect these issues to occur much in practice.Given this, does moving alignof/alignas to the allowed language features seem reasonable?

Not occuring much seems bad still if it will occur at all, given there's no workaround. These cases will have to redesign things to make the aligned thing POD or keep/invent something like AlignedMemory around for them. There doesn't seem to be a good motivation to allow these if we have to also keep AlignedMemory, unless we're implementing AlignedMemory with them when possible, and allowing them only there. Unless you think it is good and reasonable that cases that run into problems here are refactored to not require export and aligned on the same type?

My next proposal is to remove AlignedMemory in favor of std::aligned_storage =)

Strictly speaking, it's not completely true that there's no workaround: it's possible to fallback to the original compiler-specific syntax and combine that with alignas(): __declspec(align(N)) for MSVC, and __attribute__((aligned(N))) for GCC/clang (which is an argument for keeping our current macros in base/compiler_specific.h). But given that there are zero instances in Chrome that require struct __attribute__(...) alignas(N) C { ... } syntax... it seems like this is likely going to remain a theoretical concern?

Daniel

[Daniel Cheng]

Sadly, it seems like C++11 attribute syntax doesn't work in VS 2015 =/

Daniel

 

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAB%3D4xhrWzAcThwMu7ho%3Dm0SNvSNNeoi6o5%3DAK%2BK93Cz-iNEQww%40mail.gmail.com.

[Roland McGrath]

The visibility attribute is something specific to ELF anyway.  For MSVC, __declspec(dllexport) is what's used.  Does MSVC allow 'struct alignas(n) __declspec(...) Foo { ... };' ?

[Daniel Cheng]

On Mon, Feb 6, 2017 at 5:12 PM 'Roland McGrath' via cxx <c...@chromium.org> wrote:

The visibility attribute is something specific to ELF anyway.  For MSVC, __declspec(dllexport) is what's used.  Does MSVC allow 'struct alignas(n) __declspec(...) Foo { ... };' ?

Yes, but at least in the past, we've generally disallowed a language feature until it's supported on all our toolchains.

Daniel

 

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAB%3D4xhruWmC74Lhr8_OZH%2BtOcJWTnbmtT6g%2Bma7AOjh2bgphHQ%40mail.gmail.com.

[Roland McGrath]

It's not "using the C++11 [[...]] language" feature if the only use is in toolchain-specific macro definitions, which is true of our uses of the visibility attribute.

[Yuta Kitamura]

Sorry for being late to the party. Let me give some comments based on what I remember about my last experiments...

On Tue, Feb 7, 2017 at 1:50 AM, Daniel Cheng <dch...@chromium.org> wrote:

I wrote https://codereview.chromium.org/2670873002/ which removes the macros from //base/compiler_specific.h. I found a few issues while doing this:

1. GCC on ARM doesn't like alignas(N) if N > 64. There doesn't seem to be any non-test code affected by this, and it throws a compile error if it doesn't like the alignment. However, this seems strictly better than relying on runtime checks to detect unsupported configurations (see aligned_memory_unittest.cc [1]).

Yeah that's true. However, practically there's no real use case for such a big alignment. The main focus of alignas is to align data to follow the processor's requirements; 16 bytes alignments are usually sufficient for this purpose. (I imagine it's harder for compilers to implement large alignments, like for stack or data sections)

If you need to align something to page boundary, it's probably better to rely on other mechanisms.

 

2. GCC/Clang are somewhat picky about mixing __attribute__(...) with alignas().

struct alignas(16) __attribute__((packed)) Foo { ... };  // Not OK

struct __attribute__((packed)) alignas(16) Foo { ... };  // OK in clang

struct alignas(16) Foo { ... }  __attribute__((packed));  // OK in clang and gcc

3. GCC on ARM doesn't permit __attribute__((visibility(...))) after a definition:

struct alignas(16) Foo { ... } __attribute__((visibility("default")));  // OK in clang, not OK in gcc

which means it's impossible to export a struct/class while specifying alignment.

For (2), there's a workaround. For (3), there's no workaround, but it seems like structs/classes that require alignment are generally POD objects. Thus, there's generally no need to export them anyway. Usage should be relatively rare, so I don't expect these issues to occur much in practice.Given this, does moving alignof/alignas to the allowed language features seem reasonable?

I don't remember this...

Also, I remember that some syntax combining __declspec(align(...)) and typedef could not be expressed with standard alignas syntax. I think I had to leave __declspec() for that one.

[Jeremy Roman]

On Tue, Feb 7, 2017 at 4:09 AM, Yuta Kitamura <yu...@chromium.org> wrote:

Sorry for being late to the party. Let me give some comments based on what I remember about my last experiments...

On Tue, Feb 7, 2017 at 1:50 AM, Daniel Cheng <dch...@chromium.org> wrote:

I wrote https://codereview.chromium.org/2670873002/ which removes the macros from //base/compiler_specific.h. I found a few issues while doing this:

1. GCC on ARM doesn't like alignas(N) if N > 64. There doesn't seem to be any non-test code affected by this, and it throws a compile error if it doesn't like the alignment. However, this seems strictly better than relying on runtime checks to detect unsupported configurations (see aligned_memory_unittest.cc [1]).

Yeah that's true. However, practically there's no real use case for such a big alignment. The main focus of alignas is to align data to follow the processor's requirements; 16 bytes alignments are usually sufficient for this purpose. (I imagine it's harder for compilers to implement large alignments, like for stack or data sections)

If you need to align something to page boundary, it's probably better to rely on other mechanisms.

Agreed that this is okay. I'm also skeptical of the need for >64-byte alignment.

 

2. GCC/Clang are somewhat picky about mixing __attribute__(...) with alignas().

struct alignas(16) __attribute__((packed)) Foo { ... };  // Not OK

struct __attribute__((packed)) alignas(16) Foo { ... };  // OK in clang

struct alignas(16) Foo { ... }  __attribute__((packed));  // OK in clang and gcc

This seems fine (and packed already seems to occur at the end of structs). Weird, but okay.

 

3. GCC on ARM doesn't permit __attribute__((visibility(...))) after a definition:

struct alignas(16) Foo { ... } __attribute__((visibility("default")));  // OK in clang, not OK in gcc

which means it's impossible to export a struct/class while specifying alignment.

For (2), there's a workaround. For (3), there's no workaround, but it seems like structs/classes that require alignment are generally POD objects. Thus, there's generally no need to export them anyway. Usage should be relatively rare, so I don't expect these issues to occur much in practice.Given this, does moving alignof/alignas to the allowed language features seem reasonable?

(3) seems like this is unlikely to come up in practice, and there are a few possible workarounds here. (If nothing else, you could nest the aligned data in a struct -- which may well be what you want, anyways, especially if the class is virtual.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFJcur9Gn71KKjXNn_FmK2N_gidw1ge4uKYXStcVHwjUKb7u2A%40mail.gmail.com.

[dan...@chromium.org]

OK I had the impression from "I don't expect these issues to occur much in practice" as you weren't sure if they'd affect existing code. But it sounds like instead this is fine for all current use cases. So I'm with Jeremy and this seems okay to me.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13e1XOixKq3S7wfKq77UzkvxTLPT7i0-Xcd6r-tobF_24Q%40mail.gmail.com.