[lldb/Test] Always set the cleanupSubprocesses tear down hook

[JDevlieghere]

Summary

Always set the cleanupSubprocesses tear down hook when using spawnSubprocess or forkSubprocess instead of relying on the caller to do so. This is not only less error prone but also means the tests can be more concise.

[teemperor]

LGTM

[labath]

I suppose this might have been because someone wanted to ensure cleanupSubprocesses is not called twice (if multiple subprocesses are spawned). It looks like it should be safe to do so, though it's not completely ideal. TBH, I'm not sure why this needs to be a tear down "hook". I t seems like this could just be a separate step in the normal tearDown function...

[shafik]

Curious, why didn't we do it this way before?

[JDevlieghere]

In D83787#2150926, @labath wrote:

I suppose this might have been because someone wanted to ensure cleanupSubprocesses is not called twice (if multiple subprocesses are spawned). It looks like it should be safe to do so, though it's not completely ideal. TBH, I'm not sure why this needs to be a tear down "hook". I t seems like this could just be a separate step in the normal tearDown function...

Makes sense, I'll move it in the regular teardown.

lldb/packages/Python/lldbsuite/test/lldbtest.py

def cleanupSubprocesses(self):
             try:
                 os.kill(pid, signal.SIGTERM)
             except OSError:
                 pass
 
     def spawnSubprocess(self, executable, args=[], install_remote=True):
         """ Creates a subprocess.Popen object with the specified executable and arguments,
             saves it in self.subprocesses, and returns the object.
-            NOTE: if using this function, ensure you also call:
-
-              self.addTearDownHook(self.cleanupSubprocesses)
-
-            otherwise the test suite will leak processes.
         """
         proc = _RemoteProcess(
             install_remote) if lldb.remote_platform else _LocalProcess(self.TraceOn())
         proc.launch(executable, args)
         self.subprocesses.append(proc)
+        self.addTearDownHook(self.cleanupSubprocesses)
         return proc
 
     def forkSubprocess(self, executable, args=[]):
         """ Fork a subprocess with its own group ID.
-            NOTE: if using this function, ensure you also call:
-
-              self.addTearDownHook(self.cleanupSubprocesses)
-
-            otherwise the test suite will leak processes.
         """
         child_pid = os.fork()
         if child_pid == 0:
             # If more I/O support is required, this can be beefed up.
             fd = os.open(os.devnull, os.O_RDWR)
             os.dup2(fd, 1)
             os.dup2(fd, 2)
             # This call causes the child to have its of group ID
             os.setpgid(0, 0)
             os.execvp(executable, [executable] + args)
         # Give the child time to get through the execvp() call
         time.sleep(0.1)
         self.forkedProcessPids.append(child_pid)
