From 91b5d273ab829802094fd47f8cd67aea424151b2 Mon Sep 17 00:00:00 2001 From: Jeremy L Thompson Date: Mon, 16 Sep 2024 11:47:25 -0600 Subject: [PATCH] memcheck - also tidy up the ctx impl --- .../memcheck/ceed-memcheck-qfunctioncontext.c | 175 ++++++++++++++---- backends/memcheck/ceed-memcheck-vector.c | 6 +- backends/memcheck/ceed-memcheck.h | 13 +- 3 files changed, 150 insertions(+), 44 deletions(-) diff --git a/backends/memcheck/ceed-memcheck-qfunctioncontext.c b/backends/memcheck/ceed-memcheck-qfunctioncontext.c index 4da0d0ee68..57afe981af 100644 --- a/backends/memcheck/ceed-memcheck-qfunctioncontext.c +++ b/backends/memcheck/ceed-memcheck-qfunctioncontext.c @@ -20,7 +20,7 @@ static int CeedQFunctionContextHasValidData_Memcheck(CeedQFunctionContext ctx, b CeedQFunctionContext_Memcheck *impl; CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); - *has_valid_data = impl->data; + *has_valid_data = !!impl->data_allocated; return CEED_ERROR_SUCCESS; } @@ -30,9 +30,10 @@ static int CeedQFunctionContextHasValidData_Memcheck(CeedQFunctionContext ctx, b static int CeedQFunctionContextHasBorrowedDataOfType_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type, bool *has_borrowed_data_of_type) { CeedQFunctionContext_Memcheck *impl; - CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend"); - *has_borrowed_data_of_type = impl->data_borrowed; + + CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); + *has_borrowed_data_of_type = !!impl->data_borrowed; return CEED_ERROR_SUCCESS; } @@ -43,35 +44,69 @@ static int CeedQFunctionContextSetData_Memcheck(CeedQFunctionContext ctx, CeedMe size_t ctx_size; CeedQFunctionContext_Memcheck *impl; + CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend"); + CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size)); - CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend"); - + // Clear previous owned data buffers + if (impl->data_allocated) { + memset(impl->data_allocated, -42, ctx_size); + VALGRIND_DISCARD(impl->allocated_block_id); + } CeedCallBackend(CeedFree(&impl->data_allocated)); + if (impl->data_owned) { + memset(impl->data_owned, -42, ctx_size); + VALGRIND_DISCARD(impl->owned_block_id); + } CeedCallBackend(CeedFree(&impl->data_owned)); + + // Clear borrowed block id, if present + if (impl->data_borrowed) VALGRIND_DISCARD(impl->borrowed_block_id); + + // Set internal pointers to external buffers switch (copy_mode) { case CEED_COPY_VALUES: - CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_owned)); + impl->data_owned = NULL; impl->data_borrowed = NULL; - impl->data = impl->data_owned; - memcpy(impl->data, data, ctx_size); break; case CEED_OWN_POINTER: - impl->data_owned = data; - impl->data_borrowed = NULL; - impl->data = data; + impl->data_owned = data; + impl->data_borrowed = NULL; + impl->owned_block_id = VALGRIND_CREATE_BLOCK(impl->data_owned, ctx_size, "Owned external data buffer"); break; case CEED_USE_POINTER: - impl->data_borrowed = data; - impl->data = data; + impl->data_owned = NULL; + impl->data_borrowed = data; + impl->owned_block_id = VALGRIND_CREATE_BLOCK(impl->data_borrowed, ctx_size, "Borrowed external data buffer"); } - // Copy data to check ctx_size bounds + + // Create internal data buffer CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_allocated)); - memcpy(impl->data_allocated, impl->data, ctx_size); - impl->data = impl->data_allocated; - VALGRIND_DISCARD(impl->mem_block_id); - impl->mem_block_id = VALGRIND_CREATE_BLOCK(impl->data, ctx_size, "'QFunction backend context data copy'"); + impl->allocated_block_id = VALGRIND_CREATE_BLOCK(impl->data_allocated, ctx_size, "'Allocated internal context data buffer"); + memcpy(impl->data_allocated, data, ctx_size); + return CEED_ERROR_SUCCESS; +} + +//------------------------------------------------------------------------------ +// Sync data +//------------------------------------------------------------------------------ +static int CeedQFunctionContextSyncData_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type) { + size_t ctx_size; + CeedQFunctionContext_Memcheck *impl; + + CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend"); + + CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); + CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size)); + + // Copy internal buffer back to owned or borrowed data buffer + if (impl->data_owned) { + memcpy(impl->data_owned, impl->data_allocated, ctx_size); + } + if (impl->data_borrowed) { + memcpy(impl->data_borrowed, impl->data_allocated, ctx_size); + } return CEED_ERROR_SUCCESS; } @@ -79,16 +114,27 @@ static int CeedQFunctionContextSetData_Memcheck(CeedQFunctionContext ctx, CeedMe // QFunctionContext Take Data //------------------------------------------------------------------------------ static int CeedQFunctionContextTakeData_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type, void *data) { + size_t ctx_size; CeedQFunctionContext_Memcheck *impl; + CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only provide HOST memory for this backend"); + CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); + CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size)); - CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only provide HOST memory for this backend"); + // Synchronize memory + CeedCallBackend(CeedQFunctionContextSyncData_Memcheck(ctx, CEED_MEM_HOST)); + // Return borrowed buffer *(void **)data = impl->data_borrowed; impl->data_borrowed = NULL; - impl->data = NULL; - VALGRIND_DISCARD(impl->mem_block_id); + VALGRIND_DISCARD(impl->borrowed_block_id); + + // De-allocate internal memory + if (impl->data_allocated) { + memset(impl->data_allocated, -42, ctx_size); + VALGRIND_DISCARD(impl->allocated_block_id); + } CeedCallBackend(CeedFree(&impl->data_allocated)); return CEED_ERROR_SUCCESS; } @@ -97,13 +143,19 @@ static int CeedQFunctionContextTakeData_Memcheck(CeedQFunctionContext ctx, CeedM // QFunctionContext Get Data //------------------------------------------------------------------------------ static int CeedQFunctionContextGetData_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type, void *data) { + size_t ctx_size; CeedQFunctionContext_Memcheck *impl; - CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); + CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend"); - CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only provide HOST memory for this backend"); + CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); + CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size)); - *(void **)data = impl->data; + // Create and return writable buffer + CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_writable_copy)); + impl->writable_block_id = VALGRIND_CREATE_BLOCK(impl->data_writable_copy, ctx_size, "Allocated writeable data buffer copy"); + memcpy(impl->data_writable_copy, impl->data_allocated, ctx_size); + *(void **)data = impl->data_writable_copy; return CEED_ERROR_SUCCESS; } @@ -114,13 +166,18 @@ static int CeedQFunctionContextGetDataRead_Memcheck(CeedQFunctionContext ctx, Ce size_t ctx_size; CeedQFunctionContext_Memcheck *impl; + CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend"); + CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size)); - CeedCallBackend(CeedQFunctionContextGetData_Memcheck(ctx, mem_type, data)); - // Make copy to verify no write occurred - CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_read_only_copy)); - memcpy(impl->data_read_only_copy, *(void **)data, ctx_size); + // Create and return read-only buffer + if (!impl->data_read_only_copy) { + CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_read_only_copy)); + impl->writable_block_id = VALGRIND_CREATE_BLOCK(impl->data_read_only_copy, ctx_size, "Allocated read-only data buffer copy"); + memcpy(impl->data_read_only_copy, impl->data_allocated, ctx_size); + } + *(void **)data = impl->data_read_only_copy; return CEED_ERROR_SUCCESS; } @@ -134,8 +191,14 @@ static int CeedQFunctionContextRestoreData_Memcheck(CeedQFunctionContext ctx) { CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size)); CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); - if (impl->data_borrowed) memcpy(impl->data_borrowed, impl->data, ctx_size); - if (impl->data_owned) memcpy(impl->data_owned, impl->data, ctx_size); + // Copy back to internal buffer and sync + memcpy(impl->data_allocated, impl->data_writable_copy, ctx_size); + CeedCallBackend(CeedQFunctionContextSyncData_Memcheck(ctx, CEED_MEM_HOST)); + + // Invalidate writable buffer + memset(impl->data_writable_copy, -42, ctx_size); + CeedCallBackend(CeedFree(&impl->data_writable_copy)); + VALGRIND_DISCARD(impl->writable_block_id); return CEED_ERROR_SUCCESS; } @@ -143,16 +206,23 @@ static int CeedQFunctionContextRestoreData_Memcheck(CeedQFunctionContext ctx) { // QFunctionContext Restore Data Read-Only //------------------------------------------------------------------------------ static int CeedQFunctionContextRestoreDataRead_Memcheck(CeedQFunctionContext ctx) { + Ceed ceed; size_t ctx_size; CeedQFunctionContext_Memcheck *impl; + CeedCallBackend(CeedQFunctionContextGetCeed(ctx, &ceed)); CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size)); CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); - CeedCheck(!memcmp(impl->data, impl->data_read_only_copy, ctx_size), CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, - "Context data changed while accessed in read-only mode"); + // Verify no changes made during read-only access + bool is_changed = memcmp(impl->data_allocated, impl->data_read_only_copy, ctx_size); + + CeedCheck(!is_changed, ceed, CEED_ERROR_BACKEND, "Context data changed while accessed in read-only mode"); + // Invalidate read-only buffer + memset(impl->data_read_only_copy, -42, ctx_size); CeedCallBackend(CeedFree(&impl->data_read_only_copy)); + VALGRIND_DISCARD(impl->read_only_block_id); return CEED_ERROR_SUCCESS; } @@ -160,20 +230,37 @@ static int CeedQFunctionContextRestoreDataRead_Memcheck(CeedQFunctionContext ctx // QFunctionContext destroy user data //------------------------------------------------------------------------------ static int CeedQFunctionContextDataDestroy_Memcheck(CeedQFunctionContext ctx) { + Ceed ceed; CeedMemType data_destroy_mem_type; CeedQFunctionContextDataDestroyUser data_destroy_function; CeedQFunctionContext_Memcheck *impl; + CeedCallBackend(CeedQFunctionContextGetCeed(ctx, &ceed)); CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); - CeedCallBackend(CeedQFunctionContextGetDataDestroy(ctx, &data_destroy_mem_type, &data_destroy_function)); - CeedCheck(data_destroy_mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, - "Can only destroy HOST memory for this backend"); + CeedCallBackend(CeedQFunctionContextGetDataDestroy(ctx, &data_destroy_mem_type, &data_destroy_function)); + CeedCheck(data_destroy_mem_type == CEED_MEM_HOST, ceed, CEED_ERROR_BACKEND, "Can only destroy HOST memory for this backend"); + // Run user destroy routine if (data_destroy_function) { - CeedCallBackend(data_destroy_function(impl->data_borrowed ? impl->data_borrowed : impl->data_owned)); + bool is_borrowed = !!impl->data_borrowed; + + CeedCallBackend(data_destroy_function(is_borrowed ? impl->data_borrowed : impl->data_owned)); + if (is_borrowed) VALGRIND_DISCARD(impl->borrowed_block_id); + else VALGRIND_DISCARD(impl->owned_block_id); + } + // Free allocations and discard block ids + if (impl->data_allocated) { + CeedCallBackend(CeedFree(&impl->data_allocated)); + VALGRIND_DISCARD(impl->allocated_block_id); + } + if (impl->data_owned) { + CeedCallBackend(CeedFree(&impl->data_owned)); + VALGRIND_DISCARD(impl->owned_block_id); + } + if (impl->data_borrowed) { + VALGRIND_DISCARD(impl->borrowed_block_id); } - CeedCallBackend(CeedFree(&impl->data_allocated)); return CEED_ERROR_SUCCESS; } @@ -183,9 +270,19 @@ static int CeedQFunctionContextDataDestroy_Memcheck(CeedQFunctionContext ctx) { static int CeedQFunctionContextDestroy_Memcheck(CeedQFunctionContext ctx) { CeedQFunctionContext_Memcheck *impl; + // Free allocations and discard block ids CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl)); - CeedCallBackend(CeedFree(&impl->data_allocated)); - CeedCallBackend(CeedFree(&impl->data_owned)); + if (impl->data_allocated) { + CeedCallBackend(CeedFree(&impl->data_allocated)); + VALGRIND_DISCARD(impl->allocated_block_id); + } + if (impl->data_owned) { + CeedCallBackend(CeedFree(&impl->data_owned)); + VALGRIND_DISCARD(impl->owned_block_id); + } + if (impl->data_borrowed) { + VALGRIND_DISCARD(impl->borrowed_block_id); + } CeedCallBackend(CeedFree(&impl)); return CEED_ERROR_SUCCESS; } diff --git a/backends/memcheck/ceed-memcheck-vector.c b/backends/memcheck/ceed-memcheck-vector.c index 48972948f8..23979c6f77 100644 --- a/backends/memcheck/ceed-memcheck-vector.c +++ b/backends/memcheck/ceed-memcheck-vector.c @@ -53,13 +53,13 @@ static int CeedVectorSetArray_Memcheck(CeedVector vec, CeedMemType mem_type, Cee // Clear previous owned arrays if (impl->array_allocated) { for (CeedSize i = 0; i < length; i++) impl->array_allocated[i] = NAN; + VALGRIND_DISCARD(impl->allocated_block_id); } CeedCallBackend(CeedFree(&impl->array_allocated)); - VALGRIND_DISCARD(impl->allocated_block_id); if (impl->array_owned) { for (CeedSize i = 0; i < length; i++) impl->array_owned[i] = NAN; + VALGRIND_DISCARD(impl->owned_block_id); } - VALGRIND_DISCARD(impl->owned_block_id); CeedCallBackend(CeedFree(&impl->array_owned)); // Clear borrowed block id, if present @@ -139,9 +139,9 @@ static int CeedVectorTakeArray_Memcheck(CeedVector vec, CeedMemType mem_type, Ce // De-allocate internal memory if (impl->array_allocated) { for (CeedSize i = 0; i < length; i++) impl->array_allocated[i] = NAN; + VALGRIND_DISCARD(impl->allocated_block_id); } CeedCallBackend(CeedFree(&impl->array_allocated)); - VALGRIND_DISCARD(impl->allocated_block_id); return CEED_ERROR_SUCCESS; } diff --git a/backends/memcheck/ceed-memcheck.h b/backends/memcheck/ceed-memcheck.h index 13e68dd035..45eb9c5ae3 100644 --- a/backends/memcheck/ceed-memcheck.h +++ b/backends/memcheck/ceed-memcheck.h @@ -46,12 +46,21 @@ typedef struct { } CeedQFunction_Memcheck; typedef struct { - int mem_block_id; - void *data; + // Internal data buffer + int allocated_block_id; void *data_allocated; + // Owned external data + int owned_block_id; void *data_owned; + // Borrowed external data + int borrowed_block_id; void *data_borrowed; + // Externally viewable read-only data + int read_only_block_id; void *data_read_only_copy; + // Externally viewable writable data + int writable_block_id; + void *data_writable_copy; } CeedQFunctionContext_Memcheck; CEED_INTERN int CeedVectorCreate_Memcheck(CeedSize n, CeedVector vec);