Skip to content

Commit cebe980

Browse files
jeremyltjeremylt
committed
memcheck - also tidy up the ctx impl
1 parent d2117e1 commit cebe980

File tree

3 files changed

+143
-43
lines changed

3 files changed

+143
-43
lines changed

‎backends/memcheck/ceed-memcheck-qfunctioncontext.c‎

Lines changed: 129 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ static int CeedQFunctionContextHasValidData_Memcheck(CeedQFunctionContext ctx, b
2020
CeedQFunctionContext_Memcheck *impl;
2121

2222
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
23-
*has_valid_data = impl->data;
23+
*has_valid_data = !!impl->data_allocated;
2424
return CEED_ERROR_SUCCESS;
2525
}
2626

@@ -30,9 +30,10 @@ static int CeedQFunctionContextHasValidData_Memcheck(CeedQFunctionContext ctx, b
3030
static int CeedQFunctionContextHasBorrowedDataOfType_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type, bool *has_borrowed_data_of_type) {
3131
CeedQFunctionContext_Memcheck *impl;
3232

33-
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
3433
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend");
35-
*has_borrowed_data_of_type = impl->data_borrowed;
34+
35+
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
36+
*has_borrowed_data_of_type = !!impl->data_borrowed;
3637
return CEED_ERROR_SUCCESS;
3738
}
3839

@@ -43,52 +44,97 @@ static int CeedQFunctionContextSetData_Memcheck(CeedQFunctionContext ctx, CeedMe
4344
size_t ctx_size;
4445
CeedQFunctionContext_Memcheck *impl;
4546

47+
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend");
48+
4649
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
4750
CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size));
4851

49-
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend");
50-
52+
// Clear previous owned data buffers
53+
if (impl->data_allocated) {
54+
memset(impl->data_allocated, -42, ctx_size);
55+
VALGRIND_DISCARD(impl->allocated_block_id);
56+
}
5157
CeedCallBackend(CeedFree(&impl->data_allocated));
58+
if (impl->data_owned) {
59+
memset(impl->data_owned, -42, ctx_size);
60+
VALGRIND_DISCARD(impl->owned_block_id);
61+
}
5262
CeedCallBackend(CeedFree(&impl->data_owned));
63+
64+
// Clear borrowed block id, if present
65+
if (impl->data_borrowed) VALGRIND_DISCARD(impl->borrowed_block_id);
66+
67+
// Set internal pointers to external buffers
5368
switch (copy_mode) {
5469
case CEED_COPY_VALUES:
55-
CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_owned));
70+
impl->data_owned = NULL;
5671
impl->data_borrowed = NULL;
57-
impl->data = impl->data_owned;
58-
memcpy(impl->data, data, ctx_size);
5972
break;
6073
case CEED_OWN_POINTER:
61-
impl->data_owned = data;
62-
impl->data_borrowed = NULL;
63-
impl->data = data;
74+
impl->data_owned = data;
75+
impl->data_borrowed = NULL;
76+
impl->owned_block_id = VALGRIND_CREATE_BLOCK(impl->data_owned, ctx_size, "Owned external data buffer");
6477
break;
6578
case CEED_USE_POINTER:
66-
impl->data_borrowed = data;
67-
impl->data = data;
79+
impl->data_owned = NULL;
80+
impl->data_borrowed = data;
81+
impl->owned_block_id = VALGRIND_CREATE_BLOCK(impl->data_borrowed, ctx_size, "Borrowed external data buffer");
6882
}
69-
// Copy data to check ctx_size bounds
83+
84+
// Create internal data buffer
7085
CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_allocated));
71-
memcpy(impl->data_allocated, impl->data, ctx_size);
72-
impl->data = impl->data_allocated;
73-
VALGRIND_DISCARD(impl->mem_block_id);
74-
impl->mem_block_id = VALGRIND_CREATE_BLOCK(impl->data, ctx_size, "'QFunction backend context data copy'");
86+
impl->allocated_block_id = VALGRIND_CREATE_BLOCK(impl->data_allocated, ctx_size, "'Allocated internal context data buffer");
87+
memcpy(impl->data_allocated, data, ctx_size);
88+
return CEED_ERROR_SUCCESS;
89+
}
90+
91+
//------------------------------------------------------------------------------
92+
// Sync data
93+
//------------------------------------------------------------------------------
94+
static int CeedQFunctionContextSyncData_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type) {
95+
size_t ctx_size;
96+
CeedQFunctionContext_Memcheck *impl;
97+
98+
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend");
99+
100+
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
101+
CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size));
102+
103+
// Copy internal buffer back to owned or borrowed data buffer
104+
if (impl->data_owned) {
105+
memcpy(impl->data_owned, impl->data_allocated, ctx_size);
106+
}
107+
if (impl->data_borrowed) {
108+
memcpy(impl->data_borrowed, impl->data_allocated, ctx_size);
109+
}
75110
return CEED_ERROR_SUCCESS;
76111
}
77112

