[PATCH] Fix for gdb.server/non-existing-program.exp test case

[PATCH] Fix for gdb.server/non-existing-program.exp test case

[Carl E. Love]

Fix for gdb.server/non-existing-program.exp test case

The test checks to make sure GDB exits cleanly if there is
no valid target binary.  Currently, ppc and S390 fail on this
test.  The function target_post_create_inferior () calls
linux_post_create_inferior () which calls the architecture
specific functions s390_arch_setup () and ppc_arch_setup ()
which make ptrace calls	to access the architecture specific
registers.  These ptrace calls fail because the	process	does
not exist causing GDB to exit on error.

This patch checks to see if the initial ptrace (PTRACE_TRACEME, ...)
call returned a status of TARGET_WAITKIND_EXITED indicating the
target has already exited.  If the target has exited, then the
target_post_create_inferior () is not called since there is no
inferior to be setup.  The test	to see if the initial ptrace
call succeeded is done after the ptrace (PTRACE_TRACEME, ...)
call and the wait for the inferior process to stop, assuming
it exists, has occurred.

The patch has been tested on X86 64-bit, ppc64 and s390.  If
fixes the test failures	on ppc64 and s390.  The	test does not
fail on	X86 64-bit.  The patch does not	introduce any additional
regression failures on any of these three platforms.

gdbserver/ChangeLog

2016-09-06  Carl Love  <cel@us.ibm.com>

            * server.c (start_inferior):  Do not call
            function target_post_create_inferior () if the
            inferior process has already exited.
---
 gdb/gdbserver/server.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 6fbd61d..8a6708f 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -290,11 +290,16 @@ start_inferior (char **argv)
      (assuming success).  */
   last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
 
-  target_post_create_inferior ();
-
+  /* The last_status.kind was set by the call to
ptrace(PTRACE_TRACEME, ...).
+     The function linux_wait() has also been called.  At this point,
the
+     target process, if it exits, is stopped.  Depending on the
architecture,
+     the function target_post_create_inferior () may make additional
ptrace ()
+     calls that will fail if the target has already exited.
+  */
   if (last_status.kind != TARGET_WAITKIND_EXITED
       && last_status.kind != TARGET_WAITKIND_SIGNALLED)
     {
+      target_post_create_inferior ();
       current_thread->last_resume_kind = resume_stop;
       current_thread->last_status = last_status;
     }
-- 
2.4.11

Re: [PATCH] Fix for gdb.server/non-existing-program.exp test case

[Ulrich Weigand]

Carl Love wrote:

> 2016-09-06  Carl Love  <cel@us.ibm.com>
> 
>             * server.c (start_inferior):  Do not call
>             function target_post_create_inferior () if the
>             inferior process has already exited.

The patch makes sense to me, however there seem to be
some formatting issues (mail client problems?):

> +  /* The last_status.kind was set by the call to
> ptrace(PTRACE_TRACEME, ...).
> +     The function linux_wait() has also been called.  At this point,
> the
> +     target process, if it exits, is stopped.  Depending on the
> architecture,
> +     the function target_post_create_inferior () may make additional
> ptrace ()
> +     calls that will fail if the target has already exited.
> +  */

Please make sure this is properly formatted (and does not exceed
the 80 characters per line limit).

Also, the comment seems a bit too specific; this file is also used
for targets other than Linux that may not use ptrace specifically.
I'd word the comment a bit more generically, along the lines of
"Do not call target_post_create_inferior if the process has already
exited, since the target implementation of that routine may rely on
the process being live."

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix for gdb.server/non-existing-program.exp test case

[Carl E. Love]

On Tue, 2016-09-13 at 15:26 +0200, Ulrich Weigand wrote:
> Carl Love wrote:
> 
> > 2016-09-06  Carl Love  <cel@us.ibm.com>
> > 
> >             * server.c (start_inferior):  Do not call
> >             function target_post_create_inferior () if the
> >             inferior process has already exited.
> 
> The patch makes sense to me, however there seem to be
> some formatting issues (mail client problems?):
> 
> > +  /* The last_status.kind was set by the call to
> > ptrace(PTRACE_TRACEME, ...).
> > +     The function linux_wait() has also been called.  At this point,
> > the
> > +     target process, if it exits, is stopped.  Depending on the
> > architecture,
> > +     the function target_post_create_inferior () may make additional
> > ptrace ()
> > +     calls that will fail if the target has already exited.
> > +  */
> 
> Please make sure this is properly formatted (and does not exceed
> the 80 characters per line limit).
> 
> Also, the comment seems a bit too specific; this file is also used
> for targets other than Linux that may not use ptrace specifically.
> I'd word the comment a bit more generically, along the lines of
> "Do not call target_post_create_inferior if the process has already
> exited, since the target implementation of that routine may rely on
> the process being live."

