i#2157 re-attach: Always free the heap on unix #2630
Conversation
On unix it should be safe to free the heap even if there blocks still being used since we are clearing out all other memory references anyway. I also included a fix to vmm_heap_unit_init which accidentally left vmh->alloc_start uninitialized in the branch related to reserving OS memory at a preferred location.
core/heap.c
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've updated the code and verified that it doesn't crash when running code under drrun nor when doing detach/reattach. ptal
When doing_detach is false, the current stack frame is actually in the heap, so unmapping causes a segfault. Issue #2157
core/heap.c
Outdated
| * doing a detach we can be sure our stack is not actually in the heap. | ||
| */ | ||
| if (doing_detach) { | ||
| byte *sp; |
There was a problem hiding this comment.
style: indent should be 4 spaces
There was a problem hiding this comment.
Done (and fixed my editor)
core/heap.c
Outdated
| */ | ||
| if (doing_detach) { | ||
| byte *sp; | ||
| GET_STACK_PTR(sp); |
There was a problem hiding this comment.
Put this all inside a DODEBUG as it's only for the assert
Fix indents and wrapping code in DODEBUG where appropriate.
core/heap.c
Outdated
| byte *sp; | ||
| GET_STACK_PTR(sp); | ||
| ASSERT(!(sp >= vmh->start_addr && sp < vmh->end_addr)); | ||
| }) |
There was a problem hiding this comment.
bug: missing ; which is why the Travis builds all failed
There was a problem hiding this comment.
Sorry about that. I could have sworn that I checked whether DODEBUG needed a semi but I must have been not paying attention at all... all the other uses have one.
On UNIX we're on a permanent non-vmm stack at detach, so we can free the full vmm region. I also included a fix to vmm_heap_unit_init which accidentally left vmh->alloc_start uninitialized in the branch related to reserving OS memory at a preferred location. Issue: #2157
On unix it should be safe to free the heap even if there blocks still
being used since we are clearing out all other memory references anyway.
I also included a fix to vmm_heap_unit_init which accidentally left
vmh->alloc_start uninitialized in the branch related to reserving OS
memory at a preferred location.
Issue: #2157