78113
//------------------------------------------------------------------------------
79114
// QFunctionContext Take Data
80115
//------------------------------------------------------------------------------
81116
static int CeedQFunctionContextTakeData_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type, void *data) {
117+
size_t ctx_size;
82118
CeedQFunctionContext_Memcheck *impl;
83119

120+
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only provide HOST memory for this backend");
121+
84122
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
123+
CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size));
85124

86-
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only provide HOST memory for this backend");
125+
// Synchronize memory
126+
CeedCallBackend(CeedQFunctionContextSyncData_Memcheck(ctx, CEED_MEM_HOST));
87127

128+
// Return borrowed buffer
88129
*(void **)data = impl->data_borrowed;
89130
impl->data_borrowed = NULL;
90-
impl->data = NULL;
91-
VALGRIND_DISCARD(impl->mem_block_id);
131+
VALGRIND_DISCARD(impl->borrowed_block_id);
132+
133+
// De-allocate internal memory
134+
if (impl->data_allocated) {
135+
memset(impl->data_allocated, -42, ctx_size);
136+
VALGRIND_DISCARD(impl->allocated_block_id);
137+
}
92138
CeedCallBackend(CeedFree(&impl->data_allocated));
93139
return CEED_ERROR_SUCCESS;
94140
}
@@ -97,13 +143,19 @@ static int CeedQFunctionContextTakeData_Memcheck(CeedQFunctionContext ctx, CeedM
97143
// QFunctionContext Get Data
98144
//------------------------------------------------------------------------------
99145
static int CeedQFunctionContextGetData_Memcheck(CeedQFunctionContext ctx, CeedMemType mem_type, void *data) {
146+
size_t ctx_size;
100147
CeedQFunctionContext_Memcheck *impl;
101148

102-
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
149+
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend");
103150

104-
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only provide HOST memory for this backend");
151+
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
152+
CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size));
105153

106-
*(void **)data = impl->data;
154+
// Create and return writable buffer
155+
CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_writable_copy));
156+
impl->writable_block_id = VALGRIND_CREATE_BLOCK(impl->data_writable_copy, ctx_size, "Allocated writeable data buffer copy");
157+
memcpy(impl->data_writable_copy, impl->data_allocated, ctx_size);
158+
*(void **)data = impl->data_writable_copy;
107159
return CEED_ERROR_SUCCESS;
108160
}
109161

@@ -114,13 +166,18 @@ static int CeedQFunctionContextGetDataRead_Memcheck(CeedQFunctionContext ctx, Ce
114166
size_t ctx_size;
115167
CeedQFunctionContext_Memcheck *impl;
116168

169+
CeedCheck(mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND, "Can only set HOST memory for this backend");
170+
117171
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
118172
CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size));
119-
CeedCallBackend(CeedQFunctionContextGetData_Memcheck(ctx, mem_type, data));
120173

121-
// Make copy to verify no write occurred
122-
CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_read_only_copy));
123-
memcpy(impl->data_read_only_copy, *(void **)data, ctx_size);
174+
// Create and return read-only buffer
175+
if (!impl->data_read_only_copy) {
176+
CeedCallBackend(CeedMallocArray(1, ctx_size, &impl->data_read_only_copy));
177+
impl->writable_block_id = VALGRIND_CREATE_BLOCK(impl->data_read_only_copy, ctx_size, "Allocated read-only data buffer copy");
178+
memcpy(impl->data_read_only_copy, impl->data_allocated, ctx_size);
179+
}
180+
*(void **)data = impl->data_read_only_copy;
124181
return CEED_ERROR_SUCCESS;
125182
}
126183

