Re: [PATCH 0/4 v3] Exec events in gdbserver on Linux

Breazeal, Don
2014-05-28T18:02:46Z

On 5/27/2014 2:41 PM, Breazeal, Don wrote:
> On 5/27/2014 11:49 AM, Breazeal, Don wrote:
>> On 5/25/2014 9:55 PM, Doug Evans wrote:

--snip snip--

>>> Hi.
>>>
>>> How do I tweak your patch so that I can see the race condition for myself?
>>> [I realize the window is small ... I'd just like to play with it.]
>>
>> Hi Doug, thanks for looking at this. I will have to look back through
>> my notes to see if I can come up with a reasonable way of doing this. I
>> was using the same test case that you were, running the basic test
>> manually. I saw the race condition by inserting fprintf(stderr,.. into
>> linux_proc_pid_has_state and check_zombie_leaders, running with no debug
>> output.

Now that I have gone back to look at this, I recall that I wasn't
running the test manually, but using a script that ran it repeatedly
until it failed. At different times the failure would occur at
different frequencies. I attributed that to system load, which I think
was relatively high.

>>
>> From my notes:

--snip snip--

> The '#' lines are my annotations,
> the other lines were actual trace log output.
> ----------------------------------------------
> # result of if stmt condition in check_zombie_leaders
> # original leader is in zombie state
> linux_proc_pid_has_state: procfile:
> State: Z (zombie)
>
> # result inside 'if' stmt in check_zombie_leaders - execing
> # thread has replaced original leader since we evaluated
> # the 'if' condition
> linux_proc_pid_has_state: procfile:
> State: R (running)
>
> # printed inside if stmt, required zombie=1 to get here
> # we still think we have 2 lwps, but after the exec there
> # is only one. zombie=0 came from call to linux_proc_pid_has_state
> # above.
> check_zombie_leaders: leader_pid=30981, leader_lp!=NULL=1, num_lwps=2,
> zombie=0
> ----------------------------------------------

I don't think there is a way to (reasonably) tweak my existing patch to
use something other than exit events. Here is a patch for an earlier
implementation of remote exec events that contains the race condition.
If I hadn't run into the race condition in my testing I would have
submitted something similar to this.

I spent yesterday afternoon and part of this morning trying to reproduce
the failure, without success. I ran gdb.threads/non-ldr-exc-1.exp
several thousand times, with/without optimization, on local/NFS drives,
inserted random sleeps, etc.

Print statements similar to those that generated the logs above are in
check_zombie_leaders and linux_proc_pid_has_state, commented out.

I hope this is useful.
--Don

Subject: Exec patch w/race condition

---
gdb/common/linux-procfs.c | 1 +
gdb/common/linux-ptrace.c | 2 +-
gdb/gdbserver/linux-low.c | 98
++++++++++++++++++++++++++++++++++++++---
gdb/gdbserver/linux-low.h | 5 ++
gdb/gdbserver/remote-utils.c | 27 +++++++++++-
gdb/remote.c | 19 ++++++++
6 files changed, 142 insertions(+), 10 deletions(-)

diff --git a/gdb/common/linux-procfs.c b/gdb/common/linux-procfs.c
index 1443a88..7233ce4 100644
--- a/gdb/common/linux-procfs.c
+++ b/gdb/common/linux-procfs.c
@@ -95,6 +95,7 @@ linux_proc_pid_has_state (pid_t pid, const char *state)
while (fgets (buffer, sizeof (buffer), procfile) != NULL)
if (strncmp (buffer, "State:", 6) == 0)
{
+//fprintf (stderr, "%s: %s\n", __func__, buffer);
have_state = 1;
break;
}
diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index efbd1ea..e38e266 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -437,7 +437,7 @@ linux_test_for_tracefork (int child_pid)
/* Do not enable all the options for now since gdbserver does not
properly support them. This restriction will be lifted when
gdbserver is augmented to support them. */
- current_ptrace_options |= PTRACE_O_TRACECLONE;
+ current_ptrace_options |= PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
#else
current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
| PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 1932ff2..b8a808f 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -280,6 +280,32 @@ static int linux_event_pipe[2] = { -1, -1 };
static void send_sigstop (struct lwp_info *lwp);
static void wait_for_sigstop (void);

+/* Accepts an integer PID; Returns a string representing a file that
+ can be opened to get info for the child process.
+ Space for the result is malloc'd, caller must free. */
+
+static char *
+linux_child_pid_to_exec_file (int pid)
+{
+ char *name1, *name2;
+
+ name1 = xmalloc (PATH_MAX);
+ name2 = xmalloc (PATH_MAX);
+ memset (name2, 0, PATH_MAX);
+
+ sprintf (name1, "/proc/%d/exe", pid);
+ if (readlink (name1, name2, PATH_MAX) > 0)
+ {
+ free (name1);
+ return name2;
+ }
+ else
+ {
+ free (name2);
+ return name1;
+ }
+}
+
/* Return non-zero if HEADER is a 64-bit ELF file. */

static int
@@ -369,9 +395,10 @@ linux_add_process (int pid, int attached)

/* Handle a GNU/Linux extended wait response. If we see a clone
event, we need to add the new LWP to our list (and not report the
- trap to higher layers). */
+ trap to higher layers). This function returns non-zero if the
+ event should be ignored and we should wait again. */

-static void
+static int
handle_extended_wait (struct lwp_info *event_child, int wstat)
{
int event = wstat >> 16;
@@ -452,7 +479,27 @@ handle_extended_wait (struct lwp_info *event_child,
int wstat)
threads, it will have a pending SIGSTOP; we may as well
collect it now. */
linux_resume_one_lwp (event_child, event_child->stepping, 0, NULL);
+
+ /* Don't report the event. */
+ return 1;
}
+
+ if (event == PTRACE_EVENT_EXEC)
+ {
+ if (debug_threads)
+ debug_printf ("LHEW: Got exec event from LWP %ld\n",
+ lwpid_of (event_thr));
+
+ event_child->waitstatus.kind = TARGET_WAITKIND_EXECD;
+ event_child->waitstatus.value.execd_pathname
+ = linux_child_pid_to_exec_file (lwpid_of (event_thr));
+
+ /* Report the event. */
+ return 0;
+ }
+
+ internal_error (__FILE__, __LINE__,
+ _("unknown ptrace event %d"), event);
}

