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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath 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
----------------
DavidSpickett wrote:
> Unused import?
Yes, so are `lldb` and `decorator` imports. :)


================
Comment at: lldb/test/API/driver/job_control/TestJobControl.py:24
+        self.launch(run_under=run_under, post_spawn=post_spawn)
+        child = self.child
+
----------------
DavidSpickett wrote:
> 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)
The point of this was that I copy-pasted this code from some other test, which wanted to save some characters by typing `child` instead of `self.child`. However, I did not make use of that variable in any new code I actually wrote. :)

I'll just delete this line.


================
Comment at: lldb/test/API/driver/job_control/shell.py:10
+
+def preexec_fn():
+    orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU])
----------------
DavidSpickett wrote:
> 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)
It creates a new process group.

The way that process groups (and sessions), terminals and signals interact is both awesome and horrifying at the same time. One has to admire the ingenuity of the job control implementation, which completely avoids threads and waits in the shell. But otoh, we're talking about signals, so chances are any random implementation will have a bug/race somewhere, particularly if the app is multi-threaded.


================
Comment at: lldb/tools/driver/Driver.cpp:834
   signal(SIGTSTP, sigtstp_handler);
-  signal(SIGCONT, sigcont_handler);
 #endif
----------------
DavidSpickett wrote:
> Is `sigcont_handler` now unused? It only references itself and is registered here.
The patch actually does delete the function. It's just that the phabricator formatting makes that unobvious, since the patch essentially merges the two functions into one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320