Skip to content

Commit

Permalink
Recommit "[lldb/test] Don't use preexec_fn for launching inferiors"
Browse files Browse the repository at this point in the history
This recommits b15b142, which reverted in was reverted in f51c47d due to
failures on apple systems. The problem was that the patch introduced a race
where the debug server could start the attach process before the first process
(which isn't supposed to be attached to) was set up. This caused us to attach
to the wrong process.

The new version introduces additional synchronization to ensure that does not
happen.

Original commit message was:
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 7, 2022
1 parent 64a78c8 commit 82ba3f4
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 18 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,13 @@ 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
sync_file_path = lldbutil.append_to_process_working_directory(self, "process_ready")
args = [sync_file_path]

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,19 +29,29 @@ 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)] + args
exe = "shim"
self.build(dictionary={'EXE': exe, 'CXX_SOURCES': 'shim.cpp'})

self.set_inferior_startup_attach_manually()

server = self.connect_to_debug_monitor()
self.assertIsNotNone(server)

# Launch the first inferior (we shouldn't attach to this one).
launch_inferior()


lldbutil.wait_for_file_on_target(self, sync_file_path)

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
12 changes: 12 additions & 0 deletions lldb/test/API/tools/lldb-server/attach-wait/main.cpp
@@ -0,0 +1,12 @@
#include <thread>
#include <fstream>

int main(int argc, char *argv[])
{
if (argc >= 2) {
std::ofstream(argv[1]).close();
}
// 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();
execvp(argv[1], argv+1);
perror("execlp");
return 1;
}

0 comments on commit 82ba3f4

Please sign in to comment.