[Openmp-commits] [PATCH] D130714: [openmp][amdgpu] Tear down amdgpu plugin accurately

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added a comment.

In D130714#3685431 <https://reviews.llvm.org/D130714#3685431>, @JonChesterfield wrote:

>> Any call from d'tor of libomptarget to plugins will lead to UB
>
> This is untrue in general. If the plugin global destructor has executed before the libomptarget destructor and left the plugin in a state where calls to it will segfault then yes, it will segfault.

That is exactly the case.

> This patch changes the amdgpu plugin to have well defined lifetime between calls to plugin_init and plugin_deinit, and puts the call to plugin_deinit in the libomptarget destructor. That strictly nests the lifetime of the plugin within that of libomptarget while arranging for no calls to follow the one to plugin_deinit. That is the only nesting that can be correct.

Of course you can limit the lifetime between calls to plugin_init and plugin_deinit, but the key point is when they are called.

> However the order of global constructors / destructors within the plugin relative to those of other shared objects is not especially likely to respect this definition, so the fix there is to not have global constructor / destructors within the plugin. Strictly speaking there's still a statically allocated 8 byte pointer here, but as the destructor for that emits no code we're fine.

Is it possible the buffer where `DeviceInfoState` points is freed implicitly before `deinit_plugin`?

> Finally this patch makes no difference to nvptx whatsoever as it hasn't implemented init_plugin or deinit_plugin, so nvptx maintains the current roll the die and hope semantics. It can opt into well defined ones, like this patch does to amdgpu, whenever we'd like to stop it unexpectedly segfaulting.

However, it changes the behavior of `libomptarget`, even though for now others don't implement the functions. It opens the door to mystery for others.

I'll be generally okay if we document all details in `__attribute__((destructor(101))) void deinit()` before we call `R->deinit_plugin`, such as in what scenario this could work, and in what cases it may not work as expected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130714/new/

https://reviews.llvm.org/D130714