operator new and STL containers silently ignore element alignment

[Quuxplusone]

Bugzilla Link	PR22634
Status	CONFIRMED
Importance	P normal
Reported by	Benoit Jacob (jacob.benoit.1@gmail.com)
Reported on	2015-02-19 10:20:07 -0800
Last modified on	2018-10-02 05:59:42 -0700
Version	3.5
Hardware	Macintosh All
CC	aw1621107@gmail.com, eric@efcs.ca, leonard@lausen.nl, llvm-bugs@lists.llvm.org, mclow.lists@gmail.com, richard-llvm@metafoo.co.uk, ville.voutilainen@gmail.com, vsapsai@apple.com
Fixed by commit(s)
Attachments	vectoralign.cpp (188 bytes, text/x-c++src) test.pass.cpp (295 bytes, text/x-c++src) align.cpp (1296 bytes, text/plain)
Blocks
Blocked by
See also

Created attachment 13905
testcase

Even in C++11 mode, even with the C++11 alignas keyword, element alignment
isn't honored by std::vector.

$ c++ vectoralign.cpp -o a --std=c++11 && ./a
ctor 0x7fec03c03950
ctor 0x7fec03c03990

Attaching testcase.

I only tested the 3.5svn version that's installed on Mac OSX 10.9.5 here.

$ c++ --version
Apple LLVM version 6.0 (clang-600.0.56) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin13.4.0
Thread model: posix

Note: homologous libstdc++ bug report:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65122

[Quuxplusone]

Attached vectoralign.cpp (188 bytes, text/x-c++src): testcase

[Quuxplusone]

Attached test.pass.cpp (295 bytes, text/x-c++src): Over aligned types and operator new.

[Quuxplusone]

Attached align.cpp (1296 bytes, text/plain): testcase for impact of alignas and attribute(aligned) on heap alignment. GCC != Clang.

[Quuxplusone]

Right, it's not specific to vector, but I thought it may be useful to have a
specific example to work on.

How would you interprete this part of the spec:

Footnote 9 on http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf
:

> If a request for a specific extended alignment in a specific context is not
supported by an implementation,
> the program is ill-formed. Additionally, a request for runtime allocation of
dynamic storage for which the
> requested alignment cannot be honored shall be treated as an allocation
failure

Does that mean that if operator new cannot satisfy the alignment of an over-
aligned type, then it should fail i.e. return null and/or throw bad_alloc?

[Quuxplusone]

I mean Footnote 9 in Section 3.11, on page 81.

[Quuxplusone]

...actually not a footnote, got confused by the page layout.

[Quuxplusone]

Renamed the bug accordingly: not specific to vector, happens already with operator new, and question over whether it's OK to silently ignore alignment or whether the allocation should fail as suggested by my naive reading of 3.11/9.

[Quuxplusone]

I don't see how this can be a compile error. You're suggesting that the
compiler issue an error (a compile time thing) for a run time property of a
variable.

Consider the following code (typed into browser, sorry for typos)

  $ cat Foo.h
    class Foo;
    Foo * get_Foo();

  $ cat Foo.cpp
    #include "Foo.h"

    class Foo {
        alignas(0x40) float array[1];
    };

    Foo * get_Foo() { return new Foo; }

  $ cat main.cpp
    #include "Foo.h"
    int main () { Foo *f = get_Foo(); }

If you compile the two cpp files separately, where should the compiler signal
an error (if any)?
Does it matter if the file "main.cpp" contains user-defined operator new?

[Quuxplusone]

(In reply to comment #5)
> Right, it's not specific to vector, but I thought it may be useful to have a
> specific example to work on.
>
> How would you interprete this part of the spec:
>
> Footnote 9 on
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf :
>
> > If a request for a specific extended alignment in a specific context is not
supported by an implementation,
> > the program is ill-formed. Additionally, a request for runtime allocation
of dynamic storage for which the
> > requested alignment cannot be honored shall be treated as an allocation
failure
>
> Does that mean that if operator new cannot satisfy the alignment of an
> over-aligned type, then it should fail i.e. return null and/or throw
> bad_alloc?

I don't know; I think I'm going to punt this over to -core. :-)