/* Return the PC as read from the regcache of LWP, without any
@@ -1345,6 +1392,7 @@ check_zombie_leaders (void)
"(it exited, or another thread execd).\n",
leader_pid);

+//fprintf (stderr, "%s: ldr lwp %d, zombie %d\n", __func__, leader_pid,
linux_proc_pid_is_zombie (leader_pid));
delete_lwp (leader_lp);
}
}
@@ -1851,8 +1899,8 @@ linux_low_filter_event (ptid_t filter_ptid, int
lwpid, int wstat)
if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
&& wstat >> 16 != 0)
{
- handle_extended_wait (child, wstat);
- return NULL;
+ if (handle_extended_wait (child, wstat))
+ return NULL;
}

if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGSTOP
@@ -2452,6 +2500,19 @@ linux_stabilize_threads (void)
}
}

+/* Return non-zero if WAITSTATUS reflects an extended linux
+ event. Otherwise, return 0. */
+
+static int
+extended_event_reported (const struct target_waitstatus *waitstatus)
+{
+
+ if (waitstatus == NULL)
+ return 0;
+
+ return waitstatus->kind == TARGET_WAITKIND_EXECD;
+}
+
/* Wait for process, returns status. */

static ptid_t
@@ -2815,7 +2876,8 @@ retry:
&& !bp_explains_trap && !trace_event)
|| (gdb_breakpoint_here (event_child->stop_pc)
&& gdb_condition_true_at_breakpoint (event_child->stop_pc)
- && gdb_no_commands_at_breakpoint (event_child->stop_pc)));
+ && gdb_no_commands_at_breakpoint (event_child->stop_pc))
+ || extended_event_reported (&event_child->waitstatus));

run_breakpoint_commands (event_child->stop_pc);

@@ -2837,7 +2899,16 @@ retry:
paddress (event_child->stop_pc),
paddress (event_child->step_range_start),
paddress (event_child->step_range_end));
- }
+ if (debug_threads
+ && extended_event_reported (&event_child->waitstatus))
+ {
+ char *str = target_waitstatus_to_string (ourstatus);
+ debug_printf ("LWP %ld has forked, cloned, vforked or
execd"
+ " with waitstatus %s\n",
+ lwpid_of (current_inferior), str);
+ xfree (str);
+ }
+ }