@@ -134,46 +191,71 @@ static int CeedQFunctionContextRestoreData_Memcheck(CeedQFunctionContext ctx) {
134191
CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size));
135192
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
136193

137-
if (impl->data_borrowed) memcpy(impl->data_borrowed, impl->data, ctx_size);
138-
if (impl->data_owned) memcpy(impl->data_owned, impl->data, ctx_size);
194+
// Copy back to internal buffer and sync
195+
memcpy(impl->data_allocated, impl->data_writable_copy, ctx_size);
196+
CeedCallBackend(CeedQFunctionContextSyncData_Memcheck(ctx, CEED_MEM_HOST));
197+
198+
// Invalidate writable buffer
199+
memset(impl->data_writable_copy, -42, ctx_size);
200+
CeedCallBackend(CeedFree(&impl->data_writable_copy));
201+
VALGRIND_DISCARD(impl->writable_block_id);
139202
return CEED_ERROR_SUCCESS;
140203
}
141204

142205
//------------------------------------------------------------------------------
143206
// QFunctionContext Restore Data Read-Only
144207
//------------------------------------------------------------------------------
145208
static int CeedQFunctionContextRestoreDataRead_Memcheck(CeedQFunctionContext ctx) {
209+
Ceed ceed;
146210
size_t ctx_size;
147211
CeedQFunctionContext_Memcheck *impl;
148212

213+
CeedCallBackend(CeedQFunctionContextGetCeed(ctx, &ceed));
149214
CeedCallBackend(CeedQFunctionContextGetContextSize(ctx, &ctx_size));
150215
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
151216

152-
CeedCheck(!memcmp(impl->data, impl->data_read_only_copy, ctx_size), CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND,
153-
"Context data changed while accessed in read-only mode");
217+
// Verify no changes made during read-only access
218+
bool is_changed = memcmp(impl->data_allocated, impl->data_read_only_copy, ctx_size);
154219

220+
CeedCheck(!is_changed, ceed, CEED_ERROR_BACKEND, "Context data changed while accessed in read-only mode");
221+
222+
// Invalidate read-only buffer
223+
memset(impl->data_read_only_copy, -42, ctx_size);
155224
CeedCallBackend(CeedFree(&impl->data_read_only_copy));
225+
VALGRIND_DISCARD(impl->read_only_block_id);
156226
return CEED_ERROR_SUCCESS;
157227
}
158228

159229
//------------------------------------------------------------------------------
160230
// QFunctionContext destroy user data
161231
//------------------------------------------------------------------------------
162232
static int CeedQFunctionContextDataDestroy_Memcheck(CeedQFunctionContext ctx) {
233+
Ceed ceed;
163234
CeedMemType data_destroy_mem_type;
164235
CeedQFunctionContextDataDestroyUser data_destroy_function;
165236
CeedQFunctionContext_Memcheck *impl;
166237

238+
CeedCallBackend(CeedQFunctionContextGetCeed(ctx, &ceed));
167239
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
168-
CeedCallBackend(CeedQFunctionContextGetDataDestroy(ctx, &data_destroy_mem_type, &data_destroy_function));
169240

170-
CeedCheck(data_destroy_mem_type == CEED_MEM_HOST, CeedQFunctionContextReturnCeed(ctx), CEED_ERROR_BACKEND,
171-
"Can only destroy HOST memory for this backend");
241+
CeedCallBackend(CeedQFunctionContextGetDataDestroy(ctx, &data_destroy_mem_type, &data_destroy_function));
242+
CeedCheck(data_destroy_mem_type == CEED_MEM_HOST, ceed, CEED_ERROR_BACKEND, "Can only destroy HOST memory for this backend");
172243

244+
// Run user destroy routine
173245
if (data_destroy_function) {
174246
CeedCallBackend(data_destroy_function(impl->data_borrowed ? impl->data_borrowed : impl->data_owned));
175247
}
176-
CeedCallBackend(CeedFree(&impl->data_allocated));
248+
// Free allocations and discard block ids
249+
if (impl->data_allocated) {
250+
CeedCallBackend(CeedFree(&impl->data_allocated));
251+
VALGRIND_DISCARD(impl->allocated_block_id);
252+
}
253+
if (impl->data_owned) {
254+
VALGRIND_DISCARD(impl->owned_block_id);
255+
}
256+
if (impl->data_borrowed) {
257+
VALGRIND_DISCARD(impl->borrowed_block_id);
258+
}
177259
return CEED_ERROR_SUCCESS;
178260
}
179261