I fixed up the comment per your suggestion.  Not sure what happened with
the formatting. See if this looks better.

                           Carl Love 
---------------------------------------------------------------------------

Fix for gdb.server/non-existing-program.exp test case

The test checks to make sure GDB exits cleanly if there is
no valid target binary.  Currently, ppc and S390 fail on this
test.  The function target_post_create_inferior () calls
linux_post_create_inferior () which calls the architecture
specific functions s390_arch_setup () and ppc_arch_setup ()
which make ptrace calls	to access the architecture specific
registers.  These ptrace calls fail because the	process	does
not exist causing GDB to exit on error.

This patch checks to see if the initial ptrace (PTRACE_TRACEME, ...)
call returned a status of TARGET_WAITKIND_EXITED indicating the
target has already exited.  If the target has exited, then the
target_post_create_inferior () is not called since there is no
inferior to be setup.  The test	to see if the initial ptrace
call succeeded is done after the ptrace (PTRACE_TRACEME, ...)
call and the wait for the inferior process to stop, assuming
it exists, has occurred.

The patch has been tested on X86 64-bit, ppc64 and s390.  If
fixes the test failures	on ppc64 and s390.  The	test does not
fail on	X86 64-bit.  The patch does not	introduce any additional
regression failures on any of these three platforms.

gdbserver/ChangeLog

2016-09-06  Carl Love  <cel@us.ibm.com>

            * server.c (start_inferior):  Do not call
            function target_post_create_inferior () if the
            inferior process has already exited.
---
 gdb/gdbserver/server.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 6fbd61d..908be47 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -290,11 +290,16 @@ start_inferior (char **argv)
      (assuming success).  */
   last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
 
-  target_post_create_inferior ();
-
+  /* The last_status.kind was set by the call to ptrace(PTRACE_TRACEME, ...).
+     The function linux_wait() has also been called.  At this point, the
+     target process, if it exits, is stopped.  Do not call the function
+     target_post_create_inferior if the process has already exited, as the
+     target implementation of the routine may rely on the process being live.
+  */
   if (last_status.kind != TARGET_WAITKIND_EXITED
       && last_status.kind != TARGET_WAITKIND_SIGNALLED)
     {
+      target_post_create_inferior ();
       current_thread->last_resume_kind = resume_stop;
       current_thread->last_status = last_status;
     }
-- 
2.4.11

Re: [PATCH] Fix for gdb.server/non-existing-program.exp test case

[Ulrich Weigand]

Carl Love wrote:

> I fixed up the comment per your suggestion.  Not sure what happened with
> the formatting. See if this looks better.

> +  /* The last_status.kind was set by the call to ptrace(PTRACE_TRACEME, ...).
> +     The function linux_wait() has also been called.  At this point, the
> +     target process, if it exits, is stopped.  Do not call the function
> +     target_post_create_inferior if the process has already exited, as the
> +     target implementation of the routine may rely on the process being live.
> +  */

Formatting looks OK now, except that the trailing */ should not be on a
separate line; see other multi-line comments how those should look like.

However, the comment still mentions ptrace and linux_wait, which is also
only true on Linux targets.  I'd just leave that off.

Patch is OK with that change.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix for gdb.server/non-existing-program.exp test case

[Carl E. Love]

On Tue, 2016-09-13 at 17:37 +0200, Ulrich Weigand wrote:
> Carl Love wrote:
> 
> > I fixed up the comment per your suggestion.  Not sure what happened with
> > the formatting. See if this looks better.
> 
> > +  /* The last_status.kind was set by the call to ptrace(PTRACE_TRACEME, ...).
> > +     The function linux_wait() has also been called.  At this point, the
> > +     target process, if it exits, is stopped.  Do not call the function
> > +     target_post_create_inferior if the process has already exited, as the
> > +     target implementation of the routine may rely on the process being live.
> > +  */
> 
> Formatting looks OK now, except that the trailing */ should not be on a
> separate line; see other multi-line comments how those should look like.
> 
> However, the comment still mentions ptrace and linux_wait, which is also
> only true on Linux targets.  I'd just leave that off.
> 
> Patch is OK with that change.
> 
> Bye,
> Ulrich
> 

Patch committed. Note, I updated the comment per requested.  Note, I
realized I only did part of the requested update initially and then did
then removed the ptrace () comment.  The patch didn't get updated with
the removal of the ptrace () comment.  I had to do a second commit to
get the ptrace () comment fixed correctly.  Sorry for the screw up.
Here are the two commits.


https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=1d8cb77dff14d44b1e3b670442438da496f99c6e
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=7313bced5b695b71a707c82b6817763046e21bb1

                    Carl Love