[OpenMP][libomptarget] Fix deinit of NextGen AMDGPU plugin

[kevinsala]

Summary

This patch fixes a segfault that was appearing when the plugin fails to initialize and then is deinitialized. Also, do not call hsa_shut_down if the hsa_init failed.

[jhuber6]

Another solution might be to call any random HSA function and see if it returns HSA_STATUS_ERROR_NOT_INITIALIZED. Not sure if that would save us much.

[kevinsala]

In D142145#4066648, @jhuber6 wrote:

Another solution might be to call any random HSA function and see if it returns HSA_STATUS_ERROR_NOT_INITIALIZED. Not sure if that would save us much.

Would that work when dlopening HSA library with dynamic_hsa and no library is found?

[jhuber6]

In D142145#4066724, @kevinsala wrote:

In D142145#4066648, @jhuber6 wrote:

Another solution might be to call any random HSA function and see if it returns HSA_STATUS_ERROR_NOT_INITIALIZED. Not sure if that would save us much.

Would that work when dlopening HSA library with dynamic_hsa and no library is found?

I figured we'd just return an HSA_STATUS_ERROR immediately. But I guess in that case we'd still call the destructor?

[kevinsala]

In D142145#4066727, @jhuber6 wrote:

In D142145#4066724, @kevinsala wrote:

In D142145#4066648, @jhuber6 wrote:

Another solution might be to call any random HSA function and see if it returns HSA_STATUS_ERROR_NOT_INITIALIZED. Not sure if that would save us much.

Would that work when dlopening HSA library with dynamic_hsa and no library is found?

I figured we'd just return an HSA_STATUS_ERROR immediately. But I guess in that case we'd still call the destructor?

In that case, where hsa_init returns HSA_STATUS_ERROR because there is no HSA library found, the plugin object (AMDGPUPluginTy) is kept alive. We set the number of its devices to zero but there are no further initializations. We then have to handle the destructor.

[jhuber6]

This is fine overall, but fewer random globals lying around it always good. I don't know if it's possible to do something like if (some_random_hsa_call && some_random_hsa_call() != HSA_STATUS_NOT_INITIALIZED) since it default value of the pointer might not be null.

[jhuber6]

In D142145#4066770, @jhuber6 wrote:

This is fine overall, but fewer random globals lying around it always good. I don't know if it's possible to do something like if (some_random_hsa_call && some_random_hsa_call() != HSA_STATUS_NOT_INITIALIZED) since it default value of the pointer might not be null.

Double checked, global variables should be initialized to zero in C++ so you can probably just check if the function pointer is non-null to see if HSA was dynamically loaded.

[kevinsala]

In D142145#4066770, @jhuber6 wrote:

This is fine overall, but fewer random globals lying around it always good. I don't know if it's possible to do something like if (some_random_hsa_call && some_random_hsa_call() != HSA_STATUS_NOT_INITIALIZED) since it default value of the pointer might not be null.

fewer random globals lying around it always good

Not sure if you refer to the Initialized variable. In this case it's not a global, it's a data member of the AMDGPUPluginTy class.

[jhuber6]

In D142145#4066841, @kevinsala wrote:

Not sure if you refer to the Initialized variable. In this case it's not a global, it's a data member of the AMDGPUPluginTy class.

I mean when we link against HSA we have something like hsa_deinit() as a function with a valid address. When we load it dynamically it's just forward declared like HSA_STATUS_T (hsa_deinit())(void); which is initialized to zero.

[kevinsala]

In D142145#4066891, @jhuber6 wrote:

In D142145#4066841, @kevinsala wrote:

Not sure if you refer to the Initialized variable. In this case it's not a global, it's a data member of the AMDGPUPluginTy class.

I mean when we link against HSA we have something like hsa_deinit() as a function with a valid address. When we load it dynamically it's just forward declared like HSA_STATUS_T (hsa_deinit())(void); which is initialized to zero.

I'll update the patch with that change, thanks! We can avoid calling another HSA function and call directly hsa_shut_down ignoring the error code, right?

if (hsa_shut_down)
  hsa_shut_down();

[jhuber6]

In D142145#4067086, @kevinsala wrote:

In D142145#4066891, @jhuber6 wrote:

In D142145#4066841, @kevinsala wrote:

Not sure if you refer to the Initialized variable. In this case it's not a global, it's a data member of the AMDGPUPluginTy class.

I mean when we link against HSA we have something like hsa_deinit() as a function with a valid address. When we load it dynamically it's just forward declared like HSA_STATUS_T (hsa_deinit())(void); which is initialized to zero.

I'll update the patch with that change, thanks! We can avoid calling another HSA function and call directly hsa_shut_down ignoring the error code, right?

if (hsa_shut_down)
  hsa_shut_down();

That would handle the case where HSA wasn't dynamically loaded successfully. If hsa_init() was never called but HSA was successfully loaded / linked, you'd get that HSA_STATUS_ERROR_NOT_INITIALIZED code returned instead.

