[Openmp-commits] [PATCH] D89898: changing OMP rtl to use shared memory instead of env variable

[Roger Ferrer Ibanez via Phabricator via Openmp-commits]

rogfer01 added a comment.

Hi Todd,

> 1. I will change the name of the shared memory file to include UID at the end.  This way it will only conflict with users of the same UID which will reduce the likelihood of this happening because you will no longer collide with other UID.  this also implies that if a run changes UID the openMP runtime library will no longer recognize that a runtime is running.

I think this trade-off is acceptable as I believe changing the uid is not a usual thing that OpenMP applications have to do.

> 2. I will put a call into unregister the library in z_Linux.utls.cpp at about line 1153 (in the case statement for SIGTERM).  This should call __kmp_unregister_library whenever runtime gets SIGTERM, SIGQUIT, SIGSYS, SIGSEGV etc (see list in file).   This may remove some of the leftover files when a run is terminated.
>
> As Andrey said, user should set KMP_DUPLICATE_LIB_OK=1 if they want to allow the runtime to run when an existing file with the same name is outstanding.   If that env variable is not set runtime will still exit out.

I think this would be only necessary if we can't tolerate false positives at all

A false positive should require (if I read the code correctly) the three following circumstances: 1) we hit the same shm file (left by an earlier process that ended abnormally) 2) the address read from the shm is mapped in the process (ASLR mechanisms which are commonly enabled might reduce the likelihood of this) 3) we believe a runtime is running (when there is none) because the memory contains the value that means "a runtime is running".

I don't have actual evidence on this, but I believe this is rather unlikely.

> I will make these changes today in my local area and run some regressions and confirm it doesn't have any unintended consequences.

Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89898