Issue 115280044: Implemented the ZebraHeap (Closed)
|
Created:
9 years, 9 months ago by peterssen Modified:
9 years, 9 months ago CC:
sawbuck-changes_googlegroups.com Base URL:
http://sawbuck.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionImplemented the ZebraHeap
A zebra-stripe heap allocates a (maximum) predefined amount of memory
and serves allocation requests with size less or equal to the system page
size.
It divides the memory pages into "even" and "odd" types (like zebra-stripes).
all the allocations are done in the even pages, just before the "odd" pages.
The "odd" pages can be protected againt read/write which gives a basic
mechanism for detecting buffer overflows.
BUG=
R=chrisha@chromium.org, sebmarchand@chromium.org
Committed: https://code.google.com/p/sawbuck/source/detail?r=2206
Patch Set 1 : ZebraHeap cleanup #Patch Set 2 : Edited misleading comment #Patch Set 3 : Fixed some typos #
Total comments: 82
Patch Set 4 : Major cleanup, renamed to ZebraBlockHeap #Patch Set 5 : Typos #
Total comments: 28
Patch Set 6 : Cleanup and two more tests added #
Total comments: 24
Patch Set 7 : More cleanup and one test improved #
Total comments: 27
Patch Set 8 : Minor fixes #
Total comments: 8
Patch Set 9 : More fixes (check test AllocateBlockCornerCases) #Patch Set 10 : Augmented test AllocateBlockCornerCases #
Total comments: 3
Patch Set 11 : Fixed typo and removed unused #include #Patch Set 12 : Some formatting and removed duplicate values in AllocateBlockCornerCases #Patch Set 13 : Rebase #
MessagesTotal messages: 20
PTAL.
Sign in to reply to this message.
Nice start, a first round of comments mostly about the style. I haven't reviewed the logic in detail yet but I'd like you to fix all these comments to get familiar with the code review process before going into the details. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap.cc (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:21: ZebraHeap::ZebraHeap() : ZebraHeap(size_t(16 * (1 << 20))) { This should be put in a constant somewhere. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:22: // Empty. No need for the empty comment. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:27: VirtualAlloc(NULL, heap_size_, You could probably use the DirectAllocation heap here, but check with Chris for what he had in mind for this class. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:30: CHECK_NE(reinterpret_cast<uint8*>(NULL), heap_address_); use a DCHECK rather than a check here, it's a programmer error and should be catch in a debug build, no need to do this in release. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:34: CHECK(common::IsAligned(heap_address_, page_size_)); Same thing, CHECK is reserved for operations that you can't disable in Release... https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:84: allocated_address_[stripe_index] = alloc; Do you really need to do this ? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:118: size_t min_left_redzone_size, Indent to the parenthesis. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:132: layout); Indent + 2, or to the parenthesis if you can. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:135: layout->header_padding_size + layout->body_size; Indent + 2. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap.h (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:24: #include <algorithm> Only include what you need, i.e. in this case include <algorithm> in the .cc file. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:42: // all the allocations are done in the even pages, just before the "odd" pages. s/all/All/ https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:51: // Constructor. Creates a heap with 16MB initial memory. A static method should be exposed to set this default value. Like here: https://code.google.com/p/syzygy/source/browse/syzygy/agent/asan/asan_heap.h#92 https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:69: size_t min_left_redzone_size, align to the parenthesis. (or at +4 if you can't indent to the parenthesis without breaking the 80 characters rule) https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:84: // @note address can be ANY address inside the stripe. s/ANY/any. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:96: size_t max_number_of_allocations_; Not sure that you really need this one, as it's always equal to stripe_count_ / 2. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:99: // When a stripe is freed is pushed again to the queue. Incomplete sentence ? Maybe s/freed is/freed it is/ ? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:48: TestZebraHeap() : ZebraHeap(8 * (1 << 20)) {} Put this value in a constant. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:69: for (size_t i = 1; i < page_size - 2 * kShadowRatio; i += 23) { Replace the maximum size by something like page_size - sizeof(BlockHeader) - sizeof(BlockTrailer) - 7 ? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:83: // Using a 1 MB heap. Again, please use a constant here. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:94: void* alloc = h.AllocateBlock(body_size, header_size, Hum, the second and third arguments correspond to the left and right redzone sizes. They're always respectively >= to sizeof(BlockHeader) and sizeof(BlockTrailer), so in this case the 2 outer loops are useless... https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:147: const size_t kLimit = 100000; You should rather use TestZebraHeap::max_number_of_allocations_ as a limit and ensure that the next call to Allocate fail.
Sign in to reply to this message.
Looking pretty good for a first pass! https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap.cc (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:21: ZebraHeap::ZebraHeap() : ZebraHeap(size_t(16 * (1 << 20))) { Please use static_cast<size_t>(...) https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:64: return kOpaqueHeap; This should be a 'transparent' heap, as we are the implementers and will be able to provide *full* details to the runtime. Opaque heaps are for heaps whose implementation we don't control (ie: the windows internal heap...) https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:88: bool ZebraHeap::Free(void* alloc) { Don't you have to handle NULL as a special case here? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:99: // The addres must match the one returned to the caller by Allocate. address* https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:131: std::max(min_right_redzone_size, page_size_), You should simply return false if min_right_redzone_size > kPageSize, as this heap simply can't support an allocation of that shape. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:133: What happens if the allocation is too big? This heap can only support the allocations that are less than a page in size. After BlockPlanLayout the block_size should be exactly 2 * kPageSize, as a sanity check. If not, return false. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap.h (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:45: class ZebraHeap : public BlockHeapInterface { This should be ZebraBlockHeap, as its a block heap. Similarly, the files should be renamed to zebra_block_heap* https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:52: ZebraHeap(); No need to have a default constructor. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:68: virtual void* AllocateBlock(size_t size, Move size_t size down to its own line. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:69: size_t min_left_redzone_size, Indent +2. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:85: size_t GetStripeIndex(void* address); const void* address https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:88: size_t page_size_; No need for this, as its available as a constant kPageSize from constants.h. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:100: std::queue<size_t> free_stripes_; Document that freed allocations are pushed to the queue, while new allocations are taken from the *front* of the queue, so as to maximize the time a freed page is freed . https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:22: #include "syzygy/agent/asan/heaps/win_heap.h" You don't need this include? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:37: typedef std::set<BlockInfo, BlockInfoLessThan> BlockInfoSet; This is copy-pasted from another file... time to move it to unittest_util.h? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:88: size_t page_size = DirectAllocation::GetPageSize(); Rather than using DirectAllocation::GetPageSize (which is going to disappear shortly), you should use kPageSize from constants.h. (Ditto above) https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:120: TEST(ZebraHeapTest, AllocateSizeLimits) { A similar test for AllocateBlock? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:135: // Impossible allocation size Missing trailing period. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:136: EXPECT_EQ(reinterpret_cast<void*>(NULL), h.Allocate(page_size + 1)); Test more impossible sizes here... https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:139: TEST(ZebraHeapTest, AllocateUntilFull) { A similar test for allocate block?
Sign in to reply to this message.
PTAL. I renamed the files, the comments were lost, but I fixed everything. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap.cc (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:21: ZebraHeap::ZebraHeap() : ZebraHeap(size_t(16 * (1 << 20))) { On 2014/07/28 17:40:44, Sébastien Marchand wrote: > This should be put in a constant somewhere. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:21: ZebraHeap::ZebraHeap() : ZebraHeap(size_t(16 * (1 << 20))) { On 2014/07/28 17:40:44, Sébastien Marchand wrote: > This should be put in a constant somewhere. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:22: // Empty. On 2014/07/28 17:40:45, Sébastien Marchand wrote: > No need for the empty comment. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:27: VirtualAlloc(NULL, heap_size_, On 2014/07/28 17:40:45, Sébastien Marchand wrote: > You could probably use the DirectAllocation heap here, but check with Chris for > what he had in mind for this class. Acknowledged. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:30: CHECK_NE(reinterpret_cast<uint8*>(NULL), heap_address_); On 2014/07/28 17:40:44, Sébastien Marchand wrote: > use a DCHECK rather than a check here, it's a programmer error and should be > catch in a debug build, no need to do this in release. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:34: CHECK(common::IsAligned(heap_address_, page_size_)); On 2014/07/28 17:40:45, Sébastien Marchand wrote: > Same thing, CHECK is reserved for operations that you can't disable in > Release... Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:64: return kOpaqueHeap; Need a more detailed explanation about the "opacity" of the heaps. On 2014/07/28 17:45:17, chrisha wrote: > This should be a 'transparent' heap, as we are the implementers and will be able > to provide *full* details to the runtime. Opaque heaps are for heaps whose > implementation we don't control (ie: the windows internal heap...) https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:84: allocated_address_[stripe_index] = alloc; Is a good way to check in the Free function that the address is the correct one (the same returned by Allocated). For block allocations (AllocateBlock) it will be always the beginning of the page. For normal allocations (Allocate) this is not necessarily the beginning of the page. On 2014/07/28 17:40:45, Sébastien Marchand wrote: > Do you really need to do this ? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:88: bool ZebraHeap::Free(void* alloc) { On 2014/07/28 17:45:17, chrisha wrote: > Don't you have to handle NULL as a special case here? Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:99: // The addres must match the one returned to the caller by Allocate. On 2014/07/28 17:45:17, chrisha wrote: > address* Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:118: size_t min_left_redzone_size, On 2014/07/28 17:40:45, Sébastien Marchand wrote: > Indent to the parenthesis. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:131: std::max(min_right_redzone_size, page_size_), On 2014/07/28 17:45:17, chrisha wrote: > You should simply return false if min_right_redzone_size > kPageSize, as this > heap simply can't support an allocation of that shape. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:132: layout); On 2014/07/28 17:40:45, Sébastien Marchand wrote: > Indent + 2, or to the parenthesis if you can. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:133: On 2014/07/28 17:45:17, chrisha wrote: > What happens if the allocation is too big? This heap can only support the > allocations that are less than a page in size. After BlockPlanLayout the > block_size should be exactly 2 * kPageSize, as a sanity check. If not, return > false. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.cc:135: layout->header_padding_size + layout->body_size; On 2014/07/28 17:40:44, Sébastien Marchand wrote: > Indent + 2. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap.h (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:24: #include <algorithm> On 2014/07/28 17:40:47, Sébastien Marchand wrote: > Only include what you need, i.e. in this case include <algorithm> in the .cc > file. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:42: // all the allocations are done in the even pages, just before the "odd" pages. On 2014/07/28 17:40:47, Sébastien Marchand wrote: > s/all/All/ Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:45: class ZebraHeap : public BlockHeapInterface { On 2014/07/28 17:45:18, chrisha wrote: > This should be ZebraBlockHeap, as its a block heap. Similarly, the files should > be renamed to zebra_block_heap* Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:51: // Constructor. Creates a heap with 16MB initial memory. On 2014/07/28 17:40:47, Sébastien Marchand wrote: > A static method should be exposed to set this default value. Like here: > https://code.google.com/p/syzygy/source/browse/syzygy/agent/asan/asan_heap.h#92 Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:52: ZebraHeap(); On 2014/07/28 17:45:18, chrisha wrote: > No need to have a default constructor. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:52: ZebraHeap(); The default constructor enforces the use of default_heap_size_ in the heap construction. On 2014/07/28 17:45:18, chrisha wrote: > No need to have a default constructor. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:68: virtual void* AllocateBlock(size_t size, On 2014/07/28 17:45:18, chrisha wrote: > Move size_t size down to its own line. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:84: // @note address can be ANY address inside the stripe. On 2014/07/28 17:40:46, Sébastien Marchand wrote: > s/ANY/any. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:85: size_t GetStripeIndex(void* address); On 2014/07/28 17:45:18, chrisha wrote: > const void* address Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:88: size_t page_size_; On 2014/07/28 17:45:17, chrisha wrote: > No need for this, as its available as a constant kPageSize from constants.h. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:96: size_t max_number_of_allocations_; This is to handle correctly the case when the last page is "even". Suppose we allocate a block in the last page, then the trailer should occupy the immediately next page (overflow), in that case there is no guarantee that the memory is accessible because is beyond the reserved space. On 2014/07/28 17:40:47, Sébastien Marchand wrote: > Not sure that you really need this one, as it's always equal to stripe_count_ / > 2. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:96: size_t max_number_of_allocations_; On 2014/07/28 17:40:47, Sébastien Marchand wrote: > Not sure that you really need this one, as it's always equal to stripe_count_ / > 2. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:99: // When a stripe is freed is pushed again to the queue. On 2014/07/28 17:40:47, Sébastien Marchand wrote: > Incomplete sentence ? Maybe s/freed is/freed it is/ ? Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap.h:100: std::queue<size_t> free_stripes_; On 2014/07/28 17:45:18, chrisha wrote: > Document that freed allocations are pushed to the queue, while new allocations > are taken from the *front* of the queue, so as to maximize the time a freed page > is freed . Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... File syzygy/agent/asan/heaps/zebra_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:22: #include "syzygy/agent/asan/heaps/win_heap.h" On 2014/07/28 17:45:18, chrisha wrote: > You don't need this include? Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:37: typedef std::set<BlockInfo, BlockInfoLessThan> BlockInfoSet; Removed, I'll just use a vector. On 2014/07/28 17:45:18, chrisha wrote: > This is copy-pasted from another file... time to move it to unittest_util.h? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:48: TestZebraHeap() : ZebraHeap(8 * (1 << 20)) {} On 2014/07/28 17:40:48, Sébastien Marchand wrote: > Put this value in a constant. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:69: for (size_t i = 1; i < page_size - 2 * kShadowRatio; i += 23) { Changed to kPageSize. If the header an trailer are empty, the allowed allocation size is from 0 to kPageSize (inclusive). On 2014/07/28 17:40:48, Sébastien Marchand wrote: > Replace the maximum size by something like page_size - sizeof(BlockHeader) - > sizeof(BlockTrailer) - 7 ? https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:83: // Using a 1 MB heap. On 2014/07/28 17:40:48, Sébastien Marchand wrote: > Again, please use a constant here. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:88: size_t page_size = DirectAllocation::GetPageSize(); On 2014/07/28 17:45:18, chrisha wrote: > Rather than using DirectAllocation::GetPageSize (which is going to disappear > shortly), you should use kPageSize from constants.h. (Ditto above) Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:94: void* alloc = h.AllocateBlock(body_size, header_size, On 2014/07/28 17:40:48, Sébastien Marchand wrote: > Hum, the second and third arguments correspond to the left and right redzone > sizes. They're always respectively >= to sizeof(BlockHeader) and > sizeof(BlockTrailer), so in this case the 2 outer loops are useless... Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:120: TEST(ZebraHeapTest, AllocateSizeLimits) { On 2014/07/28 17:45:18, chrisha wrote: > A similar test for AllocateBlock? Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:135: // Impossible allocation size On 2014/07/28 17:45:18, chrisha wrote: > Missing trailing period. Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:136: EXPECT_EQ(reinterpret_cast<void*>(NULL), h.Allocate(page_size + 1)); On 2014/07/28 17:45:18, chrisha wrote: > Test more impossible sizes here... Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:139: TEST(ZebraHeapTest, AllocateUntilFull) { On 2014/07/28 17:45:18, chrisha wrote: > A similar test for allocate block? Done. https://codereview.appspot.com/115280044/diff/70001/syzygy/agent/asan/heaps/z... syzygy/agent/asan/heaps/zebra_heap_unittest.cc:147: const size_t kLimit = 100000; On 2014/07/28 17:40:48, Sébastien Marchand wrote: > You should rather use TestZebraHeap::max_number_of_allocations_ as a limit and > ensure that the next call to Allocate fail. Done.
Sign in to reply to this message.
Looking good! A handful more comments. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:30: VirtualAlloc(NULL, heap_size_, Please refer to this as ::VirtualAlloc. (We use the root namespace specifically when calling CRT functions...) https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:131: return NULL; I don't think you need lines 128 - 131, as that condition is tested by 'Allocate', which you call at lien 157. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:142: kShadowRatio, Align all of these with the first parameter 'kPageSize'. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.h (right): https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:47: ZebraBlockHeap(); Just to keep things simple I think we can just get rid of the default constructor, the static default_heap_size_, its mutator, etc. I don't see any real need for them. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:50: // @param heap_size The (initial) amount of memory reserved by the heap. s/(initial)// https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:71: } No need for the curly braces around a one-line for loop body. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:95: EXPECT_TRUE(IsAligned(block.block + block.block_size, kShadowRatio)); Test that the header is kPageSize aligned, and that the right redzone + trailer is >= kPageSize as well? Double check that the entire allocation is exactly 2 pages in size? https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:109: TEST(ZebraBlockHeapTest, AllocateSizeLimits) { Can you do this same test again with AllocateBlock, and ensure that the largest block body that can be allocated is kPageSize - sizeof(BlockHeader)? https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:130: uint8* alloc = reinterpret_cast<uint8*>(h.Allocate(0xFF)); You should be expecting alloc != NULL. Similarly, you could test that the number of allocations made is == i, or the number of free pages is equal to max_number_of_allocations_ - i... https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:134: break; This should never occur. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:147: EXPECT_LE(kPageSize, static_cast<size_t>(buffers[i] - buffers[i - 1])); Good test! https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:164: buffers.push_back(alloc); Need to expect that alloc != NULL. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:166: break; Ditto, this should never happen.
Sign in to reply to this message.
PTAL. All previous issues fixed and two more tests added. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:30: VirtualAlloc(NULL, heap_size_, On 2014/07/29 17:26:11, chrisha wrote: > Please refer to this as ::VirtualAlloc. > > (We use the root namespace specifically when calling CRT functions...) Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:131: return NULL; On 2014/07/29 17:26:11, chrisha wrote: > I don't think you need lines 128 - 131, as that condition is tested by > 'Allocate', which you call at lien 157. Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:142: kShadowRatio, On 2014/07/29 17:26:11, chrisha wrote: > Align all of these with the first parameter 'kPageSize'. Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:152: if ((layout->trailer_size + layout->trailer_padding_size) - kPageSize >= 8) Should be kShadowRatio and not a hardcoded 8. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.h (right): https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:47: ZebraBlockHeap(); On 2014/07/29 17:26:11, chrisha wrote: > Just to keep things simple I think we can just get rid of the default > constructor, the static default_heap_size_, its mutator, etc. I don't see any > real need for them. Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:50: // @param heap_size The (initial) amount of memory reserved by the heap. On 2014/07/29 17:26:11, chrisha wrote: > s/(initial)// Field default_heap_size_ removed. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:71: } On 2014/07/29 17:26:11, chrisha wrote: > No need for the curly braces around a one-line for loop body. Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:95: EXPECT_TRUE(IsAligned(block.block + block.block_size, kShadowRatio)); On 2014/07/29 17:26:11, chrisha wrote: > Test that the header is kPageSize aligned, and that the right redzone + trailer > is >= kPageSize as well? > > Double check that the entire allocation is exactly 2 pages in size? Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:95: EXPECT_TRUE(IsAligned(block.block + block.block_size, kShadowRatio)); On 2014/07/29 17:26:11, chrisha wrote: > Test that the header is kPageSize aligned, and that the right redzone + trailer > is >= kPageSize as well? > > Double check that the entire allocation is exactly 2 pages in size? Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:109: TEST(ZebraBlockHeapTest, AllocateSizeLimits) { On 2014/07/29 17:26:11, chrisha wrote: > Can you do this same test again with AllocateBlock, and ensure that the largest > block body that can be allocated is kPageSize - sizeof(BlockHeader)? Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:130: uint8* alloc = reinterpret_cast<uint8*>(h.Allocate(0xFF)); On 2014/07/29 17:26:11, chrisha wrote: > You should be expecting alloc != NULL. Similarly, you could test that the number > of allocations made is == i, or the number of free pages is equal to > max_number_of_allocations_ - i... Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:134: break; On 2014/07/29 17:26:11, chrisha wrote: > This should never occur. Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:147: EXPECT_LE(kPageSize, static_cast<size_t>(buffers[i] - buffers[i - 1])); On 2014/07/29 17:26:11, chrisha wrote: > Good test! Acknowledged. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:164: buffers.push_back(alloc); On 2014/07/29 17:26:11, chrisha wrote: > Need to expect that alloc != NULL. Done. https://codereview.appspot.com/115280044/diff/160001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:166: break; On 2014/07/29 17:26:11, chrisha wrote: > Ditto, this should never happen. Done.
Sign in to reply to this message.
More comments... getting there! https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:56: if (::VirtualFree(heap_address_, 0, MEM_RELEASE) == FALSE) CHECK_EQ(TRUE, ::VirtualFree ... ) should suffice. There's no real reason for this to fail where a log message would be meaningful. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:79: uint8* alloc = page_address + kPageSize - bytes; All allocations should be kShadowRatio aligned: uint8* alloc = page_address + kPageSize - common::AlignUp(bytes, kShadowRatio); Also adjust your unittests to reflect this fact. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:142: // There should be less than kShadowsRatio bytes between the body end kShadowRatio* https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:149: if (right_zone_size < kPageSize) I'd do this check *before* the one above (otherwise right_zone_size - kPageSize could be negative, and will map to a large unsigned number). The code is correct as is, but is more intuitive if you swap the order. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.h (right): https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:40: // It divides the memory pages into "even" and "odd" types (like zebra-stripes). Add a blank comment line between these paragraphs. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:47: ZebraBlockHeap(); Remove this constructor. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:50: // @param heap_size The (initial) amount of memory reserved by the heap. Remove "(initial)". https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:92: EXPECT_TRUE(IsAligned(block.header, kShadowRatio)); This test is redundant, as the 'header' and the 'block' have the same pointer. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:93: EXPECT_TRUE(IsAligned(block.block, kShadowRatio)); The block should be kPageSize aligned. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:94: EXPECT_TRUE(IsAligned(block.block + block.block_size, kShadowRatio)); This test is redundant, as kPageSize > kShadowRatio by definition. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:199: EXPECT_LE(kPageSize, static_cast<size_t>(buffers[i] - buffers[i - 1])); They should be *two* page-sizes apart, no? https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:274: // TODO(peterssen): Test unsuccessfull allocations. Complete this TODO?
Sign in to reply to this message.
PTAL. All issues fixed, improved tests. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:56: if (::VirtualFree(heap_address_, 0, MEM_RELEASE) == FALSE) On 2014/07/29 20:07:51, chrisha wrote: > CHECK_EQ(TRUE, ::VirtualFree ... ) should suffice. > > There's no real reason for this to fail where a log message would be meaningful. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:79: uint8* alloc = page_address + kPageSize - bytes; On 2014/07/29 20:07:51, chrisha wrote: > All allocations should be kShadowRatio aligned: > > uint8* alloc = page_address + kPageSize - common::AlignUp(bytes, kShadowRatio); > > Also adjust your unittests to reflect this fact. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:142: // There should be less than kShadowsRatio bytes between the body end On 2014/07/29 20:07:51, chrisha wrote: > kShadowRatio* Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:149: if (right_zone_size < kPageSize) On 2014/07/29 20:07:51, chrisha wrote: > I'd do this check *before* the one above (otherwise right_zone_size - kPageSize > could be negative, and will map to a large unsigned number). The code is correct > as is, but is more intuitive if you swap the order. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.h (right): https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:40: // It divides the memory pages into "even" and "odd" types (like zebra-stripes). On 2014/07/29 20:07:52, chrisha wrote: > Add a blank comment line between these paragraphs. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:47: ZebraBlockHeap(); On 2014/07/29 20:07:52, chrisha wrote: > Remove this constructor. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.h:50: // @param heap_size The (initial) amount of memory reserved by the heap. On 2014/07/29 20:07:52, chrisha wrote: > Remove "(initial)". Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:92: EXPECT_TRUE(IsAligned(block.header, kShadowRatio)); On 2014/07/29 20:07:52, chrisha wrote: > This test is redundant, as the 'header' and the 'block' have the same pointer. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:93: EXPECT_TRUE(IsAligned(block.block, kShadowRatio)); On 2014/07/29 20:07:52, chrisha wrote: > The block should be kPageSize aligned. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:94: EXPECT_TRUE(IsAligned(block.block + block.block_size, kShadowRatio)); On 2014/07/29 20:07:52, chrisha wrote: > This test is redundant, as kPageSize > kShadowRatio by definition. Done. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:199: EXPECT_LE(kPageSize, static_cast<size_t>(buffers[i] - buffers[i - 1])); On 2014/07/29 20:07:52, chrisha wrote: > They should be *two* page-sizes apart, no? Suppose there is a normal allocation of size 0 (all allocations are pushed to end of the page), followed by an allocation of size kPageSize (a block), the difference will be exactly kPageSize. https://codereview.appspot.com/115280044/diff/200001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:274: // TODO(peterssen): Test unsuccessfull allocations. On 2014/07/29 20:07:52, chrisha wrote: > Complete this TODO? It depends mostly on PlanBlockLayout, sometimes is possible to allocate the block but the layout would be incorrect (e.g. the body will include both pages, or the distance to the end of the page is >= kShadowRatio). I added some checks to unsuccessful allocations, and the test actually cover a lot of edge cases.
Sign in to reply to this message.
A few more comments. Don't worry about the number of comments, it's your first CL so it's normal that it takes time for you to address all our picky comments :) https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:56: CHECK_NE(FALSE, ::VirtualFree(heap_address_, 0, MEM_RELEASE)); you can also do: if (::VirtualFree(heap_address_, 0, MEM_RELEASE)) != TRUE) { LOG(ERROR) << "Unable to free the memory used by the Zebra Heap."; } https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:132: kPageSize, You can align everything to after the parenthesis: BlockPlanLayout(kPageSize, kShadowRatio, size, min_left_redzone_size, ... https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:142: size_t right_zone_size = layout->trailer_size + layout->trailer_padding_size; s/zone/redzone/ https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:38: kInitialHeapSize = 8 * (1 << 20), Use a const value rather than an enum. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:43: bool IsFull() { Add a blank line between those 2 functions and comment the second one. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:58: void* alloc = h.AllocateBlock(0, 0, 0, &layout); Feel free to add a new test just for this. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:165: BlockLayout layout1 = {}, layout2 = {}; Don't declare 2 variables on the same line: BlockLayout layout1 = {}; BlockLayout layout2 = {}; https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:272: 2014, 2047, 2048, 2049, 3000, 4096, 4097, 7000, 12345, -1 }; Indent + 2. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:272: 2014, 2047, 2048, 2049, 3000, 4096, 4097, 7000, 12345, -1 }; I'd split this in 2 list, kValidSizes and kInvalidSizes. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:272: 2014, 2047, 2048, 2049, 3000, 4096, 4097, 7000, 12345, -1 }; You don't need the -1, use arraysize. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:300: layout.header_padding_size + layout.body_size; Indent + 2. Always indent at at least + 4 if it's the continuation of the previous line. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:304: // Even page overflow. indent + 2.
Sign in to reply to this message.
https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:56: CHECK_NE(FALSE, ::VirtualFree(heap_address_, 0, MEM_RELEASE)); On 2014/07/29 22:23:42, Sébastien Marchand wrote: > you can also do: > if (::VirtualFree(heap_address_, 0, MEM_RELEASE)) != TRUE) { > LOG(ERROR) << "Unable to free the memory used by the Zebra Heap."; > } I just suggested the opposite in my last review. Logging is for runtime errors, which are things that can plausibly (and out of our control) go wrong while the program is executing. VirtualAlloc and VirtualFree have a contract and are documented to succeed as long as that contract is obeyed; if it fails, this is indication of something gone seriously wrong or a programmer error. A CHECK makes more sense here. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:38: kInitialHeapSize = 8 * (1 << 20), On 2014/07/29 22:23:42, Sébastien Marchand wrote: > Use a const value rather than an enum. The const value had problems with static variable order of initialization... but if you can get that to work it's preferable.
Sign in to reply to this message.
https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:56: CHECK_NE(FALSE, ::VirtualFree(heap_address_, 0, MEM_RELEASE)); On 2014/07/29 22:49:09, chrisha wrote: > On 2014/07/29 22:23:42, Sébastien Marchand wrote: > > you can also do: > > if (::VirtualFree(heap_address_, 0, MEM_RELEASE)) != TRUE) { > > LOG(ERROR) << "Unable to free the memory used by the Zebra Heap."; > > } > > I just suggested the opposite in my last review. > > Logging is for runtime errors, which are things that can plausibly (and out of > our control) go wrong while the program is executing. VirtualAlloc and > VirtualFree have a contract and are documented to succeed as long as that > contract is obeyed; if it fails, this is indication of something gone seriously > wrong or a programmer error. A CHECK makes more sense here. Oops, sorry for that. This is a good point, I just wanted to note the fact that we usually log on failure (instead of putting the calls in a CHECK). But in this case I agree with you.
Sign in to reply to this message.
PTAL. Small fixes. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:56: CHECK_NE(FALSE, ::VirtualFree(heap_address_, 0, MEM_RELEASE)); I'll use a CHECK too instead of DCHECK on the initial allocation. On 2014/07/29 22:49:09, chrisha wrote: > On 2014/07/29 22:23:42, Sébastien Marchand wrote: > > you can also do: > > if (::VirtualFree(heap_address_, 0, MEM_RELEASE)) != TRUE) { > > LOG(ERROR) << "Unable to free the memory used by the Zebra Heap."; > > } > > I just suggested the opposite in my last review. > > Logging is for runtime errors, which are things that can plausibly (and out of > our control) go wrong while the program is executing. VirtualAlloc and > VirtualFree have a contract and are documented to succeed as long as that > contract is obeyed; if it fails, this is indication of something gone seriously > wrong or a programmer error. A CHECK makes more sense here. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:132: kPageSize, On 2014/07/29 22:23:42, Sébastien Marchand wrote: > You can align everything to after the parenthesis: > BlockPlanLayout(kPageSize, > kShadowRatio, > size, > min_left_redzone_size, > ... Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:142: size_t right_zone_size = layout->trailer_size + layout->trailer_padding_size; On 2014/07/29 22:23:42, Sébastien Marchand wrote: > s/zone/redzone/ Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:38: kInitialHeapSize = 8 * (1 << 20), Declaring it as 'static const' solves the problem. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:43: bool IsFull() { On 2014/07/29 22:23:43, Sébastien Marchand wrote: > Add a blank line between those 2 functions and comment the second one. Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:58: void* alloc = h.AllocateBlock(0, 0, 0, &layout); On 2014/07/29 22:23:43, Sébastien Marchand wrote: > Feel free to add a new test just for this. Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:165: BlockLayout layout1 = {}, layout2 = {}; On 2014/07/29 22:23:43, Sébastien Marchand wrote: > Don't declare 2 variables on the same line: > BlockLayout layout1 = {}; > BlockLayout layout2 = {}; Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:272: 2014, 2047, 2048, 2049, 3000, 4096, 4097, 7000, 12345, -1 }; The idea is to test the combination of valid and invalid sizes. On 2014/07/29 22:23:43, Sébastien Marchand wrote: > I'd split this in 2 list, kValidSizes and kInvalidSizes. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:272: 2014, 2047, 2048, 2049, 3000, 4096, 4097, 7000, 12345, -1 }; On 2014/07/29 22:23:42, Sébastien Marchand wrote: > You don't need the -1, use arraysize. Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:272: 2014, 2047, 2048, 2049, 3000, 4096, 4097, 7000, 12345, -1 }; On 2014/07/29 22:23:43, Sébastien Marchand wrote: > Indent + 2. Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:300: layout.header_padding_size + layout.body_size; On 2014/07/29 22:23:43, Sébastien Marchand wrote: > Indent + 2. Always indent at at least + 4 if it's the continuation of the > previous line. Done. https://codereview.appspot.com/115280044/diff/240001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:304: // Even page overflow. On 2014/07/29 22:23:43, Sébastien Marchand wrote: > indent + 2. Done.
Sign in to reply to this message.
Almost done ! A few more comments, Chris can you take a look at my comment in ZebraBlockHeapTest.AllocateBlockCornerCases ? https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:22: It's generally a good idea to the order of definition of functions in your .cc file the same as that of the order of declarations in the matching .h file, can you reorder those functions to they'll match the declaration order of the header file please ? (I'm not the best person to make this comment as I'm often making this mistake, but I'm working on it !) https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:40: TestZebraBlockHeap() : ZebraBlockHeap(kInitialHeapSize) {} s/{}/{ }/ https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:52: Remove one of those blank lines. https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:282: const size_t kSizes[] = {0, 1, 3, 7, 9, 17, 1024, 1023, 1025, 1235, I'm not a big fan of this, it's often better to clearly indicates the value that should fail and the one that should not... It's why I wanted to split this in 2 lists but this might not be easy here... Maybe you could define a list of tuples { body_size, header_size, trailer_size, should_fail } ? (should_fail is a size_t: 0 = false, 1 = true, it could even indicate the cause of the failure) const size_t kTestCases = { { 0, 0, 0, 0, 1 }, .... } Chris, what do you think ?
Sign in to reply to this message.
PTAL. @Chis: About the AllocateBlockCornerCases test. I don't have any guarantees on the layout (PlanBlockLayout function). I assumed 3 possible failure causes, that should cover everything, but it depends on how PlanBlockLayout arranges the layout for some odd inputs. https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap.cc (right): https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap.cc:22: On 2014/07/30 15:43:24, Sébastien Marchand wrote: > It's generally a good idea to the order of definition of functions in your .cc > file the same as that of the order of declarations in the matching .h file, can > you reorder those functions to they'll match the declaration order of the header > file please ? (I'm not the best person to make this comment as I'm often making > this mistake, but I'm working on it !) Done. https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:40: TestZebraBlockHeap() : ZebraBlockHeap(kInitialHeapSize) {} On 2014/07/30 15:43:24, Sébastien Marchand wrote: > s/{}/{ }/ Done. https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:52: On 2014/07/30 15:43:24, Sébastien Marchand wrote: > Remove one of those blank lines. Done.
Sign in to reply to this message.
https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:282: const size_t kSizes[] = {0, 1, 3, 7, 9, 17, 1024, 1023, 1025, 1235, MkOn 2014/07/30 15:43:24, Sébastien Marchand wrote: > I'm not a big fan of this, it's often better to clearly indicates the value that > should fail and the one that should not... It's why I wanted to split this in 2 > lists but this might not be easy here... > > Maybe you could define a list of tuples { body_size, header_size, trailer_size, > should_fail } ? (should_fail is a size_t: 0 = false, 1 = true, it could even > indicate the cause of the failure) > > const size_t kTestCases = { > { 0, 0, 0, 0, 1 }, > .... > } > > Chris, what do you think ? Yeah, I'd encode this as an array of pairs: size, expected_success. Or have two arrays: kValidCornerCases, kInvalidCornerCases. As to BlockPlanLayout, there will be a single size (kPageSize - sizeof(BlockHeader)) above which it will fail.
Sign in to reply to this message.
On 2014/07/30 16:40:03, chrisha wrote: > https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... > File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): > > https://codereview.appspot.com/115280044/diff/260001/syzygy/agent/asan/heaps/... > syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:282: const size_t kSizes[] > = {0, 1, 3, 7, 9, 17, 1024, 1023, 1025, 1235, > MkOn 2014/07/30 15:43:24, Sébastien Marchand wrote: > > I'm not a big fan of this, it's often better to clearly indicates the value > that > > should fail and the one that should not... It's why I wanted to split this in > 2 > > lists but this might not be easy here... > > > > Maybe you could define a list of tuples { body_size, header_size, > trailer_size, > > should_fail } ? (should_fail is a size_t: 0 = false, 1 = true, it could even > > indicate the cause of the failure) > > > > const size_t kTestCases = { > > { 0, 0, 0, 0, 1 }, > > .... > > } > > > > Chris, what do you think ? > > Yeah, I'd encode this as an array of pairs: size, expected_success. Or have two > arrays: kValidCornerCases, kInvalidCornerCases. > > As to BlockPlanLayout, there will be a single size (kPageSize - > sizeof(BlockHeader)) above which it will fail. I still prefer my way to a hard-coded 'expected' array. My argument is that the 'expected' values won't work in certain unlikely circumstances. (e.g. a different kPageSize, different kPageRatio, changes in PlanBlockLayout...), and the way it is now tests lots of cases ensuring 3 simple post-conditions: 1) The header, left_padding and body fits entirely inside the first page. 2) The trailer fits entirely inside the second page. 3) The body is as close as possible (distance < kShadowRatio) to the end of the first page. I could agree to split the kSizes array in 3, one for each size. I've added some additional sizes, now it tests more than 85000 cases, and correctly identify all the unsuccessful allocations, which are near 43000.
Sign in to reply to this message.
Okay, that seems like a reasonable compromise for the unittest. I'm happy with this as it is. lgtm! https://codereview.appspot.com/115280044/diff/320001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/320001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:336: // Check the cause of the unsuccessfull allocation. unsuccessful*
Sign in to reply to this message.
lgtm. https://codereview.appspot.com/115280044/diff/320001/syzygy/agent/asan/heaps/... File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): https://codereview.appspot.com/115280044/diff/320001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:285: const size_t kSizes[] = {0, 1, 2, 3, 4, 5, 7, 8, 9, 15, 17, 1023, 1024, 1025, Add a space between { and the first value (i.e. "= { 0"). Same thing for the closing bracket ("kPageSize - block_trailer_size + 1 };") https://codereview.appspot.com/115280044/diff/320001/syzygy/agent/asan/heaps/... syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:286: 1235, 1365, 2014, 2047, 2048, 2049, 3000, 4095, 4096, 4097, 7000, 12345, nit: you don't really need the values 4095->4097 and some other ones as you're duplicating them below.
Sign in to reply to this message.
On 2014/07/30 19:25:36, chrisha wrote: > Okay, that seems like a reasonable compromise for the unittest. > > I'm happy with this as it is. > > lgtm! > > https://codereview.appspot.com/115280044/diff/320001/syzygy/agent/asan/heaps/... > File syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc (right): > > https://codereview.appspot.com/115280044/diff/320001/syzygy/agent/asan/heaps/... > syzygy/agent/asan/heaps/zebra_block_heap_unittest.cc:336: // Check the cause of > the unsuccessfull allocation. > unsuccessful* Typo fixed, my first LGTM ever, yeah!!!
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
