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.

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

Add valgrind memcheck hooks #27

Merged
merged 11 commits into from
Feb 24, 2022
11 changes: 11 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ COV_TGT := $(BUILD_DIR)/coverage.xml

CC = $(MAKE_CC)
CFLAGS += -g -I$(INC_DIR) -O -Wall -pedantic -std=c99

# Check whether valgrind is available.
ifeq ($(shell command -v valgrind),)
# If not, we probably shouldn't try to include <valgrind/valgrind.h>,
# which we use to instrument our memory allocator.
#
# NOTE: this is just a heuristic that may not be very reliable.
# We should probably implement better dependency management.
CFLAGS += -DNVALGRIND
endif

TEST_CFLAGS = $(CFLAGS) -g -DSSM_DEBUG --coverage

LD = $(MAKE_LD)
Expand Down
16 changes: 9 additions & 7 deletions examples/ssm-examples.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,15 @@ void ssm_throw(enum ssm_error reason, const char *file, int line,
}

#define MAX_PAGES 2048
static void *pages[MAX_PAGES];
static uint8_t *pages[SSM_MEM_PAGE_SIZE][MAX_PAGES];
Copy link
Contributor Author

@j-hui j-hui Feb 13, 2022

Choose a reason for hiding this comment

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

Mem pool pages are now allocated out of a static megapool now, so we don't have to worry about freeing them afterwards.

static size_t allocated_pages = 0;

static void *alloc_page(void) {
if (allocated_pages >= MAX_PAGES) {
SSM_THROW(SSM_EXHAUSTED_MEMORY);
exit(3);
}
void *m = pages[allocated_pages++] = malloc(SSM_MEM_PAGE_SIZE);
void *m = pages[allocated_pages++];
memset(m, 0, SSM_MEM_PAGE_SIZE);
return m;
}
Expand All @@ -66,6 +66,8 @@ static void *alloc_mem(size_t size) { return malloc(size); }

static void free_mem(void *mem, size_t size) { free(mem); }

static void free_page(void *mem) { allocated_pages--; }

