Skip to content

Commit

Permalink
[lldb/test] Don't use preexec_fn for launching inferiors
Browse files Browse the repository at this point in the history
As the documentation states, using this is not safe in multithreaded
programs, and I have traced it to a rare deadlock in some of the tests.

The reason this was introduced was to be able to attach to a program
from the very first instruction, where our usual mechanism of
synchronization -- waiting for a file to appear -- does not work.

However, this is only needed for a single test
(TestGdbRemoteAttachWait) so instead of doing this everywhere, I create
a bespoke solution for that single test. The solution basically
consists of outsourcing the preexec_fn code to a separate (and
single-threaded) shim process, which enables attaching and then executes
the real program.

This pattern could be generalized in case we needed to use it for other
tests, but I suspect that we will not be having many tests like this.

This effectively reverts commit
a997a1d.
  • Loading branch information
labath committed Jul 1, 2022
1 parent fabe915 commit b15b142
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 17 deletions.
14 changes: 1 addition & 13 deletions lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
Expand Up @@ -4,12 +4,11 @@
from __future__ import absolute_import

# System modules
import ctypes
import itertools
import os
import re
import subprocess
import sys
import os

# Third-party modules
import six
Expand Down Expand Up @@ -192,14 +191,3 @@ def hasChattyStderr(test_case):
if match_android_device(test_case.getArchitecture(), ['aarch64'], range(22, 25+1)):
return True # The dynamic linker on the device will complain about unknown DT entries
return False

if getHostPlatform() == "linux":
def enable_attach():
"""Enable attaching to _this_ process, if host requires such an action.
Suitable for use as a preexec_fn in subprocess.Popen and similar."""
c = ctypes.CDLL(None)
PR_SET_PTRACER = ctypes.c_int(0x59616d61)
PR_SET_PTRACER_ANY = ctypes.c_ulong(-1)
c.prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY)
else:
enable_attach = None
1 change: 0 additions & 1 deletion lldb/packages/Python/lldbsuite/test/lldbtest.py
Expand Up @@ -393,7 +393,6 @@ def launch(self, executable, args, extra_env):
stdout=open(
os.devnull) if not self._trace_on else None,
stdin=PIPE,
preexec_fn=lldbplatformutil.enable_attach,
env=env)

def terminate(self):
Expand Down
1 change: 1 addition & 0 deletions lldb/test/API/tools/lldb-server/attach-wait/Makefile
@@ -0,0 +1 @@
include Makefile.rules
Expand Up @@ -14,10 +14,12 @@ class TestGdbRemoteAttachWait(gdbremote_testcase.GdbRemoteTestCaseBase):
@skipIfWindows # This test is flaky on Windows
def test_attach_with_vAttachWait(self):
exe = '%s_%d' % (self.testMethodName, os.getpid())
exe_to_attach = exe
args = []

def launch_inferior():
inferior = self.launch_process_for_attach(
inferior_args=["sleep:60"],
inferior_args=args,
exe_path=self.getBuildArtifact(exe))
self.assertIsNotNone(inferior)
self.assertTrue(inferior.pid > 0)
Expand All @@ -26,7 +28,14 @@ def launch_inferior():
inferior.pid, True))
return inferior

self.build(dictionary={'EXE': exe})
self.build(dictionary={'EXE': exe, 'CXX_SOURCES': 'main.cpp'})
if self.getPlatform() != "windows":
# Use a shim to ensure that the process is ready to be attached from
# the get-go.
args = [self.getBuildArtifact(exe)]
exe = "shim"
self.build(dictionary={'EXE': exe, 'CXX_SOURCES': 'shim.cpp'})

self.set_inferior_startup_attach_manually()

server = self.connect_to_debug_monitor()
Expand All @@ -38,7 +47,8 @@ def launch_inferior():
self.do_handshake()
self.test_sequence.add_log_lines([
# Do the attach.
"read packet: $vAttachWait;{}#00".format(lldbgdbserverutils.gdbremote_hex_encode_string(exe)),
"read packet: $vAttachWait;{}#00".format(
lldbgdbserverutils.gdbremote_hex_encode_string(exe_to_attach)),
], True)
# Run the stream until attachWait.
context = self.expect_gdbremote_sequence()
Expand Down
8 changes: 8 additions & 0 deletions lldb/test/API/tools/lldb-server/attach-wait/main.cpp
@@ -0,0 +1,8 @@
#include <thread>

int main()
{
// Wait to be attached.
std::this_thread::sleep_for(std::chrono::minutes(1));
return 0;
}
9 changes: 9 additions & 0 deletions lldb/test/API/tools/lldb-server/attach-wait/shim.cpp
@@ -0,0 +1,9 @@
#include <unistd.h>
#include <cstdio>

int main(int argc, char *argv[]) {
lldb_enable_attach();
execlp(argv[1], argv[1], nullptr);
perror("execlp");
return 1;
}

0 comments on commit b15b142

Please sign in to comment.