Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#2157 re-attach: Always free the heap on unix #2630

Merged
merged 5 commits into from
Sep 25, 2017
Merged

i#2157 re-attach: Always free the heap on unix #2630

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a08fb3
i#2157 re-attach: Always free the heap on unix
Carrotman42 Sep 8, 2017
2e8ab01
i#2157 re-attach: Only free the heap during detach
Carrotman42 Sep 11, 2017
aa64ab3
i#2157 re-attach: Fixes from code review.
Carrotman42 Sep 12, 2017
e61e900
Add missing semicolon
Carrotman42 Sep 14, 2017
c336530
Merge branch 'master' into i2157-fix-heap-leak
Carrotman42 Sep 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions core/heap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,8 +796,9 @@ vmm_heap_unit_init(vm_heap_t *vmh, size_t size)
error_code = HEAP_ERROR_NOT_AT_PREFERRED;
} else {
#endif
vmh->start_addr = os_heap_reserve((void*)preferred, size, &error_code,
true/*+x*/);
vmh->alloc_start = os_heap_reserve((void*)preferred, size, &error_code,
true/*+x*/);
vmh->start_addr = vmh->alloc_start;
LOG(GLOBAL, LOG_HEAP, 1,
"vmm_heap_unit_init preferred="PFX" got start_addr="PFX"\n",
preferred, vmh->start_addr);
Expand Down Expand Up @@ -892,10 +893,15 @@ vmm_heap_unit_exit(vm_heap_t *vmh)
ASSERT(vmh->num_blocks * DYNAMO_OPTION(vmm_block_size) ==
(ptr_uint_t)(vmh->end_addr - vmh->start_addr));

#ifdef UNIX
bool free_heap = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my offline comment that it should be fine to free the whole vmm only applies to the start/stop API detach scenario where we're on the app stack. For a regular process exit we're on the dstack, which is inside the vmm, and then we swap to the initstack, also inside the vmm: so it seems that this would crash there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code and verified that it doesn't crash when running code under drrun nor when doing detach/reattach. ptal

#else // Particularly WINDOWS
/* In case there are no tombstones we can just free the unit and
* that is what we'll do, otherwise it will stay up forever.
*/
if (vmh->num_free_blocks == vmh->num_blocks) {
bool free_heap = vmh->num_free_blocks == vmh->num_blocks;
#endif
if (free_heap) {
heap_error_code_t error_code;
os_heap_free(vmh->alloc_start, vmh->alloc_size, &error_code);
ASSERT(error_code == HEAP_ERROR_SUCCESS);
Expand Down