[Openmp-commits] [openmp] [Draft][OpenMP] Fix Taskgraph bugs (PR #136837)

[Josep Pinot via Openmp-commits]

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/4] [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/4] [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 b767dba29b8237cadfb19f23e0bc5c4849a113af 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/4] [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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/openmp/runtime/src/kmp_taskdeps.cpp b/openmp/runtime/src/kmp_taskdeps.cpp
index 281580d8f08dd..2630164024721 100644
--- a/openmp/runtime/src/kmp_taskdeps.cpp
+++ b/openmp/runtime/src/kmp_taskdeps.cpp
@@ -244,10 +244,14 @@ static inline void __kmp_track_dependence(kmp_int32 gtid, kmp_depnode_t *source,
     if (!exists) {
       if (source_info->nsuccessors >= source_info->successors_size) {
         source_info->successors_size = 2 * source_info->successors_size;
+        kmp_uint old_size = source_info->successors_size;
+        kmp_uint new_size = source_info->successors_size * 2;
         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_int32 *new_succ_ids =
+            (kmp_int32 *)__kmp_allocate(new_size * sizeof(kmp_int32));
+        KMP_MEMCPY(new_succ_ids, old_succ_ids, old_size * sizeof(kmp_int32));
         source_info->successors = new_succ_ids;
+        source_info->successors_size = new_size;
         __kmp_free(old_succ_ids);
       }
 

>From 648e9618798638c5c38c5a3d824b755703a8d059 Mon Sep 17 00:00:00 2001
From: jpinot <josep.pinot at bsc.es>
Date: Tue, 11 Mar 2025 14:30:49 +0100
Subject: [PATCH 4/4] [OpenMP] Fix task record/replay comments

---
 openmp/runtime/src/kmp_tasking.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/openmp/runtime/src/kmp_tasking.cpp b/openmp/runtime/src/kmp_tasking.cpp
index 6ae18dcb25c55..3d85a29423540 100644
--- a/openmp/runtime/src/kmp_tasking.cpp
+++ b/openmp/runtime/src/kmp_tasking.cpp
@@ -5454,7 +5454,6 @@ bool __kmpc_omp_has_task_team(kmp_int32 gtid) {
 
 #if OMPX_TASKGRAPH
 // __kmp_find_tdg: identify a TDG through its ID
-// gtid:   Global Thread ID
 // tdg_id: ID of the TDG
 // returns: If a TDG corresponding to this ID is found and not
 // its initial state, return the pointer to it, otherwise nullptr
@@ -5507,7 +5506,7 @@ void __kmp_print_tdg_dot(kmp_tdg_info_t *tdg, kmp_int32 gtid) {
   KA_TRACE(10, ("__kmp_print_tdg_dot(exit): T#%d tdg_id=%d \n", gtid, tdg_id));
 }
 
-// __kmp_start_record: launch the execution of a previous
+// __kmp_exec_tdg: launch the execution of a previous
 // recorded TDG
 // gtid:   Global Thread ID
 // tdg:    ID of the TDG