Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Ed Maste via cfe-commits]

emaste added a comment.

Note that the addition of the referenceCount at the beginning of 
`__cxa_exception` for 64-bit builds causes the `_Unwind_Exception unwindHeader` 
to become misaligned. libstdc++ had this same issue 
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38732, now fixed) and libcxxrt 
does as well (https://github.com/pathscale/libcxxrt/issues/38).


https://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Ed Maste via cfe-commits]

emaste added inline comments.


Comment at: test/test_cxa_allocate_exception.pass.cpp:34
@@ +33,3 @@
+const std::size_t required_alignment = alignof(__cxa_exception);
+const bool requires_over_alignment = max_alignment < required_alignment;
+

`requires_over_alignment` not used?


https://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Ed Maste via cfe-commits]

emaste added a comment.

As it happens on FreeBSD/i386 malloc returns a 16-byte-aligned pointer for 
allocations with size >= 16 so will not be affected by this issue.


https://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Dan Albert via cfe-commits]

danalbert added a comment.

The configure-time check LGTM. Android has it for all recent versions (since 
JB), but that will cover the case of GB and ICS.



Comment at: src/cxa_exception.cpp:127
@@ +126,3 @@
+// on 32 bit targets.
+ptr = std::malloc(size);
+#endif

`#warning` until that's done?


https://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Ed Maste via cfe-commits]

emaste added a comment.

Is this going to be committed?
Libunwind is about to grow the same alignment attribute 
(https://reviews.llvm.org/D22543)


https://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Eric Fiselier via cfe-commits]

EricWF added a comment.

Whats preventing this from landing? @joerg @danalbert Do we want to use 
"posix_memalign" on Android or not?


http://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Joerg Sonnenberger via cfe-commits]

On Fri, Sep 04, 2015 at 09:28:39PM +, Eric Fiselier via cfe-commits wrote:
> EricWF added a comment.
> 
> In http://reviews.llvm.org/D12512#237175, @joerg wrote:
> 
> > Please don't commit this as is. Many platforms have posix_memalign or 
> > equivalent, which makes this both simpler and potentially without wasting 
> > memory. Compare e.g. http://reviews.llvm.org/D12001.
> 
> 
> Will do. Any advice on detecting posix_memalign? I don't see anything in the 
> patch you pointed me to.

Provide a feature variable for it and set it by default. For Unix-like
systems, I think only Android wants to disable it by default. Not sure
about Windows and OSX.

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Dan Albert via cfe-commits]

danalbert added a comment.

Android has posix_memalign too.


http://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Dan Albert via cfe-commits]

Android has posix_memalign too.
On Sep 5, 2015 12:00, "Joerg Sonnenberger"  wrote:

> On Fri, Sep 04, 2015 at 09:28:39PM +, Eric Fiselier via cfe-commits
> wrote:
> > EricWF added a comment.
> >
> > In http://reviews.llvm.org/D12512#237175, @joerg wrote:
> >
> > > Please don't commit this as is. Many platforms have posix_memalign or
> equivalent, which makes this both simpler and potentially without wasting
> memory. Compare e.g. http://reviews.llvm.org/D12001.
> >
> >
> > Will do. Any advice on detecting posix_memalign? I don't see anything in
> the patch you pointed me to.
>
> Provide a feature variable for it and set it by default. For Unix-like
> systems, I think only Android wants to disable it by default. Not sure
> about Windows and OSX.
>
> Joerg
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Joerg Sonnenberger via cfe-commits]

On Sat, Sep 05, 2015 at 07:29:43PM +, Dan Albert via cfe-commits wrote:
> danalbert added a comment.
> 
> Android has posix_memalign too.

Yes, but people seem to be reluctant to use it. See the linked review.

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Joerg Sonnenberger via cfe-commits]

joerg added a subscriber: joerg.
joerg requested changes to this revision.
joerg added a reviewer: joerg.
joerg added a comment.
This revision now requires changes to proceed.

