From 5c00c84cac796147370a1f078175b55f41c53c57 Mon Sep 17 00:00:00 2001 From: TB Schardl Date: Mon, 17 Nov 2025 14:42:05 -0500 Subject: [PATCH] [cilksan] Add support for new C++ reducer interface. --- cilksan/cilksan_internal.h | 57 ++++++++-- cilksan/hyperobject_base.h | 18 +-- cilksan/hypertable.h | 10 +- cilksan/reducers.cpp | 207 ++++++++++++++++++++++++++--------- test/cilksan/TestCases/vla.c | 11 +- 5 files changed, 227 insertions(+), 76 deletions(-) diff --git a/cilksan/cilksan_internal.h b/cilksan/cilksan_internal.h index 7b6da63..a4eb867 100644 --- a/cilksan/cilksan_internal.h +++ b/cilksan/cilksan_internal.h @@ -169,33 +169,68 @@ class CilkSanImpl_t { hyper_table::bucket *b = reducer_views->find(key); if (b) { assert(key == b->key); - return b->value.view; + return b->data.view; } return nullptr; } // Create a new reducer view. - void *create_reducer_view(hyper_table *__restrict__ reducer_views, - uintptr_t key, size_t size, void *identity_ptr, - void *reduce_ptr) { - __cilk_identity_fn identity = (__cilk_identity_fn)identity_ptr; - __cilk_reduce_fn reduce = (__cilk_reduce_fn)reduce_ptr; + void *create_reducer_view_0(hyper_table *__restrict__ reducer_views, + __reducer_base *key) { + // Create a new view and initialize it with the identity function. + size_t size = key->size(); + void *new_view = malloc(size); + DBG_TRACE(REDUCER, "create_reducer_view_0(%p): created view %p -> %p\n", + (void *)reducer_views, (void *)key, new_view); + mark_alloc(new_view, size); + __reducer_base *base = key->identity(new_view); + // Insert the new view into the local hypertable. + hyper_table::bucket new_bucket = { + .key = (uintptr_t)key, + .data = {.view = new_view, .extra = base}}; + [[maybe_unused]] bool success = reducer_views->insert(new_bucket); + assert(success && "create_reducer_view failed to insert new reducer."); + + // Return the new view. + return new_view; + } + void *create_reducer_view_1(hyper_table *__restrict__ reducer_views, + uintptr_t key, + const __reducer_callbacks &callbacks) { + // Create a new view and initialize it with the identity function. + void *new_view = malloc(callbacks.size); + DBG_TRACE(REDUCER, "create_reducer_view_1(%p): created view %p -> %p\n", + (void *)reducer_views, (void *)key, new_view); + mark_alloc(new_view, callbacks.size); + callbacks.identity(new_view); + // Insert the new view into the local hypertable. + hyper_table::bucket new_bucket = { + .key = (uintptr_t)key, + .data = {.view = new_view, .extra = &callbacks.reduce}}; + [[maybe_unused]] bool success = reducer_views->insert(new_bucket); + assert(success && "create_reducer_view failed to insert new reducer."); + + // Return the new view. + return new_view; + } + + void *create_reducer_view_2(hyper_table *__restrict__ reducer_views, + uintptr_t key, size_t size, void (*identity)(void *), + void (*reduce)(void *, void *)) { // Allocate and initialize a new view. Make sure the shadow memory is clear // for that allocation. void *new_view = malloc(size); - DBG_TRACE(REDUCER, "create_reducer_view(%p): created view %p -> %p\n", + DBG_TRACE(REDUCER, "create_reducer_view_2(%p): created view %p -> %p\n", (void *)reducer_views, (void *)key, new_view); mark_alloc(new_view, size); identity(new_view); // Insert the view into the table of reducer_views. hyper_table::bucket new_bucket = { - .key = (uintptr_t)key, - .value = {.view = new_view, .reduce_fn = reduce}}; - bool success = reducer_views->insert(new_bucket); + .key = (uintptr_t)key, .data = {.view = new_view, .extra = reduce}}; + [[maybe_unused]] bool success = reducer_views->insert(new_bucket); assert(success && "create_reducer_view failed to insert new reducer."); - (void)success; // Return the new view. return new_view; diff --git a/cilksan/hyperobject_base.h b/cilksan/hyperobject_base.h index 6b934c6..da6d403 100644 --- a/cilksan/hyperobject_base.h +++ b/cilksan/hyperobject_base.h @@ -2,27 +2,29 @@ #ifndef _HYPEROBJECT_BASE #define _HYPEROBJECT_BASE -#include -#include +#include +#include // Reducer data. // // NOTE: Since the size and identity_fn are only used when a worker // looks up a reducer after a steal, we don't need to store these in -// the reducer_base structure as long as the reducer_lookup function +// the reducer_data structure as long as the reducer_lookup function // gets them as parameters. // // TODO: For small reducer views of size less than sizeof(void *), -// consider storing the view directly within the reducer_base +// consider storing the view directly within the reducer_data. // structure. -// - Problem: A reducer_base structure may move around in the hash +// - Problem: A reducer_data structure may move around in the hash // table as other reducers are inserted. As a result, a pointer to // a view may be invalidated by other hyper_lookup operations. // - Problem: Need a way to keep track of whether the view in a -// reducer_base is storing a pointer to the view or the view itself. -struct reducer_base { +// reducer_data is storing a pointer to the view or the view itself. +struct reducer_data { void *view = nullptr; - __cilk_reduce_fn reduce_fn = nullptr; + std::variant<__reducer_base *, const std::function *, + void (*)(void *, void *)> + extra; }; #endif /* _HYPEROBJECT_BASE */ diff --git a/cilksan/hypertable.h b/cilksan/hypertable.h index 9d0ca49..0519f16 100644 --- a/cilksan/hypertable.h +++ b/cilksan/hypertable.h @@ -28,7 +28,13 @@ class hyper_table { struct bucket { uintptr_t key = KEY_EMPTY; /* EMPTY, DELETED, or a user-provided pointer. */ index_t hash = 0; /* hash of the key when inserted into the table. */ - reducer_base value; + reducer_data data; + + static void reduce(bucket *left, bucket *right); + + bool is_empty() const { return key == KEY_EMPTY; } + bool is_tombstone() const { return key == KEY_DELETED; } + bool is_valid() const { return key != KEY_EMPTY && key != KEY_DELETED; } void make_tombstone() { key = KEY_DELETED; } }; @@ -370,7 +376,7 @@ class hyper_table { // Found the key? Overwrite that bucket. // TODO: Reconsider what to do in this case. if (b.key == curr_key) { - buckets[i].value = b.value; + buckets[i].data = b.data; return true; } diff --git a/cilksan/reducers.cpp b/cilksan/reducers.cpp index a0e8a9f..30842bb 100644 --- a/cilksan/reducers.cpp +++ b/cilksan/reducers.cpp @@ -1,15 +1,21 @@ #include "cilksan_internal.h" #include "debug_util.h" #include "driver.h" +#include "hypertable.h" #include "vector.h" +#include #include +#include #include #include +#include +#include // Hooks for handling reducer hyperobjects. +template static void reducer_register(const csi_id_t call_id, unsigned MAAP_count, - void *key, void *identity_ptr, void *reduce_ptr) { + void *key, ExtraTy *extra) { for (unsigned i = 0; i < MAAP_count; ++i) MAAPs.pop(); @@ -17,7 +23,7 @@ static void reducer_register(const csi_id_t call_id, unsigned MAAP_count, hyper_table *reducer_views = CilkSanImpl.get_or_create_reducer_views(); reducer_views->insert((hyper_table::bucket){ .key = (uintptr_t)key, - .value = {.view = key, .reduce_fn = (__cilk_reduce_fn)reduce_ptr}}); + .data = {.view = key, .extra = extra}}); DBG_TRACE(REDUCER, "reducer_register: registered %p, reducer_views %p, occupancy %d\n", key, reducer_views, reducer_views->occupancy); @@ -31,23 +37,31 @@ static void reducer_register(const csi_id_t call_id, unsigned MAAP_count, } CILKSAN_API void -__csan_llvm_reducer_register_i32(const csi_id_t call_id, const csi_id_t func_id, - unsigned MAAP_count, const call_prop_t prop, - void *key, size_t size, void *identity_ptr, - void *reduce_ptr) { - START_HOOK(call_id); - - reducer_register(call_id, MAAP_count, key, identity_ptr, reduce_ptr); -} - -CILKSAN_API void -__csan_llvm_reducer_register_i64(const csi_id_t call_id, const csi_id_t func_id, - unsigned MAAP_count, const call_prop_t prop, - void *key, size_t size, void *identity_ptr, - void *reduce_ptr) { +__csan_llvm_reducer_register(const csi_id_t call_id, const csi_id_t func_id, + unsigned MAAP_count, const call_prop_t prop, + int32_t type, void *key, void *data) { START_HOOK(call_id); - - reducer_register(call_id, MAAP_count, key, identity_ptr, reduce_ptr); + switch (type) { + case 0: { + __reducer_base *rb = static_cast<__reducer_base *>(key); + reducer_register(call_id, MAAP_count, key, rb); + break; + } + case 1: { + __reducer_callbacks *cb = + static_cast<__reducer_callbacks *>(data); + reducer_register(call_id, MAAP_count, key, static_cast<__cilk_reduce_fn *>(&cb->reduce)); + break; + } + case 2: { + void (*reduce)(void *, void *) = + reinterpret_cast(data); + reducer_register(call_id, MAAP_count, key, reduce); + break; + } + default: + cilksan_assert(false && "Unknown reducer type in reducer_register"); + } } CILKSAN_API void __csan_llvm_reducer_unregister(const csi_id_t call_id, @@ -76,14 +90,12 @@ CILKSAN_API void __csan_llvm_reducer_unregister(const csi_id_t call_id, check_read_bytes(call_id, MAAP_t::Ref, key, 1); } -CILKSAN_API void *__csan_llvm_hyper_lookup(const csi_id_t call_id, - const csi_id_t func_id, - unsigned MAAP_count, - const call_prop_t prop, void *view, - void *key, size_t size, - void *identity_fn, void *reduce_fn) { +std::pair +hyper_lookup_common(const csi_id_t call_id, const csi_id_t func_id, + unsigned MAAP_count, const call_prop_t prop, + void *view, void *key) { if (!CILKSAN_INITIALIZED || !should_check()) - return view; + return {view, nullptr}; if (__builtin_expect(!call_pc[call_id], false)) call_pc[call_id] = CALLERPC; @@ -91,7 +103,7 @@ CILKSAN_API void *__csan_llvm_hyper_lookup(const csi_id_t call_id, MAAPs.pop(); if (!is_execution_parallel()) - return view; + return {view, nullptr}; if (CilkSanImpl.stealable()) { // Get the table of reducer views to update. @@ -100,22 +112,61 @@ CILKSAN_API void *__csan_llvm_hyper_lookup(const csi_id_t call_id, if (void *new_view = CilkSanImpl.reducer_lookup(reducer_views, (uintptr_t)key)) { DBG_TRACE(REDUCER, "hyper_lookup: found view: %p -> %p\n", key, new_view); - return new_view; + return {new_view, reducer_views}; } - // Create and return a new reducer view. - return CilkSanImpl.create_reducer_view(reducer_views, (uintptr_t)key, size, - identity_fn, reduce_fn); + // Return nullptr for the view and a pointer to the hyper table. The caller + // will create a new view and install it in the hyper table. + return {nullptr, reducer_views}; } - return view; + return {view, nullptr}; +} + +CILKSAN_API void *__csan_llvm_hyper_lookup_0(const csi_id_t call_id, + const csi_id_t func_id, + unsigned MAAP_count, + const call_prop_t prop, void *view, + __reducer_base *key) { + auto result = + hyper_lookup_common(call_id, func_id, MAAP_count, prop, view, key); + if (result.first || result.second == nullptr) + return result.first; + // Create and return a new reducer view. + return CilkSanImpl.create_reducer_view_0(result.second, key); } CILKSAN_API void * -__csan_llvm_hyper_lookup_i64(const csi_id_t call_id, const csi_id_t func_id, - unsigned MAAP_count, const call_prop_t prop, - void *view, void *key, size_t size, - void *identity_fn, void *reduce_fn) { - return __csan_llvm_hyper_lookup(call_id, func_id, MAAP_count, prop, view, key, - size, identity_fn, reduce_fn); +__csan_llvm_hyper_lookup_1(const csi_id_t call_id, const csi_id_t func_id, + unsigned MAAP_count, const call_prop_t prop, + void *view, void *key, + const __reducer_callbacks &callbacks) { + auto result = + hyper_lookup_common(call_id, func_id, MAAP_count, prop, view, key); + if (result.first || result.second == nullptr) + return result.first; + // Create and return a new reducer view. + return CilkSanImpl.create_reducer_view_1(result.second, (uintptr_t)key, + callbacks); +} + +CILKSAN_API void *__csan_llvm_hyper_lookup_2( + const csi_id_t call_id, const csi_id_t func_id, unsigned MAAP_count, + const call_prop_t prop, void *view, void *key, size_t size, + void (*identity_fn)(void *), void (*reduce_fn)(void *, void *)) { + auto result = + hyper_lookup_common(call_id, func_id, MAAP_count, prop, view, key); + if (result.first || result.second == nullptr) + return result.first; + // Create and return a new reducer view. + return CilkSanImpl.create_reducer_view_2(result.second, (uintptr_t)key, size, + identity_fn, reduce_fn); +} + +CILKSAN_API void *__csan_llvm_hyper_lookup_2_i64( + const csi_id_t call_id, const csi_id_t func_id, unsigned MAAP_count, + const call_prop_t prop, void *view, void *key, size_t size, + void (*identity_fn)(void *), void (*reduce_fn)(void *, void *)) { + return __csan_llvm_hyper_lookup_2(call_id, func_id, MAAP_count, prop, view, + key, size, identity_fn, reduce_fn); } void CilkSanImpl_t::reduce_local_views() { @@ -153,21 +204,42 @@ void CilkSanImpl_t::reduce_local_views() { hyper_table::bucket b = buckets[i]; if (!is_valid(b.key)) continue; - if (b.key == (uintptr_t)(b.value.view)) { + if (b.key == (uintptr_t)(b.data.view)) { holdsLeftmostViews = true; continue; } DBG_TRACE(REDUCER, "reduce_local_views: found view to reduce at %d: %p -> %p\n", i, - (void *)b.key, (void *)b.value.view); - // The key is the pointer to the leftmost view. - void *left_view = (void *)b.key; - reducer_base rb = b.value; - rb.reduce_fn(left_view, rb.view); - // Delete the right view. - free(rb.view); - mark_free(rb.view); + (void *)b.key, (void *)b.data.view); + + void *left_view = (void *)b.key, *right_view = b.data.view; + { + // Custom version of hyper_table::bucket::reduce() to reduce view with key + // in the same bucket. The key points to the leftmost view. + reducer_data rd = b.data; + if (std::holds_alternative<__reducer_base *>(rd.extra)) { + __reducer_base *leftmost = + static_cast<__reducer_base *>(reinterpret_cast(b.key)); + __reducer_base *left_r = leftmost; + __reducer_base *right_r = std::get<__reducer_base *>(rd.extra); + leftmost->reduce(left_r, right_r); + right_r->~__reducer_base(); + } else if (std::holds_alternative(rd.extra)) { + const __cilk_reduce_fn *reduce_fn = + std::get(rd.extra); + (*reduce_fn)(left_view, right_view); + } else { + void (*reduce_fn)(void *, void *) = + std::get(rd.extra); + reduce_fn(left_view, right_view); + } + rd.extra = (__reducer_base *)nullptr; + rd.view = nullptr; + } + // Free the right view. + free(right_view); + mark_free(right_view); keysToRemove.push_back(b.key); } enable_checking(); @@ -184,6 +256,33 @@ void CilkSanImpl_t::reduce_local_views() { } } +void hyper_table::bucket::reduce(bucket *left, bucket *right) { + assert(left->data.extra.index() == right->data.extra.index()); + void *left_view = left->data.view, *right_view = right->data.view; + if (std::holds_alternative<__reducer_base *>(left->data.extra)) { + __reducer_base *leftmost = + static_cast<__reducer_base *> + (reinterpret_cast(left->key)); + __reducer_base *left_r = std::get<__reducer_base *>(left->data.extra); + __reducer_base *right_r = + std::get<__reducer_base *>(right->data.extra); + leftmost->reduce(left_r, right_r); + right_r->~__reducer_base(); + } else if (std::holds_alternative(left->data.extra)) { + const __cilk_reduce_fn *reduce_fn = + std::get(left->data.extra); + (*reduce_fn)(left_view, right_view); + } else { + void (*reduce_fn)(void *, void *) = + std::get(left->data.extra); + reduce_fn(left_view, right_view); + } + right->data.extra = (__reducer_base *)nullptr; + right->data.view = nullptr; + // Free the right view. + free(right_view); +} + hyper_table * hyper_table::merge_two_hyper_tables(CilkSanImpl_t *__restrict__ tool, hyper_table *__restrict__ left, @@ -236,16 +335,16 @@ hyper_table::merge_two_hyper_tables(CilkSanImpl_t *__restrict__ tool, } else { // Merge the two views in the source and destination buckets, being sure // to preserve left-to-right ordering. Free the right view when done. - reducer_base dst_rb = dst_bucket->value; + reducer_data dst_rd = dst_bucket->data; if (left_dst) { - dst_rb.reduce_fn(dst_rb.view, b.value.view); - free(b.value.view); - tool->mark_free(b.value.view); + bucket::reduce(dst_bucket, &b); + tool->mark_free(b.data.view); } else { - dst_rb.reduce_fn(b.value.view, dst_rb.view); - free(dst_rb.view); - tool->mark_free(dst_rb.view); - dst_bucket->value.view = b.value.view; + bucket::reduce(&b, dst_bucket); + tool->mark_free(dst_rd.view); + dst_bucket->data = b.data; + b.data.extra = (__reducer_base *)nullptr; + b.data.view = nullptr; } } } diff --git a/test/cilksan/TestCases/vla.c b/test/cilksan/TestCases/vla.c index 4d94a28..15ed3d5 100644 --- a/test/cilksan/TestCases/vla.c +++ b/test/cilksan/TestCases/vla.c @@ -62,6 +62,15 @@ int main(int argc, char *argv[]) return errors != 0; } +// CHECK: Race detected on location [[DATA:[0-9a-f]+]] +// CHECK-NEXT: * Write {{[0-9a-f]+}} fill +// CHECK-NEXT: to variable data +// CHECK-NEXT: Spawn {{[0-9a-f]+}} loop +// CHECK-NEXT: * Read {{[0-9a-f]+}} loop +// CHECK: Common calling context +// CHECK-NEXT: Call {{[0-9a-f]+}} main +// CHECK: Stack object + // CHECK: Race detected on location [[DATA:[0-9a-f]+]] // CHECK-NEXT: * Write {{[0-9a-f]+}} fill // CHECK-NEXT: to variable data @@ -80,5 +89,5 @@ int main(int argc, char *argv[]) // CHECK-NEXT: Call {{[0-9a-f]+}} main // CHECK: Stack object -// CHECK: Cilksan detected 2 distinct races. +// CHECK: Cilksan detected 3 distinct races. // CHECK-NEXT: Cilksan suppressed {{[0-9]+}} duplicate race reports.