/* We're not reporting this breakpoint to GDB, so apply the
decr_pc_after_break adjustment to the inferior's regcache
@@ -2935,7 +3006,18 @@ retry:
unstop_all_lwps (1, event_child);
}

- ourstatus->kind = TARGET_WAITKIND_STOPPED;
+ /* If the reported event is a fork, vfork or exec, let GDB know. */
+ if (extended_event_reported (&event_child->waitstatus))
+ {
+ ourstatus->kind = event_child->waitstatus.kind;
+ ourstatus->value = event_child->waitstatus.value;
+
+ /* Reset the event child's waitstatus since we handled it
+ already. */
+ event_child->waitstatus.kind = TARGET_WAITKIND_IGNORE;
+ }
+ else
+ ourstatus->kind = TARGET_WAITKIND_STOPPED;

if (current_inferior->last_resume_kind == resume_stop
&& WSTOPSIG (w) == SIGSTOP)
@@ -2952,7 +3034,7 @@ retry:
but, it stopped for other reasons. */
ourstatus->value.sig = gdb_signal_from_host (WSTOPSIG (w));
}
- else
+ else if (ourstatus->kind == TARGET_WAITKIND_STOPPED)
{
ourstatus->value.sig = gdb_signal_from_host (WSTOPSIG (w));
}
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 498b221..2534ad1 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -269,6 +269,11 @@ struct lwp_info
/* When stopped is set, the last wait status recorded for this lwp. */
int last_status;

+ /* If WAITSTATUS->KIND != TARGET_WAITKIND_SPURIOUS, the waitstatus
+ for this LWP's last event. This may correspond to LAST_STATUS above,
+ or to a local variable in lin_lwp_wait. */
+ struct target_waitstatus waitstatus;
+
/* When stopped is set, this is where the lwp stopped, with
decr_pc_after_break already accounted for. */
CORE_ADDR stop_pc;
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 4fcafa0..6b8c10b 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -1111,14 +1111,39 @@ prepare_resume_reply (char *buf, ptid_t ptid,
switch (status->kind)
{
case TARGET_WAITKIND_STOPPED:
+ case TARGET_WAITKIND_EXECD:
{
struct thread_info *saved_inferior;
const char **regp;
struct regcache *regcache;
+ enum gdb_signal signal;

- sprintf (buf, "T%02x", status->value.sig);
+ if (status->kind == TARGET_WAITKIND_EXECD)
+ signal = GDB_SIGNAL_TRAP;
+ else
+ signal = status->value.sig;
+
+ sprintf (buf, "T%02x", signal);
buf += strlen (buf);

+ if (status->kind == TARGET_WAITKIND_EXECD && multi_process)
+ {
+ const char *event = "target_exec";
+ char hexified_pathname[PATH_MAX];
+
+ sprintf (buf, "%s:", event);
+ buf += strlen (buf);
+
+ /* Encode pathname to hexified format. */
+ bin2hex ((const gdb_byte *) status->value.execd_pathname,
+ hexified_pathname,
strlen(status->value.execd_pathname));
+
+ sprintf (buf, "%s;", hexified_pathname);
+ xfree (status->value.execd_pathname);
+ status->value.execd_pathname = NULL;
+ buf += strlen (buf);
+ }
+
saved_inferior = current_inferior;

current_inferior = find_thread_ptid (ptid);
diff --git a/gdb/remote.c b/gdb/remote.c
index 964bd41..55bafc2 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5471,6 +5471,25 @@ Packet: '%s'\n"),
p = unpack_varlen_hex (++p1, &c);
event->core = c;
}
+ else if (strncmp (p, "target_exec", p1 - p) == 0)
+ {
+ ULONGEST pid;
+ char pathname[PATH_MAX];
+
+ p = unpack_varlen_hex (++p1, &pid);
+
+ /* Save the pathname for event reporting and for
+ the next run command. */
+ hex2bin (p1, (gdb_byte *) pathname, (p - p1)/2);
+ /* Add the null terminator. */
+ pathname[(p - p1)/2] = '\0';
+ /* This is freed during event handling. */
+ event->ws.value.execd_pathname = xstrdup (pathname);
+ event->ws.kind = TARGET_WAITKIND_EXECD;
+ /* Save the pathname for the next run command. */
+ xfree (remote_exec_file);
+ remote_exec_file = pathname;
+ }
else
{
/* Silently skip unknown optional info. */
--
1.7.0.4