Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misuse of libomptarget plugin interface #48551

Closed
shiltian opened this issue Feb 16, 2021 · 5 comments
Closed

Misuse of libomptarget plugin interface #48551

shiltian opened this issue Feb 16, 2021 · 5 comments
Labels
bugzilla Issues migrated from bugzilla openmp

Comments

@shiltian
Copy link
Contributor

Bugzilla Link 49207
Resolution FIXED
Resolved on Feb 19, 2021 22:28
Version unspecified
OS All
Blocks #48246
CC @jdoerfert,@tstellar
Fixed by commit(s) 2518cc6 76d5d54

Extended Description

This bug was reported by Guilherme Valarini via Openmp-dev (openmp-dev@lists.llvm.org). The link is https://lists.llvm.org/pipermail/openmp-dev/2021-February/003867.html

The following is the bug description.

In both cases, the address to a local variable is passed as the host data to be copied to the target device, but since this function can be asynchronously executed, such variables can get out of scope when the actual submission is done.

// File: openmp/libomptarget/src/omptarget.cpp:419
if (arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ && !IsHostPtr) {
  DP("Update pointer (" DPxMOD ") -> [" DPxMOD "]\n",
      DPxPTR(PointerTgtPtrBegin), DPxPTR(TgtPtrBegin));
  uint64_t Delta = (uint64_t)HstPtrBegin - (uint64_t)HstPtrBase;
  void *TgtPtrBase = (void *)((uint64_t)TgtPtrBegin - Delta);
  int rt = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
                              sizeof(void *), async_info_ptr);
  if (rt != OFFLOAD_SUCCESS) {
    REPORT("Copying data to device failed.\n");
    return OFFLOAD_FAIL;
  }
  // create shadow pointers for this entry
  Device.ShadowMtx.lock();
  Device.ShadowPtrMap[Pointer_HstPtrBegin] = {
      HstPtrBase, PointerTgtPtrBegin, TgtPtrBase};
  Device.ShadowMtx.unlock();
}
// File: openmp/libomptarget/src/omptarget.cpp:1141
DP("Parent lambda base " DPxMOD "\n", DPxPTR(TgtPtrBase));
uint64_t Delta = (uint64_t)HstPtrBegin - (uint64_t)HstPtrBase;
void *TgtPtrBegin = (void *)((uintptr_t)TgtPtrBase + Delta);
void *PointerTgtPtrBegin = Device.getTgtPtrBegin(
    HstPtrVal, ArgSizes[I], IsLast, false, IsHostPtr);
if (!PointerTgtPtrBegin) {
  DP("No lambda captured variable mapped (" DPxMOD ") - ignored\n",
      DPxPTR(HstPtrVal));
  continue;
}
if (PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
    TgtPtrBegin == HstPtrBegin) {
  DP("Unified memory is active, no need to map lambda captured"
      "variable (" DPxMOD ")\n",
      DPxPTR(HstPtrVal));
  continue;
}
DP("Update lambda reference (" DPxMOD ") -> [" DPxMOD "]\n",
    DPxPTR(PointerTgtPtrBegin), DPxPTR(TgtPtrBegin));
Ret = Device.submitData(TgtPtrBegin, &PointerTgtPtrBegin,
                        sizeof(void *), AsyncInfo);
if (Ret != OFFLOAD_SUCCESS) {
  REPORT("Copying data to device failed.\n");
  return OFFLOAD_FAIL;
}
@shiltian
Copy link
Contributor Author

Johannes proposed a fix in https://reviews.llvm.org/D96667.

@jdoerfert
Copy link
Member

fix for release branch

@jdoerfert
Copy link
Member

I'll push the fix into main today, the release branch version is attached as patch file.

@jdoerfert
Copy link
Member

@​tstellar Can you apply the patch file to fix this in 12?

@tstellar
Copy link
Collaborator

Merged: 76d5d54

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla openmp
Projects
None yet
Development

No branches or pull requests

3 participants