Page MenuHomePhabricator

[IRMemoryMap] Fix the alignment adjustment in Malloc
ClosedPublic

Authored by vsk on May 30 2018, 12:38 PM.

Details

Summary

This prevents Malloc from allocating the same chunk of memory twice, as
a byproduct of an alignment adjustment which gave the client access to
unallocated memory.

Prior to this patch, the newly-added test failed with:

$ lldb-test ir-memory-map ... ir-memory-map-overlap1.test
Command: malloc(size=8, alignment=16)
Malloc: address = 0x1000cd000
Command: malloc(size=16, alignment=8)
Malloc: address = 0x1000cd010
Command: malloc(size=64, alignment=32)
Malloc: address = 0x1000cd020
Command: malloc(size=1, alignment=8)
Malloc: address = 0x1000cd060
Command: malloc(size=64, alignment=32)
Malloc: address = 0x1000cd080
Command: malloc(size=64, alignment=8)
Malloc: address = 0x1000cd0b0
Malloc error: overlapping allocation detected, previous allocation at [0x1000cd080, 0x1000cd0c0)

I don't see anything controversial here (in fact Jim lgtm'd part of this patch off-list), but as this is unfamiliar territory for me I think it'd help to have a proper review. Depends on D47508.

Diff Detail

Event Timeline

vsk created this revision.May 30 2018, 12:38 PM
vsk edited the summary of this revision. (Show Details)
vsk updated this revision to Diff 149198.May 30 2018, 2:11 PM
vsk added a reviewer: lhames.
  • Don't insert extra padding bytes when alignment = 1.
  • + Lang

LGTM.

I haven't looked at process memory management. How hard would your FIXME be to implement?

  • Lang.

I don't know much about IRMemoryMap myself, but this does seem uncontroversial. Nonetheless, I did manage to find something to rip into. :D

source/Expression/IRMemoryMap.cpp
311–315

allocation_size = llvm::alignTo(size, alignment)

323–324

I think this should be just allocation_size += alignment -1. The subsequent realignment cannot eat more than alignment-1 bytes.

vsk added a comment.May 31 2018, 1:58 PM

LGTM.

I haven't looked at process memory management. How hard would your FIXME be to implement?

After looking at this more carefully, I think the FIXME makes a bad prescription. It's based on the assumption that each platform-specific allocator makes use of an efficient API to make aligned allocations. This doesn't generally seem to be true, because the platform-specific allocators rely on mmap().

A better alternative might be to add a new API, AllocatedMemoryCache::AllocateAlignedMemory(size, alignment). You could implement an alignment-aware allocator here, say, by splitting up free memory into a list of buckets (one list per alignment). Next, you could surface the API from Process, provided there's a fallback strategy when the allocated memory cache is disabled.

One caveat to all of this is that it might not be worth doing unless fragmentation-related overhead is identified as a bottleneck.

I'll remove the FIXME.

vsk updated this revision to Diff 149355.May 31 2018, 2:00 PM
vsk marked 2 inline comments as done.
  • Address Pavel's feedback, remove a questionable FIXME.
This revision was not accepted when it landed; it landed in state Needs Review.May 31 2018, 3:14 PM
This revision was automatically updated to reflect the committed changes.