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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

In D130714#3685453 <https://reviews.llvm.org/D130714#3685453>, @tianshilei1992 wrote:

> 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.

Sometimes. It's a property of the system it runs on at present.

In D130714#3685453 <https://reviews.llvm.org/D130714#3685453>, @tianshilei1992 wrote:

> In D130714#3685431 <https://reviews.llvm.org/D130714#3685431>, @JonChesterfield wrote:
>
>> 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`?

No. It's allocated by new and freed with delete.

>> 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.

Can't be done. Depends on compiler and libc. As implemented this will work for any plugin that doesn't depend on undefined behaviour in its global constructor/destructor order, but those plugins are of course broken at present. This patch leaves nvptx unchanged but I'd really like to fix amdgpu for the 15 branch whatever we do with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130714