[PATCH] D46858: Signal handling should be signal-safe

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

I wonder if there's some way to write a test for this.  E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.



================
Comment at: lib/Support/Unix/Signals.inc:90
+/// FileToRemoveList - Signal-safe removal of files.
+/// Adding and removing from the list isn't signal-safe, but removal itself is.
+/// Memory is freed when the head is freed, deletion is therefore not
----------------
You've said that removing is not signal-safe, but removal itself is; that's not computing for me.  It looks like `insert` and `unregister` are not signal-save, but `removeAll` is signal-safe -- should you use that terminology here?


================
Comment at: lib/Support/Unix/Signals.inc:109
+
+  static void Insert(std::atomic<FileToRemoveList *> &Head,
+                     const std::string &Filename) { // Not signal-safe.
----------------
I think current style guidelines for functions are lowerCamelCase, so this should be "insert".


================
Comment at: lib/Support/Unix/Signals.inc:121-122
 
-static ManagedStatic<std::vector<std::string>> FilesToRemove;
+  static void Unregister(std::atomic<FileToRemoveList *> &Head,
+                         const std::string &Filename) { // Not signal-safe.
+    // Technically this is signal-safe because we're lazy and aren't removing
----------------
"unregister" instead "Unregister"


================
Comment at: lib/Support/Unix/Signals.inc:133
+
+  static void RemoveAll(std::atomic<FileToRemoveList *> &Head) { // Signal-safe.
+    // If cleanup were to occur while we're removing files we'd have a bad time.
----------------
"removeAll" instead of "RemoveAll"


================
Comment at: lib/Support/Unix/Signals.inc:166
+
+/// FilesToRemoveCleanup - clean up the list in a signal-friendly manner.
+/// Recall that signals can fire during llvm_shutdown. If this occurs we should
----------------
Current style doesn't have us repeating the name of what's being documented.

If you're just matching the style in the file you can do a prep NFC commit that fixes the style elsewhere, but either way let's avoid the redundancy in the new code.


================
Comment at: lib/Support/Unix/Signals.inc:256
 
+  auto RegisterHandler = [&](int Signal) {
+    unsigned Index = NumRegisteredSignals.load();
----------------
Why did you lambda-fy this?  (If it's unrelated to the patch, can you split it out into a separate NFC commit?)

Also, I would prefer spelling this as `registerHandler` (lower camel case) since it's callable.


Repository:
  rL LLVM

https://reviews.llvm.org/D46858