Skip to content

[SYCL][Coverity] Fix MISSING_LOCK coverity hits in the program manager #18121

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

Merged
merged 6 commits into from
Apr 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 39 additions & 29 deletions sycl/source/detail/program_manager/program_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -756,35 +756,42 @@ ProgramManager::collectDependentDeviceImagesForVirtualFunctions(
}
}

while (!WorkList.empty()) {
std::string SetName = WorkList.front();
WorkList.pop();
if (!WorkList.empty()) {
// Guard read access to m_VFSet2BinImage:
// TODO: a better solution should be sought in the future, i.e. a different
// mutex than m_KernelIDsMutex, check lock check pattern, etc.
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);

// There could be more than one device image that uses the same set
// of virtual functions, or provides virtual funtions from the same
// set.
for (RTDeviceBinaryImage *BinImage : m_VFSet2BinImage[SetName]) {
// Here we can encounter both uses-virtual-functions-set and
// virtual-functions-set properties, but their handling is the same: we
// just grab all sets they reference and add them for consideration if
// we haven't done so already.
for (const sycl_device_binary_property &VFProp :
BinImage->getVirtualFunctions()) {
std::string StrValue = DeviceBinaryProperty(VFProp).asCString();
for (const auto &SetName : detail::split_string(StrValue, ',')) {
if (HandledSets.insert(SetName).second)
WorkList.push(SetName);
while (!WorkList.empty()) {
std::string SetName = WorkList.front();
WorkList.pop();

// There could be more than one device image that uses the same set
// of virtual functions, or provides virtual funtions from the same
// set.
for (RTDeviceBinaryImage *BinImage : m_VFSet2BinImage.at(SetName)) {
// Here we can encounter both uses-virtual-functions-set and
// virtual-functions-set properties, but their handling is the same: we
// just grab all sets they reference and add them for consideration if
// we haven't done so already.
for (const sycl_device_binary_property &VFProp :
BinImage->getVirtualFunctions()) {
std::string StrValue = DeviceBinaryProperty(VFProp).asCString();
for (const auto &SetName : detail::split_string(StrValue, ',')) {
if (HandledSets.insert(SetName).second)
WorkList.push(SetName);
}
}
}

// TODO: Complete this part about handling of incompatible device images.
// If device image uses the same virtual function set, then we only
// link it if it is compatible.
// However, if device image provides virtual function set and it is
// incompatible, then we should link its "dummy" version to avoid link
// errors about unresolved external symbols.
if (doesDevSupportDeviceRequirements(Dev, *BinImage))
DeviceImagesToLink.insert(BinImage);
// TODO: Complete this part about handling of incompatible device
// images. If device image uses the same virtual function set, then we
// only link it if it is compatible. However, if device image provides
// virtual function set and it is incompatible, then we should link its
// "dummy" version to avoid link errors about unresolved external
// symbols.
if (doesDevSupportDeviceRequirements(Dev, *BinImage))
DeviceImagesToLink.insert(BinImage);
}
}
}

Expand Down Expand Up @@ -2146,8 +2153,14 @@ void ProgramManager::addImages(sycl_device_binaries DeviceBinary) {
}

void ProgramManager::removeImages(sycl_device_binaries DeviceBinary) {
if (DeviceBinary->NumDeviceBinaries == 0)
return;
// Acquire lock to read and modify maps for kernel bundles
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);

for (int I = 0; I < DeviceBinary->NumDeviceBinaries; I++) {
sycl_device_binary RawImg = &(DeviceBinary->DeviceBinaries[I]);

auto DevImgIt = m_DeviceImages.find(RawImg);
if (DevImgIt == m_DeviceImages.end())
continue;
Expand All @@ -2161,9 +2174,6 @@ void ProgramManager::removeImages(sycl_device_binaries DeviceBinary) {
// Drop the kernel argument mask map
m_EliminatedKernelArgMasks.erase(Img);

// Acquire lock to modify maps for kernel bundles
std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);

// Unmap the unique kernel IDs for the offload entries
for (sycl_offload_entry EntriesIt = EntriesB; EntriesIt != EntriesE;
EntriesIt = EntriesIt->Increment()) {
Expand Down
2 changes: 2 additions & 0 deletions sycl/source/detail/program_manager/program_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@ class ProgramManager {

/// Keeps all device images we are refering to during program lifetime. Used
/// for proper cleanup.
/// Access must be guarded by the m_KernelIDsMutex mutex.
std::unordered_map<sycl_device_binary, RTDeviceBinaryImageUPtr>
m_DeviceImages;

Expand All @@ -469,6 +470,7 @@ class ProgramManager {

/// Caches list of device images that use or provide virtual functions from
/// the same set. Used to simplify access.
/// Access must be guarded by the m_KernelIDsMutex mutex.
std::unordered_map<std::string, std::set<RTDeviceBinaryImage *>>
m_VFSet2BinImage;

Expand Down