Skip to content

Commit 8f302d0

Browse files
jeremyltjeremylt
authored
Merge pull request #1664 from CEED/jeremy/memcheck-tidy
memcheck - clarify vector access
2 parents f009e52 + 2bf66f3 commit 8f302d0

File tree

4 files changed

+410
-102
lines changed

4 files changed

+410
-102
lines changed

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

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
static int CeedQFunctionApply_Memcheck(CeedQFunction qf, CeedInt Q, CeedVector *U, CeedVector *V) {
2020
Ceed ceed;
2121
void *ctx_data = NULL;
22+
int input_block_ids[CEED_FIELD_MAX], output_block_ids[CEED_FIELD_MAX];
2223
CeedInt num_in, num_out;
2324
CeedQFunctionUser f = NULL;
2425
CeedQFunctionField *output_fields;
@@ -29,12 +30,21 @@ static int CeedQFunctionApply_Memcheck(CeedQFunction qf, CeedInt Q, CeedVector *
2930
CeedCallBackend(CeedQFunctionGetContextData(qf, CEED_MEM_HOST, &ctx_data));
3031
CeedCallBackend(CeedQFunctionGetUserFunction(qf, &f));
3132
CeedCallBackend(CeedQFunctionGetNumArgs(qf, &num_in, &num_out));
32-
int mem_block_ids[num_out];
3333

34-
// Get input/output arrays
34+
// Get input arrays
3535
for (CeedInt i = 0; i < num_in; i++) {
36+
CeedSize len;
37+
char name[32] = "";
38+
3639
CeedCallBackend(CeedVectorGetArrayRead(U[i], CEED_MEM_HOST, &impl->inputs[i]));
40+
41+
CeedCallBackend(CeedVectorGetLength(U[i], &len));
42+
43+
snprintf(name, 32, "QFunction input %" CeedInt_FMT, i);
44+
input_block_ids[i] = VALGRIND_CREATE_BLOCK(impl->inputs[i], len, name);
3745
}
46+
47+
// Get output arrays
3848
for (CeedInt i = 0; i < num_out; i++) {
3949
CeedSize len;
4050
char name[32] = "";
@@ -44,8 +54,8 @@ static int CeedQFunctionApply_Memcheck(CeedQFunction qf, CeedInt Q, CeedVector *
4454
CeedCallBackend(CeedVectorGetLength(V[i], &len));
4555
VALGRIND_MAKE_MEM_UNDEFINED(impl->outputs[i], len);
4656

47-
snprintf(name, 32, "'QFunction output %" CeedInt_FMT "'", i);
48-
mem_block_ids[i] = VALGRIND_CREATE_BLOCK(impl->outputs[i], len, name);
57+
snprintf(name, 32, "QFunction output %" CeedInt_FMT, i);
58+
output_block_ids[i] = VALGRIND_CREATE_BLOCK(impl->outputs[i], len, name);
4959
}
5060

5161
// Call user function
@@ -54,26 +64,30 @@ static int CeedQFunctionApply_Memcheck(CeedQFunction qf, CeedInt Q, CeedVector *
5464
// Restore input arrays
5565
for (CeedInt i = 0; i < num_in; i++) {
5666
CeedCallBackend(CeedVectorRestoreArrayRead(U[i], &impl->inputs[i]));
67+
VALGRIND_DISCARD(input_block_ids[i]);
5768
}
58-
// Check for unset output values
69+
70+
// Check for unset output values and restore arrays
5971
{
6072
const char *kernel_name, *kernel_path;
6173

6274
CeedCallBackend(CeedQFunctionGetSourcePath(qf, &kernel_path));
6375
CeedCallBackend(CeedQFunctionGetKernelName(qf, &kernel_name));
6476
CeedCallBackend(CeedQFunctionGetFields(qf, NULL, NULL, NULL, &output_fields));
6577
for (CeedInt i = 0; i < num_out; i++) {
66-
CeedInt field_size;
78+
const char *field_name;
79+
CeedInt field_size;
6780

6881
// Note: need field size because vector may be longer than needed for output
6982
CeedCallBackend(CeedQFunctionFieldGetSize(output_fields[i], &field_size));
83+
CeedCallBackend(CeedQFunctionFieldGetName(output_fields[i], &field_name));
7084
for (CeedSize j = 0; j < field_size * (CeedSize)Q; j++) {
7185
CeedCheck(!isnan(impl->outputs[i][j]), ceed, CEED_ERROR_BACKEND,
72-
"QFunction output %" CeedInt_FMT " entry %" CeedSize_FMT " is NaN after restoring write-only access: %s:%s ", i, j, kernel_path,
73-
kernel_name);
86+
"QFunction output %" CeedInt_FMT " '%s' entry %" CeedSize_FMT " is NaN after restoring write-only access: %s:%s ", i, field_name, j,
87+
kernel_path, kernel_name);
7488
}
7589
CeedCallBackend(CeedVectorRestoreArray(V[i], &impl->outputs[i]));
76-
VALGRIND_DISCARD(mem_block_ids[i]);
90+
VALGRIND_DISCARD(output_block_ids[i]);
7791
}
7892
}
7993
CeedCallBackend(CeedQFunctionRestoreContextData(qf, &ctx_data));

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

Lines changed: 136 additions & 39 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,76 @@ 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);
219+
220+
CeedCheck(!is_changed, ceed, CEED_ERROR_BACKEND, "Context data changed while accessed in read-only mode");
154221

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) {
174-
CeedCallBackend(data_destroy_function(impl->data_borrowed ? impl->data_borrowed : impl->data_owned));
246+
bool is_borrowed = !!impl->data_borrowed;
247+
248+
CeedCallBackend(data_destroy_function(is_borrowed ? impl->data_borrowed : impl->data_owned));
249+
if (is_borrowed) VALGRIND_DISCARD(impl->borrowed_block_id);
250+
else VALGRIND_DISCARD(impl->owned_block_id);
251+
}
252+
// Free allocations and discard block ids
253+
if (impl->data_allocated) {
254+
CeedCallBackend(CeedFree(&impl->data_allocated));
255+
VALGRIND_DISCARD(impl->allocated_block_id);
256+
}
257+
if (impl->data_owned) {
258+
CeedCallBackend(CeedFree(&impl->data_owned));
259+
VALGRIND_DISCARD(impl->owned_block_id);
260+
}
261+
if (impl->data_borrowed) {
262+
VALGRIND_DISCARD(impl->borrowed_block_id);
175263
}
176-
CeedCallBackend(CeedFree(&impl->data_allocated));
177264
return CEED_ERROR_SUCCESS;
178265
}
179266

@@ -183,9 +270,19 @@ static int CeedQFunctionContextDataDestroy_Memcheck(CeedQFunctionContext ctx) {
183270
static int CeedQFunctionContextDestroy_Memcheck(CeedQFunctionContext ctx) {
184271
CeedQFunctionContext_Memcheck *impl;
185272

273+
// Free allocations and discard block ids
186274
CeedCallBackend(CeedQFunctionContextGetBackendData(ctx, &impl));
187-
CeedCallBackend(CeedFree(&impl->data_allocated));
188-
CeedCallBackend(CeedFree(&impl->data_owned));
275+
if (impl->data_allocated) {
276+
CeedCallBackend(CeedFree(&impl->data_allocated));
277+
VALGRIND_DISCARD(impl->allocated_block_id);
278+
}
279+
if (impl->data_owned) {
280+
CeedCallBackend(CeedFree(&impl->data_owned));
281+
VALGRIND_DISCARD(impl->owned_block_id);
282+
}
283+
if (impl->data_borrowed) {
284+
VALGRIND_DISCARD(impl->borrowed_block_id);
285+
}
189286
CeedCallBackend(CeedFree(&impl));
190287
return CEED_ERROR_SUCCESS;
191288
}

0 commit comments

Comments
 (0)