Page MenuHomePhabricator

[libcxxabi] Correctly align fallback heap
AbandonedPublic

Authored by olista01 on Oct 27 2015, 7:26 AM.

Details

Summary

The fallback malloc in libcxxabi (used to allocate space for exception
objects in out-of-memory situations) defines its heap as an array of
chars, but casts it to a struct containing shorts before accessing it.
Sometimes, the heap does not get placed on a 2-byte boundary, so
accesses to it caused unaligned access faults on targets that do not
support unaligned accesses.

The fix is to specify the alignment of the heap array, so that it will
always be sufficient for a heap_node.

This is still technically invoking undefined behaviour, as it is
accessing an object of type "char array" through an lvalue of a
different type. However, I don't think it is possible to write malloc
without violating that rule, and we have tests covering this.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 38543.Oct 27 2015, 7:26 AM
olista01 retitled this revision from to [libcxxabi] Correctly align fallback heap.
olista01 updated this object.
olista01 added reviewers: mclow.lists, compnerd.
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: cfe-commits.

Using alignas(heap_node) might be a little clearer and more semantically correct here. Should be OK support-wise, libc++ is already using that for some of its bits. Otherwise, good find!

Tim.

EricWF added a subscriber: EricWF.Oct 27 2015, 9:17 AM

This patch is incomplete and incorrect. The heap actually needs to be aligned to a 16 byte boundary, and all pointers returned from it must also be 16 byte aligned. I have a complete fix for this issue as D12669.

A fix for the non-fallback case can be found D12512. Also see https://llvm.org/bugs/show_bug.cgi?id=24604.

Oh, and I think it can probably be justified under C++, though I've not quite joined up all the dots. The "object lifetime" rules seem to bless declaring a char array dead and reusing its storage for another purpose. Since heap_node has trivial initialization, you probably don't even need to do anything special to make it a heap.

I could be wrong there though.

@t.p.northover @olista01 A char array can legally alias any other type memory AFAIK. Its perfectly legal to use a char array to provide raw memory. There shouldn't be any undefined behavior here.

olista01 abandoned this revision.Oct 27 2015, 9:39 AM

Ok, I'll abandon this patch and wait for Eric's.

I think the char* aliasing rule only works one way, i.e. any object can be accessed through an lvalue of type char, but not the other way round (c++11, 3.10/10). I didn't know about the "object lifetime" rules though, maybe they allow it.