From daab651c5ca0d7198f6db2fa0abe2399fc08c69e Mon Sep 17 00:00:00 2001 From: Damien George Date: Fri, 25 Apr 2014 23:37:55 +0100 Subject: [PATCH] py, gc: Zero out newly allocated blocks in the GC. Also add some more debugging output to gc_dump_alloc_table(). Now that newly allocated heap is always zero'd, maybe we just make this a policy for the uPy API to keep it simple (ie any new implementation of memory allocation must zero all allocations). This follows the D language philosophy. Before this patch, a previously used memory block which had pointers in it may still retain those pointers if the new user of that block does not actually use the entire block. Eg, if I want 5 blocks worth of heap, I actually get 8 (round up to nearest 4). Then I never use the last 3, so they keep their old values, which may be pointers pointing to the heap, hence preventing GC. In rare (or maybe not that rare) cases, this leads to long, unintentional "linked lists" within the GC'd heap, filling it up completely. It's pretty rare, because you have to reuse exactly that memory which is part of this "linked list", and reuse it in just the right way. This should fix issue #522, and might have something to do with issue #510. --- py/gc.c | 29 ++++++++++++++++++++++++++--- py/malloc.c | 4 ++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/py/gc.c b/py/gc.c index 9a5f9d89bf..d0ba26ddd3 100644 --- a/py/gc.c +++ b/py/gc.c @@ -366,10 +366,17 @@ found: // get pointer to first block void *ret_ptr = (void*)(gc_pool_start + start_block * WORDS_PER_BLOCK); + // zero out the newly allocated blocks + // This is needed because the blocks may have previously held pointers + // to the heap and will not be set to something else if the caller + // doesn't actually use the entire block. As such they will continue + // to point to the heap and may prevent other blocks from being reclaimed. + memset(ret_ptr, 0, (end_block - start_block + 1) * BYTES_PER_BLOCK); + #if MICROPY_ENABLE_FINALISER if (has_finaliser) { - // clear type pointer in case it is never set - ((mp_obj_base_t*)ret_ptr)->type = MP_OBJ_NULL; + // clear type pointer in case it is never set (now done above in memset) + //((mp_obj_base_t*)ret_ptr)->type = MP_OBJ_NULL; // set mp_obj flag only if it has a finaliser FTB_SET(start_block); } @@ -523,6 +530,10 @@ void *gc_realloc(void *ptr_in, machine_uint_t n_bytes) { assert(ATB_GET_KIND(bl) == AT_FREE); ATB_FREE_TO_TAIL(bl); } + + // zero out the newly allocated blocks (see comment above in gc_alloc) + memset(ptr_in + n_blocks * BYTES_PER_BLOCK, 0, (new_blocks - n_blocks) * BYTES_PER_BLOCK); + return ptr_in; } @@ -556,7 +567,7 @@ void gc_dump_info() { } void gc_dump_alloc_table(void) { - printf("GC memory layout:"); + printf("GC memory layout; from %p:", gc_pool_start); for (machine_uint_t bl = 0; bl < gc_alloc_table_byte_len * BLOCKS_PER_ATB; bl++) { if (bl % 64 == 0) { printf("\n%04x: ", (uint)bl); @@ -565,6 +576,18 @@ void gc_dump_alloc_table(void) { switch (ATB_GET_KIND(bl)) { case AT_FREE: c = '.'; break; case AT_HEAD: c = 'h'; break; + /* this prints the uPy object type of the head block + case AT_HEAD: { + machine_uint_t *ptr = gc_pool_start + bl * WORDS_PER_BLOCK; + if (*ptr == (machine_uint_t)&mp_type_tuple) { c = 'T'; } + else if (*ptr == (machine_uint_t)&mp_type_list) { c = 'L'; } + else if (*ptr == (machine_uint_t)&mp_type_dict) { c = 'D'; } + else if (*ptr == (machine_uint_t)&mp_type_float) { c = 'F'; } + else if (*ptr == (machine_uint_t)&mp_type_fun_bc) { c = 'B'; } + else { c = 'h'; } + break; + } + */ case AT_TAIL: c = 't'; break; case AT_MARK: c = 'm'; break; } diff --git a/py/malloc.c b/py/malloc.c index db2578d9ad..76c3fe6cad 100644 --- a/py/malloc.c +++ b/py/malloc.c @@ -88,9 +88,13 @@ void *m_malloc_with_finaliser(int num_bytes) { void *m_malloc0(int num_bytes) { void *ptr = m_malloc(num_bytes); +#if MICROPY_ENABLE_GC + // the GC already zeros out all memory +#else if (ptr != NULL) { memset(ptr, 0, num_bytes); } +#endif return ptr; }