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

This PR guards accesses to `m_DeviceImage` and `m_VFSet2BinImage`
caches/maps in the Program Manager using `m_KernelIDsMutex` in
situations where multiple threads may be accessing the maps at the same
time.

I am opting to use `m_KernelIDsMutex` here as both maps are commonly
accessed while holding said mutex: In fact, a lot of maps/caches in the
program manager use `m_KernelIDsMutex` (This is not the most optimal
solution, but that is out of the scope of this PR, see #18165)

This PR addresses Coverity hits that needs to have fixes submitted
before the code cutoff.

---------

Co-authored-by: Steffen Larsen <[email protected]>

Author: ianayl

sycl/source/detail/program_manager/program_manager.cpp

@@ -765,35 +765,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);
+      }
     }
   }
 
@@ -2163,8 +2170,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;
@@ -2178,9 +2191,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()) {

sycl/source/detail/program_manager/program_manager.hpp

@@ -459,6 +459,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;
 
@@ -468,6 +469,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;