[kevinsala]

@jhuber6 I was testing the change but the compilation reports the following warning:

openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:2331:22: warning: the address of ‘hsa_status_t hsa_shut_down()’ will never be NULL [-Waddress]
     if (hsa_shut_down)

When we build with dynamic_hsa, hsa_shut_down (and the rest) appear as a defined function. Looking at nm libomptarget.rtl.amdgpu.nextgen.so, it appears the symbol with symbol type t, i.e. text (code) section. So it sounds this will never be null. I guess hsa_shut_down is a function defined in dynamic_hsa/hsa.cpp acting as a wrapper function.

[jhuber6]

In D142145#4067211, @kevinsala wrote:

@jhuber6 I was testing the change but the compilation reports the following warning:

openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:2331:22: warning: the address of ‘hsa_status_t hsa_shut_down()’ will never be NULL [-Waddress]
     if (hsa_shut_down)

When we build with dynamic_hsa, hsa_shut_down (and the rest) appear as a defined function. Looking at nm libomptarget.rtl.amdgpu.nextgen.so, it appears the symbol with symbol type t, i.e. text (code) section. So it sounds this will never be null. I guess hsa_shut_down is a function defined in dynamic_hsa/hsa.cpp acting as a wrapper function.

I see, most likely from how we do the DLWRAP stuff I'm guessing. It's not a huge deal either way. Go ahead and land this since it's not trivial to replace like that.

openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp

Error getGlobalMetadataFromELF(const DeviceImageTy &Image,
 
     return Plugin::success();
   }
 };
 
 /// Class implementing the AMDGPU-specific functionalities of the plugin.
 struct AMDGPUPluginTy final : public GenericPluginTy {
   /// Create an AMDGPU plugin and initialize the AMDGPU driver.
-  AMDGPUPluginTy() : GenericPluginTy(getTripleArch()), HostDevice(nullptr) {}
+  AMDGPUPluginTy()
+      : GenericPluginTy(getTripleArch()), Initialized(false),
+        HostDevice(nullptr) {}
 
   /// This class should not be copied.
   AMDGPUPluginTy(const AMDGPUPluginTy &) = delete;
   AMDGPUPluginTy(AMDGPUPluginTy &&) = delete;
 
   /// Initialize the plugin and return the number of devices.
   Expected<int32_t> initImpl() override {
     hsa_status_t Status = hsa_init();
     if (Status != HSA_STATUS_SUCCESS) {
       // Cannot call hsa_success_string.
-      DP("Failed initialize AMDGPU's HSA library\n");
+      DP("Failed to initialize AMDGPU's HSA library\n");
       return 0;
     }
 
+    // The initialization of HSA was successful. It should be safe to call
+    // HSA functions from now on, e.g., hsa_shut_down.
+    Initialized = true;
+
     // Register event handler to detect memory errors on the devices.
     Status = hsa_amd_register_system_event_handler(eventHandler, nullptr);
     if (auto Err = Plugin::check(
             Status, "Error in hsa_amd_register_system_event_handler: %s"))
       return std::move(Err);
 
     // List of host (CPU) agents.
     llvm::SmallVector<hsa_agent_t> HostAgents;
Expected<int32_t> initImpl() override {
     if (auto Err = HostDevice->init())
       return std::move(Err);
 
     return NumDevices;
   }
 
   /// Deinitialize the plugin.
   Error deinitImpl() override {
+    // The HSA runtime was not initialized, so nothing from the plugin was
+    // actually initialized.
+    if (!Initialized)
+      return Plugin::success();
+
+    if (HostDevice)
       if (auto Err = HostDevice->deinit())
         return Err;
 
     // Finalize the HSA runtime.
     hsa_status_t Status = hsa_shut_down();
     return Plugin::check(Status, "Error in hsa_shut_down: %s");
   }
 
   Triple::ArchType getTripleArch() const override { return Triple::amdgcn; }
 
FATAL_MESSAGE(1,
                   "Found HSA_AMD_GPU_MEMORY_FAULT_EVENT in agent %" PRIu64
                   " at virtual address %p and reasons: %s",
                   Event->memory_fault.agent.handle,
                   (void *)Event->memory_fault.virtual_address, Reasons.data());
 
     return HSA_STATUS_ERROR;
   }
 
+  /// Indicate whether the HSA runtime was correctly initialized. Even if there
+  /// is no available devices this boolean will be true. It indicates whether
+  /// we can safely call HSA functions (e.g., hsa_shut_down).
+  bool Initialized;
+
   /// Arrays of the available GPU and CPU agents. These arrays of handles should
   /// not be here but in the AMDGPUDeviceTy structures directly. However, the
   /// HSA standard does not provide API functions to retirve agents directly,
   /// only iterating functions. We cache the agents here for convenience.
   llvm::SmallVector<hsa_agent_t> KernelAgents;
 
   /// The device representing all HSA host agents.
   AMDHostDeviceTy *HostDevice;