@@ -183,9 +265,18 @@ static int CeedQFunctionContextDataDestroy_Memcheck(CeedQFunctionContext ctx) {
183265
static int CeedQFunctionContextDestroy_Memcheck(CeedQFunctionContext ctx) {
184266
CeedQFunctionContext_Memcheck *impl;
185267

268+
// Free allocations and discard block ids
186269
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
187-
CeedCallBackend(CeedFree(&impl->data_allocated));
188-
CeedCallBackend(CeedFree(&impl->data_owned));
270+
if (impl->data_allocated) {
271+
CeedCallBackend(CeedFree(&impl->data_allocated));
272+
VALGRIND_DISCARD(impl->allocated_block_id);
273+
}
274+
if (impl->data_owned) {
275+
VALGRIND_DISCARD(impl->owned_block_id);
276+
}
277+
if (impl->data_borrowed) {
278+
VALGRIND_DISCARD(impl->borrowed_block_id);
279+
}
189280
CeedCallBackend(CeedFree(&impl));
190281
return CEED_ERROR_SUCCESS;
191282
}

‎backends/memcheck/ceed-memcheck-vector.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,13 @@ static int CeedVectorSetArray_Memcheck(CeedVector vec, CeedMemType mem_type, Cee
5353
// Clear previous owned arrays
5454
if (impl->array_allocated) {
5555
for (CeedSize i = 0; i < length; i++) impl->array_allocated[i] = NAN;
56+
VALGRIND_DISCARD(impl->allocated_block_id);
5657
}
5758
CeedCallBackend(CeedFree(&impl->array_allocated));
58-
VALGRIND_DISCARD(impl->allocated_block_id);
5959
if (impl->array_owned) {
6060
for (CeedSize i = 0; i < length; i++) impl->array_owned[i] = NAN;
61+
VALGRIND_DISCARD(impl->owned_block_id);
6162
}
62-
VALGRIND_DISCARD(impl->owned_block_id);
6363
CeedCallBackend(CeedFree(&impl->array_owned));
6464

6565
// Clear borrowed block id, if present
@@ -139,9 +139,9 @@ static int CeedVectorTakeArray_Memcheck(CeedVector vec, CeedMemType mem_type, Ce
139139
// De-allocate internal memory
140140
if (impl->array_allocated) {
141141
for (CeedSize i = 0; i < length; i++) impl->array_allocated[i] = NAN;
142+
VALGRIND_DISCARD(impl->allocated_block_id);
142143
}
143144
CeedCallBackend(CeedFree(&impl->array_allocated));
144-
VALGRIND_DISCARD(impl->allocated_block_id);
145145
return CEED_ERROR_SUCCESS;
146146
}
147147

‎backends/memcheck/ceed-memcheck.h‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,21 @@ typedef struct {
4646
} CeedQFunction_Memcheck;
4747

4848
typedef struct {
49-
int mem_block_id;
50-
void *data;
49+
// Internal data buffer
50+
int allocated_block_id;
5151
void *data_allocated;
52+
// Owned external data
53+
int owned_block_id;
5254
void *data_owned;
55+
// Borrowed external data
56+
int borrowed_block_id;
5357
void *data_borrowed;
58+
// Externally viewable read-only data
59+
int read_only_block_id;
5460
void *data_read_only_copy;
61+
// Externally viewable writable data
62+
int writable_block_id;
63+
void *data_writable_copy;
5564
} CeedQFunctionContext_Memcheck;
5665

5766
CEED_INTERN int CeedVectorCreate_Memcheck(CeedSize n, CeedVector vec);

0 commit comments

Comments
 (0)