Unix: Avoid concurrency hazard in signal handler registration

[MarijnS95]

Fixes #5271
backported from llvm-mirror/llvm@fa92ec8

Note that this doesn't touch a similar possible race condition for UnregisterHandlers().

---

Several static functions from the signal API can be invoked simultaneously; RemoveFileOnSignal for instance can be called indirectly by multiple parallel loadModule() invocations, which might lead to the assertion:

Assertion failed: (NumRegisteredSignals < array_lengthof(RegisteredSignalInfo) && "Out of space for signal handlers!"),
function RegisterHandler, file /llvm/lib/Support/Unix/Signals.inc, line 105.

RemoveFileOnSignal calls RegisterHandlers(), which isn't currently mutex protected, leading to the behavior above. This potentially affect a few other users of RegisterHandlers() too.

rdar://problem/30381224

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@298871 91177308-0d34-0410-b5e6-96231b3b80d8

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3315 completed (commit 55bf747c85 by @bcardosolopes)

[llvm-beanz]

LGTM

[MarijnS95]

Thanks @llvm-beanz! I was about to ping this in a few minutes as we've been looking forward to integrating the latest DXC with this fix hopefully addressing this issue, but would also like to discuss enabling asserts more broadly: what is your thought on that? Should I create a new discussion item for it?

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3350 completed (commit 8e90a7ef63 by @bcardosolopes)

[llvm-beanz]

like to discuss enabling asserts more broadly: what is your thought on that? Should I create a new discussion item for it?

I think it kinda depends what you mean. DXC converts some asserts to exceptions (which I hate), and other asserts get compiled out in Release builds, but we have a CMake option to enable them.

We don't really use GitHub discussions. Filing an issue might be the best forum for a discussion.

[MarijnS95]

Meant an issue (separate from this PR), discussion boards are disabled for this project.

and other asserts get compiled out in Release builds

Simply said I think it's worthwhile to have those retained, by changing the default of this CMake option to always be enabled.

[llvm-beanz]

Simply said I think it's worthwhile to have those retained, by changing the default of this CMake option to always be enabled.

The 30-second answer to this is no, we won't do that. Enabling asserts has a significant negative impact on performance for LLVM because it enables a bunch of validation checks that can be expensive.

[MarijnS95]

Okay. Will see how much it impacts our build times as we have quite a few shaders, and maybe compile the library with that enabled for our own consumption: we've ran into far too many bugs and issues to not have asserts.