[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/test/API/driver/job_control/TestJobControl.py:9
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.lldbpexpect import PExpectTest
----------------
Unused import?


================
Comment at: lldb/test/API/driver/job_control/TestJobControl.py:24
+        self.launch(run_under=run_under, post_spawn=post_spawn)
+        child = self.child
+
----------------
Am I correct that `self.launch` will wait until `post_spawn` finishes, meaning that self.child will always be set by this point?
(or `post_spawn` timed out and we don't get here anyway)

Also if the point is just to assert that self.child has been set, it would be a bit clearer to use an assert. Even if it is:
```
self.assertTrue(hasattr(self, "child"))
```
At least it looks like a deliberate check rather than a mistake if it fails. (I probably have the arguments the wrong way around, I always do somehow)


================
Comment at: lldb/test/API/driver/job_control/shell.py:10
+
+def preexec_fn():
+    orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU])
----------------
Please comment this function.

I'm not sure if this intends to put the new process into a new process group or the same process group as this script. (but mostly because I haven't used process groups before now)


================
Comment at: lldb/tools/driver/Driver.cpp:834
   signal(SIGTSTP, sigtstp_handler);
-  signal(SIGCONT, sigcont_handler);
 #endif
----------------
Is `sigcont_handler` now unused? It only references itself and is registered here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320