[Quuxplusone]

(In reply to comment #9)
> I don't see how this can be a compile error. You're suggesting that the
> compiler issue an error (a compile time thing) for a run time property of a
> variable.
>
> Consider the following code (typed into browser, sorry for typos)
>
>   $ cat Foo.h
>     class Foo;
>     Foo * get_Foo();
>
>   $ cat Foo.cpp
>     #include "Foo.h"
>
>     class Foo {
>         alignas(0x40) float array[1];
>     };
>
>     Foo * get_Foo() { return new Foo; }
>
>   $ cat main.cpp
>     #include "Foo.h"
>     int main () { Foo *f = get_Foo(); }
>
> If you compile the two cpp files separately, where should the compiler
> signal an error (if any)?

In my naive reading of 3.11/9, the error would be generated while compiling
Foo.cpp, assuming we're using the compiler's default operator new and it
doesn't support the required alignment.

> Does it matter if the file "main.cpp" contains user-defined operator new?

Why would it? (Honest question - I'm way outside of my comfort zone here). In
my naive understanding, all what matters is what operator new is being used
when compiling the 'return new Foo' line in Foo.cpp. I understand 3.11/9 as
saying that if that operator new doesn't support the required alignment then
the allocation must fail. What part of my understanding is wrong?

[Quuxplusone]

There is a proposal to solve this, see
http://open-std.org/JTC1/SC22/WG21/docs/papers/2012/n3396.htm

This was also NB comment FI 16 for the C++14 ballot, but the committee
decided to postpone the solution after C++14. I guess a potential work-around
is allocating aligned_storage dynamically.

[Quuxplusone]

N3396 points out (for a case very similar to yours) that:

    "In this example, not only is an implementation of C++11 not required to allocate properly-aligned memory, for the array, for practical purposes it is very nearly required to do the allocation incorrectly; in any event, it is certainly required to perform the allocation by a process that does not take the specified alignment value into account".

I'm going to close this as "wontfix", not because I don't think it's a bug - I
think it is - but because it is a problem that the standards group has to
solve. If you disagree and have a solution, please re-open the bug.

When the standard is rectified, clang and libc++ will implement it.

What it boils down to is that by the time you get to operator new, all you've
got is a number of bytes to allocate; all the alignment information has been
discarded. We need a change in the way that operator new works, so as not to
discard that information.

[ Note that *sometimes* clang/libc++ will give you data aligned how you want.
If your memory subsystem hands out addresses at random, and they are all 16
byte aligned, then half of them will be 32-byte aligned. Your test program
handles this by allocating 100 times, and effectively looking for the least-
aligned one in the bunch. ]

P.S. Using gcc 4.9.2 on my mac, I get the same results as clang from your test
program (4, 32, 32, 16, 16, 16)

[Quuxplusone]

The corresponding libstdc++ bug report (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65122) has been closed as fixed due to wording changes in the C++17 standard. Have the corresponding changes in libc++ already been applied? I see the new operator new overloads in , but I'm just an end user, so I don't know whether additional changes need to be made.

[Quuxplusone]

In order to fix this properly we need to teach __builtin_operator_new and __builtin_operator_delete about over-aligned types and how to use align_val_t.

@Richard does this sound reasonable to you?

[Quuxplusone]

Assigning to myself. About half way through a patch to add the necessary
mechanics to Clang. My plan is to add __builtin_operator_new_aligned(size,
alignment) and __builtin_operator_delete_aligned(void*, alignment).

Clang seems to have all the information necessary to determine if libc++
supports aligned new/delete, so that makes things easy. We should be able to
switch over to the aligned calls in all dialects and let Clang sort out which
operator to call.

Once that's been done Libc++ will provide correctly aligned memory while still
allowing LLVM to elide the builtin calls.

[Quuxplusone]

Hi Eric, looks like your recent change r328180 http://llvm.org/viewvc/llvm-project?view=revision&revision=328180 doesn't work well for the case: old compiler (without builtin aligned operator new/delete), new libc++ headers, old libc++ dylib (without aligned operator new/delete). In this case _LIBCPP_HAS_NO_ALIGNED_ALLOCATION is not defined, _LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE is defined, at link time we fail because of undefined symbols like "operator new(unsigned long, std::align_val_t)".

As for me, the tricky part is that _LIBCPP_HAS_NO_ALIGNED_ALLOCATION doesn't distinguish between "cannot have aligned allocation implemented" and "shouldn't call aligned allocation operators". Currently the macro means the former, so when it is possible to implement aligned allocation, in

ifndef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION

if (__is_overaligned_for_new(align)) { const align_val_t align_val = static_cast(__align);

ifdef _LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE

return ::operator new(__size, __align_val);

else

return __builtin_operator_new(__size, __align_val);

endif

}

else

((void)__align);

endif

you would call operator new(__size, __align_val) but it might fail during linking because dylib doesn't have the operator.

I don't know if it is worth having 2 macros to distinguish between these 2 cases. Currently I am gravitating toward defining _LIBCPP_HAS_NO_ALIGNED_ALLOCATION when targeting systems that don't support aligned allocation. Eric, what do you think? What approach would you recommend?

[Quuxplusone]

> I am gravitating toward defining _LIBCPP_HAS_NO_ALIGNED_ALLOCATION when
targeting systems that don't support aligned allocation.

By "system that don't support aligned allocation" we mean "systems with older
libc++ dylibs", right? Isn't that what this code is intended to do?

https://github.com/llvm-mirror/libcxx/blob/master/include/__config#L946

If we could take an approach like this to disable the aligned new/delete when
users target a min version that doesn't have aligned new/delete, then I think
the one macro solution makes sense.

However, if there are cases where it's possible the user wants to use a custom
libc++, which does have aligned alloc support despite the system dylib lacking
it, then I think a two macro solution makes the most sense.

Also, we might need a second macro anyway because libc++ needs to build the
aligned new/delete versions in the dylib even if users don't want to call them
from their code (to achieve older dylib support).

Is it alright if we leave these commits in for now and fix your issues in the
next week?

[Quuxplusone]

Also, I think I link error is preferable to silently calling unaligned new for overaligned types and getting a runtime bug.

[Quuxplusone]

On further consideration, I think a two macro solution might be best.

_LIBCPP_HAS_NO_ALIGNED_ALLOCATION which denotes if (A) the system supports a C aligned allocation interface, and (B) the compiler has aligned allocation enabled.

And _LIBCPP_DISABLE_ALIGNED_ALLOCATION (name is just a suggestion), which denotes that aligned allocation shouldn't be used at runtime.

[Quuxplusone]

(In reply to Eric Fiselier from comment #18)

By "system that don't support aligned allocation" we mean "systems with older libc++ dylibs", right? Isn't that what this code is intended to do?

https://github.com/llvm-mirror/libcxx/blob/master/include/__config#L946

You are right, I mean systems with older libc++ dylibs. Check in __config covers if posix_memalign is available and if aligned allocations can be implemented at all. But a bunch of macOS versions don't have libc++ with aligned allocations even if they could. That state is captured by

https://github.com/llvm-mirror/clang/blob/85c1a32cd398e0e5dbb965a4e0d8ee25899cde62/include/clang/Basic/AlignedAllocation.h#L25-L40

I agree that link error is better than silent misalignment at runtime. Also relevant is https://reviews.llvm.org/D34574 I agree with your reasoning about one macro/two macros but I'll leave the final decision to you. For my specific case both solutions should work but two macros are more flexible and should cover more use cases, not only the one I've described.

It is OK to leave these commits in for now, we still have sufficient test coverage for other configurations.

[Quuxplusone]

Thanks for the link to the Clang patch, I was just about to ask how to implement the second macro.

I'll also add a warning when over-aligned new requests are made but are not supported by the library.

[Quuxplusone]

A couple of reviews related to this PR:

(1) [Clang] Allow libc++ to detect when aligned allocation is unavailable: https://reviews.llvm.org/D45015

(2) [libc++] Generate warning when over-aligned new call is required but not supported: https://reviews.llvm.org/D45013

(3) [LLVM] Support aligned new/delete functions: https://reviews.llvm.org/D44769

I'm not sure if (2) is needed in light of D34574, but it might still be useful to not depend on the Clang diagnostic.