[Openmp-commits] [openmp] [OpenMP] Fix Taskgraph bugs (PR #136837)
Josep Pinot via Openmp-commits
openmp-commits at lists.llvm.org
Thu Apr 24 06:44:33 PDT 2025
https://github.com/jpinot updated https://github.com/llvm/llvm-project/pull/136837
>From 29c73b6613aca73abf8bea72895b425fb43e6365 Mon Sep 17 00:00:00 2001
From: jpinot <josep.pinot at bsc.es>
Date: Thu, 10 Apr 2025 15:00:43 +0200
Subject: [PATCH 1/3] [OpenMP] Initialize taskgraph-related fields in task
allocation
Fix a potential use of uninitialized is_taskgraph and tdg fields when a
task is created outside of a taskgraph construct. These fields were
previously left uninitialized, which could lead to undefined behavior
when later accessed.
---
openmp/runtime/src/kmp_tasking.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/openmp/runtime/src/kmp_tasking.cpp b/openmp/runtime/src/kmp_tasking.cpp
index 563aa29f6265e..6ae18dcb25c55 100644
--- a/openmp/runtime/src/kmp_tasking.cpp
+++ b/openmp/runtime/src/kmp_tasking.cpp
@@ -1614,6 +1614,8 @@ kmp_task_t *__kmp_task_alloc(ident_t *loc_ref, kmp_int32 gtid,
taskdata->td_flags.freed = 0;
#if OMPX_TASKGRAPH
taskdata->td_flags.onced = 0;
+ taskdata->is_taskgraph = 0;
+ taskdata->tdg = nullptr;
#endif
KMP_ATOMIC_ST_RLX(&taskdata->td_incomplete_child_tasks, 0);
// start at one because counts current task and children
>From 106473c3f2f2c11e28bd90debb52ad730c1592cb Mon Sep 17 00:00:00 2001
From: jpinot <josep.pinot at bsc.es>
Date: Mon, 21 Apr 2025 11:25:57 +0200
Subject: [PATCH 2/3] [OpenMP] Fix out-of-bounds access in
__kmpc_omp_task_with_deps due to incorrect task ID
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Correct use of task ID field when accessing the taskgraph’s record_map.
The previous logic mistakenly used td_task_id instead of td_tdg_task_id,
potentially causing out-of-bounds memory access when td_task_id exceeded
the map_size.
td_task_id and td_tdg_task_id was introduced in #130660
---
openmp/runtime/src/kmp_taskdeps.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/openmp/runtime/src/kmp_taskdeps.cpp b/openmp/runtime/src/kmp_taskdeps.cpp
index e002877aaead2..281580d8f08dd 100644
--- a/openmp/runtime/src/kmp_taskdeps.cpp
+++ b/openmp/runtime/src/kmp_taskdeps.cpp
@@ -698,9 +698,9 @@ kmp_int32 __kmpc_omp_task_with_deps(ident_t *loc_ref, kmp_int32 gtid,
__kmp_tdg_is_recording(new_taskdata->tdg->tdg_status)) {
kmp_tdg_info_t *tdg = new_taskdata->tdg;
// extend record_map if needed
- if (new_taskdata->td_task_id >= tdg->map_size) {
+ if (new_taskdata->td_tdg_task_id >= tdg->map_size) {
__kmp_acquire_bootstrap_lock(&tdg->graph_lock);
- if (new_taskdata->td_task_id >= tdg->map_size) {
+ if (new_taskdata->td_tdg_task_id >= tdg->map_size) {
kmp_uint old_size = tdg->map_size;
kmp_uint new_size = old_size * 2;
kmp_node_info_t *old_record = tdg->record_map;
>From b13090d8913db35ab3f30e5cb316140baa15644f Mon Sep 17 00:00:00 2001
From: jpinot <josep.pinot at bsc.es>
Date: Wed, 23 Apr 2025 09:25:25 +0200
Subject: [PATCH 3/3] [OpenMP] Fix reallocation of task successors in taskgraph
mode
Ensure proper resizing and copying of the successors array when its
capacity is exceeded. The previous implementation allocated a new array
but did not copy the existing elements. This led to loss of successor
data and incorrect task dependency tracking.
---
openmp/runtime/src/kmp_taskdeps.cpp | 2 +
.../omp_record_replay_deps_multi_succ.cpp | 56 +++++++++++++++++++
2 files changed, 58 insertions(+)
create mode 100644 openmp/runtime/test/tasking/omp_record_replay_deps_multi_succ.cpp
diff --git a/openmp/runtime/src/kmp_taskdeps.cpp b/openmp/runtime/src/kmp_taskdeps.cpp
index 281580d8f08dd..abbca752f0587 100644
--- a/openmp/runtime/src/kmp_taskdeps.cpp
+++ b/openmp/runtime/src/kmp_taskdeps.cpp
@@ -243,10 +243,12 @@ static inline void __kmp_track_dependence(kmp_int32 gtid, kmp_depnode_t *source,
}
if (!exists) {
if (source_info->nsuccessors >= source_info->successors_size) {
+ kmp_uint old_size = source_info->successors_size;
source_info->successors_size = 2 * source_info->successors_size;
kmp_int32 *old_succ_ids = source_info->successors;
kmp_int32 *new_succ_ids = (kmp_int32 *)__kmp_allocate(
source_info->successors_size * sizeof(kmp_int32));
+ KMP_MEMCPY(new_succ_ids, old_succ_ids, old_size * sizeof(kmp_int32));
source_info->successors = new_succ_ids;
__kmp_free(old_succ_ids);
}
diff --git a/openmp/runtime/test/tasking/omp_record_replay_deps_multi_succ.cpp b/openmp/runtime/test/tasking/omp_record_replay_deps_multi_succ.cpp
new file mode 100644
index 0000000000000..906fab335f510
--- /dev/null
+++ b/openmp/runtime/test/tasking/omp_record_replay_deps_multi_succ.cpp
@@ -0,0 +1,56 @@
+// REQUIRES: ompx_taskgraph
+// RUN: %libomp-cxx-compile-and-run
+#include <omp.h>
+#include <cassert>
+#include <vector>
+
+constexpr const int TASKS_SIZE = 12;
+
+typedef struct ident ident_t;
+
+extern "C" {
+int __kmpc_global_thread_num(ident_t *);
+int __kmpc_start_record_task(ident_t *, int, int, int);
+void __kmpc_end_record_task(ident_t *, int, int, int);
+}
+
+void init(int &A, int val) { A = val; }
+
+void update(int &A, int &B, int val) { A = B + val; }
+
+void test(int nb, std::vector<std::vector<int>> &Ah) {
+#pragma omp parallel
+#pragma omp single
+ {
+ int gtid = __kmpc_global_thread_num(nullptr);
+ int res = __kmpc_start_record_task(nullptr, gtid, 0, 0);
+ if (res) {
+ for (int k = 0; k < nb; ++k) {
+#pragma omp task depend(inout : Ah[k][0])
+ init(Ah[k][0], k);
+
+ for (int i = 1; i < nb; ++i) {
+#pragma omp task depend(in : Ah[k][0]) depend(out : Ah[k][i])
+ update(Ah[k][i], Ah[k][0], 1);
+ }
+ }
+ }
+ __kmpc_end_record_task(nullptr, gtid, 0, 0);
+ }
+}
+
+int main() {
+ std::vector<std::vector<int>> matrix(TASKS_SIZE,
+ std::vector<int>(TASKS_SIZE, 0));
+
+ test(TASKS_SIZE, matrix);
+ test(TASKS_SIZE, matrix);
+
+ for (int k = 0; k < TASKS_SIZE; ++k) {
+ assert(matrix[k][0] == k);
+ for (int i = 1; i < TASKS_SIZE; ++i) {
+ assert(matrix[k][i] == k + 1);
+ }
+ }
+ return 0;
+}
More information about the Openmp-commits
mailing list