static void print_help(char *prog) {
printf("Usage: %s [OPTION]... [--] [ARG]...\n", prog);
printf("\n");
Expand All @@ -84,8 +86,6 @@ static void print_help(char *prog) {
char **ssm_init_args;

int main(int argc, char *argv[]) {
ssm_mem_init(alloc_page, alloc_mem, free_mem);

size_t stop_at_s = 20;
char *prog = *argv;

Expand Down Expand Up @@ -118,8 +118,11 @@ int main(int argc, char *argv[]) {
}

ssm_init_args = argv;

ssm_time_t stop_at = stop_at_s == 0 ? SSM_NEVER : stop_at_s * SSM_SECOND;
printf("%s: simulating up to %lu seconds\n", prog,
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 added this print statement here to spook printf into "leaking" memory earlier; under the hood, it actually keeps around a 1024B buffer that it doesn't free until the very end, so we make sure it gets counted by the leak checkpoint take during ssm_mem_init().

Choose a reason for hiding this comment

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

Are you saying that printf will allocate a buffer and not free it until the program terminates? Since you are calling printf here, it will allocate a buffer, and then before it has a chance to free its buffer, you will check for memory leaks, which will include printf's buffer. Is this correct?

(unsigned long)stop_at / SSM_SECOND);

ssm_mem_init(alloc_page, alloc_mem, free_mem);

ssm_program_init();

Expand All @@ -133,8 +136,7 @@ int main(int argc, char *argv[]) {

ssm_program_exit();

for (size_t p = 0; p < allocated_pages; p++)
free(pages[p]);
ssm_mem_destroy(free_page);

Choose a reason for hiding this comment

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

Right now ssm_mem_destroy doesn't free any memory, right? Is this okay because the pools of pages are statically allocated/don't need to be freed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ssm-runtime's allocator doesn't actually care how the pools are allocated (they could also be dynamically allocated).

ssm_mem_destroy is supposed to handle freeing these pages, but I haven't implemented that just yet.


return 0;
}
Expand Down
30 changes: 27 additions & 3 deletions include/ssm-internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,16 +154,36 @@ enum ssm_kind {
* alloc_mem_handler. These handlers may also assume they will not be invoked
* to request memory ranges of less than #SSM_MEM_POOL_MAX bytes.
*
* If the allocator is compiled with valgrind support (i.e., without defining
* @a NVALGRIND), it will perform a leak-check summary, to checkpoint how much
* memory has already been allocated.
*
* @platformonly
*
* @param alloc_page_handler allocates pages.
* @param alloc_mem_handler allocates arbitrarily large.
* @param free_mem_handler frees pages allocated with @a alloc_mem_handler.
* @param alloc_page_handler allocates pages.
* @param alloc_mem_handler allocates arbitrarily large.
* @param free_mem_handler frees memory allocated with @a alloc_mem_handler.
*/
void ssm_mem_init(void *(*alloc_page_handler)(void),
void *(*alloc_mem_handler)(size_t),
void (*free_mem_handler)(void *, size_t));

/** @brief Tears down the underlying allocator system.
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 added this endpoint to give us the opportunity to deallocate pages in the future.

We might not be able to do this very efficiently, but that's ok. In my view, this exists primarily for testing purposes.

*
* If the allocator is compiled with valgrind support (i.e., without defining
* @a NVALGRIND), it will perform a full leak-check summary, to report how much
* memory has been leaked since ssm_mem_init().
*
* @TODO this doesn't actually call @a free_page_handler yet. It still needs to
* be implemented, perhaps with the help of a superblock header to keep
* track of all pages allocated for each mempool
*
* @platformonly
*
* @param free_page_handler frees pages allocated with @a alloc_page_handler.
*/
void ssm_mem_destroy(void (*free_page_handler)(void *));

#ifndef SSM_MEM_POOL_MIN
/** @brief Block size of the smallest memory pool.
*
Expand Down Expand Up @@ -232,4 +252,8 @@ void ssm_mem_init(void *(*alloc_page_handler)(void),

/** @} */

#ifndef NVALGRIND
#include <valgrind/memcheck.h>
EmilySillars marked this conversation as resolved.
Show resolved Hide resolved
#endif

#endif /* _SSM_SCHED_H */
2 changes: 1 addition & 1 deletion runtests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ else
fi

make clean
CFLAGS=-DSSM_DEBUG_NO_ALLOC make exes tests
make exes tests
j-hui marked this conversation as resolved.
Show resolved Hide resolved

if command -v valgrind >/dev/null ; then
rm -f build/examples.vg-out
Expand Down
86 changes: 68 additions & 18 deletions src/ssm-mem.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ static inline size_t find_pool_size(size_t size) {
return SSM_MEM_POOL_COUNT;
}

static inline block_t *find_next_block(block_t *block, size_t pool_size) {
if (block->free_list_next == UNINITIALIZED_FREE_BLOCK)
return block + pool_size / sizeof(block_t);
else
return block->free_list_next;
}

/** @brief Allocate a new block for a memory pool.
*
* Calls alloc_page() to allocate a new zero-initialized page for the memory
Expand All @@ -91,11 +98,18 @@ static inline size_t find_pool_size(size_t size) {
static inline void alloc_pool(size_t p) {
block_t *new_page = alloc_page();
SSM_ASSERT(END_OF_FREELIST < new_page);

struct mem_pool *pool = &mem_pools[p];
size_t last_block = BLOCKS_PER_PAGE - SSM_MEM_POOL_SIZE(p) / sizeof(block_t);
new_page[last_block].free_list_next = pool->free_list_head;
pool->free_list_head = new_page;

#ifndef NVALGRIND
// Mark as NOACCESS; nothing should touch this memory until it is allocated.
VALGRIND_MAKE_MEM_NOACCESS(new_page, SSM_MEM_PAGE_SIZE);

// Label this page with a human-readable name, for memory error reporting.
VALGRIND_CREATE_BLOCK(new_page, SSM_MEM_PAGE_SIZE, "SSM memory pool page");
#endif
}

void ssm_mem_init(void *(*alloc_page_handler)(void),
Expand All @@ -105,8 +119,34 @@ void ssm_mem_init(void *(*alloc_page_handler)(void),
alloc_mem = alloc_mem_handler;
free_mem = free_mem_handler;

for (size_t p = 0; p < SSM_MEM_POOL_COUNT; p++)
for (size_t p = 0; p < SSM_MEM_POOL_COUNT; p++) {
mem_pools[p].free_list_head = END_OF_FREELIST;
}

#ifndef NVALGRIND
// Ask Valgrind to checkpoint the amount of memory allocated so far.
VALGRIND_PRINTF("Performing leak check at memory initialization.\n");
VALGRIND_PRINTF("(Checkpoints how much memory is allocated at init time.)\n");
VALGRIND_PRINTF("\n");
VALGRIND_DO_QUICK_LEAK_CHECK;
#endif
}

void ssm_mem_destroy(void (*free_page_handler)(void *)) {
// TODO: call free_page_handler on all superblocks in each mem pool.
// for (size_t p = 0; p < SSM_MEM_POOL_COUNT; p++) {
// if (mem_pools[p].free_list_head != END_OF_FREELIST) {
// }
// }

#ifndef NVALGRIND
// Report how much memory has been leaked since the checkpoint in
// ssm_mem_init().
VALGRIND_PRINTF("About to destroy SSM allocator. Performing leak check.\n");
VALGRIND_PRINTF("(Note that leaks from malloc() may be false positives.)\n");
VALGRIND_PRINTF("\n");
VALGRIND_DO_ADDED_LEAK_CHECK;
#endif
}

void ssm_mem_prealloc(size_t size, size_t num_pages) {
Expand All @@ -118,9 +158,6 @@ void ssm_mem_prealloc(size_t size, size_t num_pages) {
}

void *ssm_mem_alloc(size_t size) {
#ifdef SSM_DEBUG_NO_ALLOC
return alloc_mem(size);
#else
size_t p = find_pool_size(size);
if (p >= SSM_MEM_POOL_COUNT)
return alloc_mem(size);
Expand All @@ -130,30 +167,43 @@ void *ssm_mem_alloc(size_t size) {
if (pool->free_list_head == END_OF_FREELIST)
alloc_pool(p);

void *buf = pool->free_list_head->block_buf;
void *m = pool->free_list_head->block_buf;

if (pool->free_list_head->free_list_next == UNINITIALIZED_FREE_BLOCK)
pool->free_list_head += SSM_MEM_POOL_SIZE(p) / sizeof(block_t);
else
pool->free_list_head = pool->free_list_head->free_list_next;
#ifndef NVALGRIND
// Tell Valgrind that we have allocated m as a chunk of pool.
//
// The memory range [m..m+size] is now considered DEFINED.
VALGRIND_MALLOCLIKE_BLOCK(m, size, 0, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how we tell Valgrind that we have allocated something.

This appears to also work even if the underlying page is malloc'ed (at least leak-checking still seems to work). Valgrind seems to get confused over memory permissions, but we manage that manually (using VALGRIND_MAKE_MEM_ macros).

#endif

pool->free_list_head =
find_next_block(pool->free_list_head, SSM_MEM_POOL_SIZE(p));

return buf;
#ifndef NVALGRIND
// Make the memory range [m..m+size] undefined, because the caller should
// not rely on allocated chunks being defined.
VALGRIND_MAKE_MEM_UNDEFINED(m, size);
#endif

return m;
}

void ssm_mem_free(void *m, size_t size) {
#ifdef SSM_DEBUG_NO_ALLOC
free_mem(m, size);
#else
size_t pool = find_pool_size(size);
if (pool >= SSM_MEM_POOL_COUNT) {
size_t p = find_pool_size(size);
if (p >= SSM_MEM_POOL_COUNT) {
free_mem(m, size);
return;
}

struct mem_pool *pool = &mem_pools[p];

block_t *new_head = m;
new_head->free_list_next = mem_pools[pool].free_list_head;
mem_pools[pool].free_list_head = new_head;
new_head->free_list_next = pool->free_list_head;
pool->free_list_head = new_head;

#ifndef NVALGRIND
// Tell Valgrind that we have freed m.
VALGRIND_FREELIKE_BLOCK(m, 0);
#endif
}

Expand Down
Loading