+        self.addTearDownHook(self.cleanupSubprocesses)
         return child_pid
 
     def HideStdout(self):
         """Hide output to stdout from the user.
 
         During test execution, there might be cases where we don't want to show the
         standard output to the user.  For example,
 

lldb/test/API/commands/platform/process/list/TestProcessList.py

class ProcessListTestCase(TestBase):
     @skipIfWindows  # https://bugs.llvm.org/show_bug.cgi?id=43702
     def test_process_list_with_args(self):
         """Test process list show process args"""
         self.build()
         exe = self.getBuildArtifact("TestProcess")
 
         # Spawn a new process
         popen = self.spawnSubprocess(exe, args=["arg1", "--arg2", "arg3"])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         substrs = [str(popen.pid), "TestProcess arg1 --arg2 arg3"]
 
         # Because LLDB isn't the one spawning the subprocess, the PID will be
         # different during replay.
         if configuration.is_reproducer_replay():
             substrs.pop(0)
 
         self.expect("platform process list -v", substrs=substrs)

lldb/test/API/commands/process/attach-resume/TestAttachResume.py

def test_attach_continue_interrupt_detach(self):
         self.process_attach_continue_interrupt_detach()
 
     def process_attach_continue_interrupt_detach(self):
         """Test attach/continue/interrupt/detach"""
 
         exe = self.getBuildArtifact(exe_name)
 
         popen = self.spawnSubprocess(exe)
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         self.runCmd("process attach -p " + str(popen.pid))
 
         self.setAsync(True)
         listener = self.dbg.GetListener()
         process = self.dbg.GetSelectedTarget().GetProcess()
 
         self.runCmd("c")

lldb/test/API/commands/process/attach/TestProcessAttach.py

class ProcessAttachTestCase(TestBase):
     @expectedFailureNetBSD
     def test_attach_to_process_by_id(self):
         """Test attach by process id"""
         self.build()
         exe = self.getBuildArtifact(exe_name)
 
         # Spawn a new process
         popen = self.spawnSubprocess(exe)
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         self.runCmd("process attach -p " + str(popen.pid))
 
         target = self.dbg.GetSelectedTarget()
 
         process = target.GetProcess()
         self.assertTrue(process, PROCESS_IS_VALID)
 
def test_attach_to_process_from_different_dir_by_id(self):
                 raise
         testdir = self.getBuildDir()
         exe = os.path.join(newdir, 'proc_attach')
         self.buildProgram('main.cpp', exe)
         self.addTearDownHook(lambda: shutil.rmtree(newdir))
 
         # Spawn a new process
         popen = self.spawnSubprocess(exe)
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         os.chdir(newdir)
         self.addTearDownHook(lambda: os.chdir(testdir))
         self.runCmd("process attach -p " + str(popen.pid))
 
         target = self.dbg.GetSelectedTarget()
 
         process = target.GetProcess()
         self.assertTrue(process, PROCESS_IS_VALID)
 
     @expectedFailureNetBSD
     def test_attach_to_process_by_name(self):
         """Test attach by process name"""
         self.build()
         exe = self.getBuildArtifact(exe_name)
 
         # Spawn a new process
         popen = self.spawnSubprocess(exe)
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         self.runCmd("process attach -n " + exe_name)
 
         target = self.dbg.GetSelectedTarget()
 
         process = target.GetProcess()
         self.assertTrue(process, PROCESS_IS_VALID)
 
     def tearDown(self):
         # Destroy process before TestBase.tearDown()
         self.dbg.GetSelectedTarget().GetProcess().Destroy()
 
         # Call super's tearDown().
         TestBase.tearDown(self)

lldb/test/API/commands/process/attach/attach_denied/TestAttachDenied.py

def test_attach_to_process_by_id_denied(self):
             "pid_file_%d" % (int(time.time())))
         self.addTearDownHook(
             lambda: self.run_platform_command(
                 "rm %s" %
                 (pid_file_path)))
 
         # Spawn a new process
         popen = self.spawnSubprocess(exe, [pid_file_path])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         pid = lldbutil.wait_for_file_on_target(self, pid_file_path)
 
         self.expect('process attach -p ' + pid,
                     startstr='error: attach failed:',
                     error=True)

lldb/test/API/commands/register/register/register_command/TestRegisters.py

def convenience_registers(self):
                     substrs=['0x1234567887654321'])
 
     def convenience_registers_with_process_attach(self, test_16bit_regs):
         """Test convenience registers after a 'process attach'."""
         exe = self.getBuildArtifact("a.out")
 
         # Spawn a new process
         pid = self.spawnSubprocess(exe, ['wait_for_attach']).pid
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         if self.TraceOn():
             print("pid of spawned process: %d" % pid)
 
         self.runCmd("process attach -p %d" % pid)
 
         # Check that "register read eax" works.
         self.runCmd("register read eax")

lldb/test/API/commands/target/auto-install-main-executable/TestAutoInstallMainExecutable.py

def test_target_auto_install_main_executable(self):
             listen_url,
             "--server"
             ]
 
         self.spawnSubprocess(
             self.debug_monitor_exe,
             commandline_args,
             install_remote=False)
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         # Wait for the new process gets ready.
         time.sleep(0.1)
 
         self.dbg.SetAsync(False)
 
         new_platform = lldb.SBPlatform(lldb.remote_platform.GetName())
         self.dbg.SetSelectedPlatform(new_platform)

lldb/test/API/functionalities/deleted-executable/TestDeletedExecutable.py

def test(self):
             "token_pid_%d" % (int(os.getpid())))
         self.addTearDownHook(
             lambda: self.run_platform_command(
                 "rm %s" %
                 (pid_file_path)))
 
         # Spawn a new process
         popen = self.spawnSubprocess(exe, [pid_file_path])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         # Wait until process has fully started up.
         pid = lldbutil.wait_for_file_on_target(self, pid_file_path)
 
         # Now we can safely remove the executable and test if we can attach.
         os.remove(exe)
 
         self.runCmd("process attach -p " + str(popen.pid))
         self.runCmd("kill")

lldb/test/API/functionalities/process_group/TestChangeProcessGroup.py

def test_setpgid(self):
         pid_file_path = lldbutil.append_to_process_working_directory(self,
             "pid_file_%d" % (int(time.time())))
         self.addTearDownHook(
             lambda: self.run_platform_command(
                 "rm %s" %
                 (pid_file_path)))
 
         popen = self.spawnSubprocess(exe, [pid_file_path])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         pid = lldbutil.wait_for_file_on_target(self, pid_file_path)
 
         # make sure we cleanup the forked child also
         def cleanupChild():
             if lldb.remote_platform:
                 lldb.remote_platform.Kill(int(pid))
             else:

lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py

def test_reproducer_attach(self):
         reproducer = self.getBuildArtifact(exe + '.reproducer')
         if os.path.exists(reproducer):
             try:
                 shutil.rmtree(reproducer)
             except OSError:
                 pass
 
         self.build(dictionary={'EXE': exe})
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         inferior = self.spawnSubprocess(self.getBuildArtifact(exe), [token])
         pid = inferior.pid
 
         lldbutil.wait_for_file_on_target(self, token)
 
         # Use Popen because pexpect is overkill and spawnSubprocess is
         # asynchronous.

lldb/test/API/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

def create_after_attach(self, use_fork):
         exe = self.getBuildArtifact("a.out")
 
         # Spawn a new process
         if use_fork:
             pid = self.forkSubprocess(exe)
         else:
             popen = self.spawnSubprocess(exe)
             pid = popen.pid
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         # Attach to the spawned process
         self.runCmd("process attach -p " + str(pid))
 
         target = self.dbg.GetSelectedTarget()
 
         process = target.GetProcess()
         self.assertTrue(process, PROCESS_IS_VALID)

lldb/test/API/macosx/find-dsym/bundle-with-dot-in-filename/TestBundleWithDotInFilename.py

class BundleWithDotInFilenameTestCase(TestBase):
     # This test is explicitly a dSYM test, it doesn't need to run for any other config.
     @skipIf(debug_info=no_match(["dsym"]))
     def test_attach_and_check_dsyms(self):
         """Test attach to binary, see if the bundle dSYM is found"""
         exe = self.getBuildArtifact(exe_name)
         self.build()
         os.chdir(self.getBuildDir());
         popen = self.spawnSubprocess(exe)
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         # Give the inferior time to start up, dlopen a bundle, remove the bundle it linked in
         sleep(5)
 
         # Since the library that was dlopen()'ed is now removed, lldb will need to find the
         # binary & dSYM via target.exec-search-paths
         settings_str = "settings set target.exec-search-paths " + self.get_process_working_directory() + "/hide.app"
         self.runCmd(settings_str)

lldb/test/API/macosx/find-dsym/deep-bundle/TestDeepBundle.py

class DeepBundleTestCase(TestBase):
     @skipUnlessDarwin
     # This test is explicitly a dSYM test, it doesn't need to run for any other config.
     @skipIf(debug_info=no_match(["dsym"]))
     def test_attach_and_check_dsyms(self):
         """Test attach to binary, see if the framework dSYM is found"""
         exe = self.getBuildArtifact(exe_name)
         self.build()
         popen = self.spawnSubprocess(exe, [self.getBuildDir()])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         # Give the inferior time to start up, dlopen a bundle, remove the bundle it linked in
         sleep(5)
 
         # Since the library that was dlopen()'ed is now removed, lldb will need to find the
         # binary & dSYM via target.exec-search-paths
         settings_str = "settings set target.exec-search-paths " + self.get_process_working_directory() + "/hide.app"
         self.runCmd(settings_str)

lldb/test/API/macosx/function-starts/TestFunctionStarts.py

def do_function_starts(self, in_memory):
         pid_file_path = lldbutil.append_to_process_working_directory(self,
             "token_pid_%d" % (int(os.getpid())))
         self.addTearDownHook(
             lambda: self.run_platform_command(
                 "rm %s" %
                 (pid_file_path)))
 
         popen = self.spawnSubprocess(exe, [pid_file_path])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         # Wait until process has fully started up.
         pid = lldbutil.wait_for_file_on_target(self, pid_file_path)
 
         if in_memory:
           remove_file(exe)
 
         target = self.dbg.CreateTarget(None)

lldb/test/API/macosx/universal/TestUniversal.py

def test_process_attach_with_wrong_arch(self):
         bkpt = target.BreakpointCreateBySourceRegex(
             "sleep", lldb.SBFileSpec("main.c"))
         self.assertTrue(bkpt.IsValid(), "Valid breakpoint")
         self.assertTrue(
             bkpt.GetNumLocations() >= 1,
             "Our main breakpoint has locations.")
 
         popen = self.spawnSubprocess(exe, ["keep_waiting"])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         error = lldb.SBError()
         empty_listener = lldb.SBListener()
         process = target.AttachToProcessWithID(
             empty_listener, popen.pid, error)
         self.assertTrue(error.Success(), "Attached to process.")
 
         self.expect("image list -A -b", substrs=["x86_64h testit"])

lldb/test/API/python_api/hello_world/TestHelloWorld.py

def test_with_attach_to_process_with_id_api(self):
 
         # Spawn a new process
         token = exe+'.token'
         if not lldb.remote_platform:
             token = self.getBuildArtifact(token)
             if os.path.exists(token):
                 os.remove(token)
         popen = self.spawnSubprocess(self.getBuildArtifact(exe), [token])
-        self.addTearDownHook(self.cleanupSubprocesses)
         lldbutil.wait_for_file_on_target(self, token)
 
         listener = lldb.SBListener("my.attach.listener")
         error = lldb.SBError()
         process = target.AttachToProcessWithID(listener, popen.pid, error)
 
         self.assertTrue(error.Success() and process, PROCESS_IS_VALID)
 
def test_with_attach_to_process_with_name_api(self):
 
         # Spawn a new process.
         token = exe+'.token'
         if not lldb.remote_platform:
             token = self.getBuildArtifact(token)
             if os.path.exists(token):
                 os.remove(token)
         popen = self.spawnSubprocess(self.getBuildArtifact(exe), [token])
-        self.addTearDownHook(self.cleanupSubprocesses)
         lldbutil.wait_for_file_on_target(self, token)
 
         listener = lldb.SBListener("my.attach.listener")
         error = lldb.SBError()
         # Pass 'False' since we don't want to wait for new instance of
         # "hello_world" to be launched.
         name = os.path.basename(exe)
 

lldb/test/API/tools/lldb-server/platform-process-connect/TestPlatformProcessConnect.py

def test_platform_process_connect(self):
             "--",
             "%s/a.out" %
             working_dir,
             "foo"]
         self.spawnSubprocess(
             self.debug_monitor_exe,
             commandline_args,
             install_remote=False)
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         socket_id = lldbutil.wait_for_file_on_target(self, port_file)
 
         self.dbg.SetAsync(False)
 
         new_platform = lldb.SBPlatform(lldb.remote_platform.GetName())
         self.dbg.SetSelectedPlatform(new_platform)
 

lldb/test/API/tools/lldb-vscode/attach/TestVSCode_attach.py

def test_by_name(self):
         def cleanup():
             if os.path.exists(program):
                 os.unlink(program)
             self.run_platform_command("rm %s" % (pid_file_path))
         # Execute the cleanup function during test case tear down.
         self.addTearDownHook(cleanup)
 
         popen = self.spawnSubprocess(program, [pid_file_path])
-        self.addTearDownHook(self.cleanupSubprocesses)
 
         pid = lldbutil.wait_for_file_on_target(self, pid_file_path)
 
         self.attach(program=program)
         self.set_and_hit_breakpoint(continueToExit=True)
 
     @skipUnlessDarwin
     @skipIfDarwin