Skip to content

Commit 2518cc6

Browse files
committed
[OpenMP][FIX] Avoid use of stack allocations in asynchronous calls
As reported by Guilherme Valarini [0], we used to pass stack allocations to calls that can nowadays be asynchronous. This is arguably a problem and it will inevitably result in UB. To remedy the situation we allocate the locations as part of the AsyncInfoTy object. The lifetime of that object matches what we need for now. If the synchronization is not tied to the AsyncInfoTy object anymore we might need to have a different buffer construct in global space. This should be back-ported to LLVM 12 but needs slight modifications as it is based on refactoring patches we do not need to backport. [0] https://lists.llvm.org/pipermail/openmp-dev/2021-February/003867.html Reviewed By: JonChesterfield Differential Revision: https://reviews.llvm.org/D96667
1 parent 758b849 commit 2518cc6

File tree

2 files changed

+19
-3
lines changed

2 files changed

+19
-3
lines changed

openmp/libomptarget/include/omptarget.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#ifndef _OMPTARGET_H_
1515
#define _OMPTARGET_H_
1616

17+
#include <deque>
1718
#include <stddef.h>
1819
#include <stdint.h>
1920

@@ -136,6 +137,10 @@ struct DeviceTy;
136137
/// associated with a libomptarget layer device. RAII semantics to avoid
137138
/// mistakes.
138139
class AsyncInfoTy {
140+
/// Locations we used in (potentially) asynchronous calls which should live
141+
/// as long as this AsyncInfoTy object.
142+
std::deque<void *> BufferLocations;
143+
139144
__tgt_async_info AsyncInfo;
140145
DeviceTy &Device;
141146

@@ -151,6 +156,10 @@ class AsyncInfoTy {
151156
///
152157
/// \returns OFFLOAD_FAIL or OFFLOAD_SUCCESS appropriately.
153158
int synchronize();
159+
160+
/// Return a void* reference with a lifetime that is at least as long as this
161+
/// AsyncInfoTy object. The location can be used as intermediate buffer.
162+
void *&getVoidPtrLocation();
154163
};
155164

156165
/// This struct is a record of non-contiguous information

openmp/libomptarget/src/omptarget.cpp

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,11 @@ int AsyncInfoTy::synchronize() {
3131
return Result;
3232
}
3333

34+
void *&AsyncInfoTy::getVoidPtrLocation() {
35+
BufferLocations.push_back(nullptr);
36+
return BufferLocations.back();
37+
}
38+
3439
/* All begin addresses for partially mapped structs must be 8-aligned in order
3540
* to ensure proper alignment of members. E.g.
3641
*
@@ -432,7 +437,8 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
432437
DP("Update pointer (" DPxMOD ") -> [" DPxMOD "]\n",
433438
DPxPTR(PointerTgtPtrBegin), DPxPTR(TgtPtrBegin));
434439
uint64_t Delta = (uint64_t)HstPtrBegin - (uint64_t)HstPtrBase;
435-
void *TgtPtrBase = (void *)((uint64_t)TgtPtrBegin - Delta);
440+
void *&TgtPtrBase = AsyncInfo.getVoidPtrLocation();
441+
TgtPtrBase = (void *)((uint64_t)TgtPtrBegin - Delta);
436442
int rt = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
437443
sizeof(void *), AsyncInfo);
438444
if (rt != OFFLOAD_SUCCESS) {
@@ -1123,8 +1129,9 @@ static int processDataBefore(ident_t *loc, int64_t DeviceId, void *HostPtr,
11231129
DP("Parent lambda base " DPxMOD "\n", DPxPTR(TgtPtrBase));
11241130
uint64_t Delta = (uint64_t)HstPtrBegin - (uint64_t)HstPtrBase;
11251131
void *TgtPtrBegin = (void *)((uintptr_t)TgtPtrBase + Delta);
1126-
void *PointerTgtPtrBegin = Device.getTgtPtrBegin(
1127-
HstPtrVal, ArgSizes[I], IsLast, false, IsHostPtr);
1132+
void *&PointerTgtPtrBegin = AsyncInfo.getVoidPtrLocation();
1133+
PointerTgtPtrBegin = Device.getTgtPtrBegin(HstPtrVal, ArgSizes[I],
1134+
IsLast, false, IsHostPtr);
11281135
if (!PointerTgtPtrBegin) {
11291136
DP("No lambda captured variable mapped (" DPxMOD ") - ignored\n",
11301137
DPxPTR(HstPtrVal));

0 commit comments

Comments
 (0)