Page MenuHomePhabricator

[libcxxabi] Fix alignment of pointers returned by fallback_malloc
Needs ReviewPublic

Authored by EricWF on Sep 6 2015, 11:20 PM.

Details

Summary

Currently the pointers returned by fallback_malloc do not have *ANY* alignment guarantees. This is caused by two different problems is fallback_malloc.ipp.

The first reason is because the heap array used by fallback malloc only has an alignment requirement of '1'. Currently we try and put the first heap_node directly at the start of heap even though 'heap_node' has an alignment requirement of at least 2 bytes. This patch fixes this issue by manually aligning heap using alignas(heap_node).

The second reason is because fallback_malloc returns the pointers that are exactly sizeof(heap_node) bytes after the heap_node header itself. Because heap_nodes only have an alignment requirement of '2' the resulting pointers also only have an alignment requirement of '2' even though an alignment of '16' bytes is required. This patch fixes this problem by
manually requiring that all heap_nodes have an address that is 4 bytes before a 16 byte boundary.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 34127.Sep 6 2015, 11:20 PM
EricWF retitled this revision from to [libcxxabi] Fix alignment of pointers returned by fallback_malloc.
EricWF updated this object.
EricWF added a subscriber: cfe-commits.
compnerd edited edge metadata.Sep 7 2015, 9:37 AM

While I think that ensuring that the fallback malloc path works properly is needed, AIUI, this is still insufficient, as there is a first attempt at using malloc, which doesn't have alignment guarantees (except on Darwin). Also, a clang-format over the patch would be appreciated.

src/fallback_malloc.ipp
25

Ick, but I see why this is needed. Might be nice to expand the comment about not being able to include headers with a why.

69

Make this a set of static_asserts?

104

Why the movement here?

EricWF added a comment.Sep 7 2015, 2:57 PM

While I think that ensuring that the fallback malloc path works properly is needed, AIUI, this is still insufficient, as there is a first attempt at using malloc, which doesn't have alignment guarantees (except on Darwin).

I split this change into two parts. D12512 fixes the first attempt with malloc. This patch only touches fallback_malloc.

Also, a clang-format over the patch would be appreciated.

I would appreciate if I could delay that until after this patch. fallback_malloc.ipp is currently a mess and requires a lot of cleanup. However I wanted to avoid obscuring these changes by having a bunch of cleanup.

src/fallback_malloc.ipp
25

I plan to rip this out when I make fallback_malloc.ipp a proper header. I'll do that right after this patch lands. I guess I didn't need this change after all.

EricWF added inline comments.Sep 7 2015, 4:56 PM
src/fallback_malloc.ipp
69

I was going to but I wasn't 100% sure this would be true on all platforms.

104

Just trying to better group the functions. I wanted to put this closer to the definition of heap and freelist.

jroelofs added inline comments.Oct 27 2015, 9:53 AM
src/fallback_malloc.ipp
69

Better to have it fail at compile time so we can at least detect this and decide what to do about it, no?