Please don't commit this as is. Many platforms have posix_memalign or 
equivalent, which makes this both simpler and potentially without wasting 
memory. Compare e.g. http://reviews.llvm.org/D12001.


http://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Jonathan Roelofs via cfe-commits]

jroelofs added a comment.

In http://reviews.llvm.org/D12512#236988, @majnemer wrote:

> In http://reviews.llvm.org/D12512#236987, @EricWF wrote:
>
> > In http://reviews.llvm.org/D12512#236984, @majnemer wrote:
> >
> > > Wouldn't this change be problematic if you threw to code which was 
> > > statically linked with a prior version of libcxxabi?
> >
> >
> > How do you mean? As in you have two different versions of libc++abi linked 
> > into one executable? If so your already in bad shape.
>
>
> Say you have two binaries, foo.exe and bar.so.  Foo.exe statically links 
> against an older libc++abi and bar.so links against a newer libc++abi.  In 
> this instance, our program has two copies of libc++abi statically linked with 
> no ill effects and such a scenario was supported before this patch (at least 
> AFAICT).
>
> However, we might have problems after this patch if foo.exe is linked against 
> a newer static library than bar.so


I don't think that is/was supported. The abi library holds some global state, 
which wouldn't be shared between the two instances of it.


http://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Saleem Abdulrasool via cfe-commits]

compnerd added a comment.

While I can see the argument you raise, the saving grace here is that this is 
buried in libc++abi.  The only real piece of libc++abi of real use to users is 
`__cxa_demangle` (which is only relevant for Windows, and Darwin where you have 
two level namespaces).  For everyone else, libc++abi is merely an 
implementation detail for libc++ and will not be linked to directly.  If you 
are mixing and matching the ABI support interfaces, or the runtime, you are 
already in a pretty bad shape, and allowing things to break in such a 
circumstances doesn't seem entirely terrible.  Unfortunately, for compatibility 
with GCC, we do need to maintain this alignment, which is not guaranteed 
without this patch.


http://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12512: [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[Eric Fiselier via cfe-commits]

EricWF added a comment.

In http://reviews.llvm.org/D12512#236988, @majnemer wrote:

> In http://reviews.llvm.org/D12512#236987, @EricWF wrote:
>
> > In http://reviews.llvm.org/D12512#236984, @majnemer wrote:
> >
> > > Wouldn't this change be problematic if you threw to code which was 
> > > statically linked with a prior version of libcxxabi?
> >
> >
> > How do you mean? As in you have two different versions of libc++abi linked 
> > into one executable? If so your already in bad shape.
>
>
> Say you have two binaries, foo.exe and bar.so.  Foo.exe statically links 
> against an older libc++abi and bar.so links against a newer libc++abi.  In 
> this instance, our program has two copies of libc++abi statically linked with 
> no ill effects and such a scenario was supported before this patch (at least 
> AFAICT).
>
> However, we might have problems after this patch if foo.exe is linked against 
> a newer static library than bar.so


If foo.exe is already getting `__cxa_allocate_exception(...)` from one 
libc++abi version and `__cxa_free_exception` from another then they are already 
in trouble because `__cxa_free_exception(ptr)` checks to see if `ptr` has been 
allocated from a builtin buffer.  This only happens when malloc is out of 
memory but it would still be an existing problem in the scenario you describe.

Furthermore the new code is only executed in cases where we already have 
undefined behavior.  We don't add any extra padding to the pointers returned 
from malloc unless not doing so would lead to UB.

I know if multiple instances of libc++abi in one executable is not supported on 
OS X (see FAQ at the bottom of libcxxabi.llvm.org). I don't know if thats 
supported on linux though. People really shouldn't be using a static libc++abi. 
In order for the exception handling globals to work there should only be one 
copy of the in the entire program.

Hopefully this change isn't too problematic and thanks for looking at it.


http://reviews.llvm.org/D12512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits