[patch] [gdbserver] Do not crash on file load without inferior

[patch] [gdbserver] Do not crash on file load without inferior

[Jan Kratochvil]

Hi Pedro,

I always got:

(gdb) file .../gdb/testsuite/gdb.server/ext-run
Load new symbol table from ".../gdb/testsuite/gdb.server/ext-run"? (y or n) y
Reading symbols from .../gdb/testsuite/gdb.server/ext-run...done.
gdbserver: Current inferior requested, but current_inferior is NULL

Remote connection closed
(gdb)

if one connects to gdbserver --multi before loading the file.  One needs to
load the file first to be able to place a breakpoint at *_start or main.

But I face other bugs so I cannot say much more.

I do not think this patch can ever have a regression.


Thanks,
Jan


gdb/gdbserver/
2011-02-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* server.c (handle_query) <qSymbol::>: Do not error on NULL
	CURRENT_INFERIOR.

gdb/testsuite/
2011-02-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.server/ext-run.exp
	(load new file without any gdbserver inferior): New test.

--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1373,13 +1373,17 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	 the library at all.  We also re-validate breakpoints when we
 	 see a second GDB breakpoint for the same address, and or when
 	 we access breakpoint shadows.  */
-      validate_breakpoints ();
 
-      if (target_supports_tracepoints ())
-	tracepoint_look_up_symbols ();
+      if (current_inferior != NULL)
+	{
+	  validate_breakpoints ();
 
-      if (target_running () && the_target->look_up_symbols != NULL)
-	(*the_target->look_up_symbols) ();
+	  if (target_supports_tracepoints ())
+	    tracepoint_look_up_symbols ();
+
+	  if (target_running () && the_target->look_up_symbols != NULL)
+	    (*the_target->look_up_symbols) ();
+	}
 
       strcpy (own_buf, "OK");
       return;
--- a/gdb/testsuite/gdb.server/ext-run.exp
+++ b/gdb/testsuite/gdb.server/ext-run.exp
@@ -60,4 +60,9 @@ if { [istarget *-*-linux*] } {
 }
 
 gdb_test "kill" "" "kill" "Kill the program being debugged.*" "y"
+
+gdb_load $binfile
+gdb_test "monitor help" "The following monitor commands.*" \
+	 "load new file without any gdbserver inferior"
+
 gdb_test_no_output "monitor exit"

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Pedro Alves]

On Thursday 24 February 2011 11:40:02, Jan Kratochvil wrote:
> Hi Pedro,
> 
> I always got:
> 
> (gdb) file .../gdb/testsuite/gdb.server/ext-run
> Load new symbol table from ".../gdb/testsuite/gdb.server/ext-run"? (y or n) y
> Reading symbols from .../gdb/testsuite/gdb.server/ext-run...done.
> gdbserver: Current inferior requested, but current_inferior is NULL
> 
> Remote connection closed
> (gdb)
> 
> if one connects to gdbserver --multi before loading the file.  One needs to
> load the file first to be able to place a breakpoint at *_start or main.
> 
> But I face other bugs so I cannot say much more.
> 
> I do not think this patch can ever have a regression.

Thanks.  I that revealed a problem on the GDB side instead.

E.g., if you have two inferiors loaded, and the not-current
inferior is running, but you do "file" on the not-running-yet
inferior, you'll see:

$ ./gdb ./testsuite/gdb.server/ext-run  -ex "tar extended-remote :9999" -ex "set remote exec-file /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run" -ex "start"
...
Reading symbols from /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run...done.
Setting up the environment for debugging gdb.
Remote debugging using :9999
Temporary breakpoint 1 at 0x4004ef: file ../../../src/gdb/testsuite/gdb.server/server.c, line 21.
Starting program: /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run 

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe068) at ../../../src/gdb/testsuite/gdb.server/server.c:21
21        return 0;
(gdb) add-inferior 
Added inferior 2
(gdb) info inferiors 
  Num  Description       Executable        
  2    <null>                              
* 1    process 15952     /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run 
(gdb) inferior 2
[Switching to inferior 2 [Thread 0.0] (<noexec>)]
(gdb) set debug remote 1
(gdb) file /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run
Reading symbols from /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run...done.
Sending packet: $Hgp0.0#ad...Packet received: OK
                ^^^^^^^
Sending packet: $qSymbol::#5b...Packet received: qSymbol:6764625f6167656e745f6764625f74705f686561705f627566666572
Sending packet: $qSymbol::6764625f6167656e745f6764625f74705f686561705f627566666572#1e...Packet received: qSymbol:6764625f6167656e745f6764625f6a756d705f7061645f627566666572
Sending packet: $qSymbol::6764625f6167656e745f6764625f6a756d705f7061645f627566666572#e1...Packet received: qSymbol:6764625f6167656e745f6764625f6a756d705f7061645f6275666665725f656e64
Sending packet: $qSymbol::6764625f6167656e745f6764625f6a756d705f7061645f6275666665725f656e64#ec...Packet received: qSymbol:6764625f6167656e745f636f6c6c656374696e67


But, Hgp0.0 means select _any_ thread in any process, not _no thread at all_.
Meaning GDBserver's current_inferior ends up set to a thread of the
wrong process, and GDBserver is querying inferior 1's symbols on
inferior 2!  See on GDBserver:

(gdb) p current_inferior->entry.id 
$3 = {pid = 15952, lwp = 15952, tid = 0}

So I think that we shouldn't send qSymbol at all when inferior_ptid is
null_ptid or rather with !target_has_execution (which ends up in
default_child_has_execution and currently resolves the same)?

-- 
Pedro Alves

RE: [patch] [gdbserver] Do not crash on file load without inferior

[Marc Khouzam]

 

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Thursday, February 24, 2011 7:43 AM
> To: gdb-patches@sourceware.org
> Cc: Jan Kratochvil
> Subject: Re: [patch] [gdbserver] Do not crash on file load 
> without inferior
> 
> On Thursday 24 February 2011 11:40:02, Jan Kratochvil wrote:
> > Hi Pedro,
> > 
> > I always got:
> > 
> > (gdb) file .../gdb/testsuite/gdb.server/ext-run
> > Load new symbol table from 
> ".../gdb/testsuite/gdb.server/ext-run"? (y or n) y
> > Reading symbols from .../gdb/testsuite/gdb.server/ext-run...done.
> > gdbserver: Current inferior requested, but current_inferior is NULL
> > 
> > Remote connection closed
> > (gdb)
> > 
> > if one connects to gdbserver --multi before loading the 
> file.  One needs to
> > load the file first to be able to place a breakpoint at 
> *_start or main.
> > 
> > But I face other bugs so I cannot say much more.
> > 
> > I do not think this patch can ever have a regression.
> 
> Thanks.  I that revealed a problem on the GDB side instead.
> 
> E.g., if you have two inferiors loaded, and the not-current
> inferior is running, but you do "file" on the not-running-yet
> inferior, you'll see:
> 
> $ ./gdb ./testsuite/gdb.server/ext-run  -ex "tar 
> extended-remote :9999" -ex "set remote exec-file 
> /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-ru
> n" -ex "start"
> ...
> Reading symbols from 
> /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-ru
> n...done.
> Setting up the environment for debugging gdb.
> Remote debugging using :9999
> Temporary breakpoint 1 at 0x4004ef: file 
> ../../../src/gdb/testsuite/gdb.server/server.c, line 21.
> Starting program: 
> /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run 
> 
> Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe068) at 
> ../../../src/gdb/testsuite/gdb.server/server.c:21
> 21        return 0;
> (gdb) add-inferior 
> Added inferior 2
> (gdb) info inferiors 
>   Num  Description       Executable        
>   2    <null>                              
> * 1    process 15952     
> /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run 
> (gdb) inferior 2
> [Switching to inferior 2 [Thread 0.0] (<noexec>)]
> (gdb) set debug remote 1
> (gdb) file 
> /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-run
> Reading symbols from 
> /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.server/ext-ru
> n...done.
> Sending packet: $Hgp0.0#ad...Packet received: OK
>                 ^^^^^^^
> Sending packet: $qSymbol::#5b...Packet received: 
> qSymbol:6764625f6167656e745f6764625f74705f686561705f627566666572
> Sending packet: 
> $qSymbol::6764625f6167656e745f6764625f74705f686561705f62756666
> 6572#1e...Packet received: 
> qSymbol:6764625f6167656e745f6764625f6a756d705f7061645f627566666572
> Sending packet: 
> $qSymbol::6764625f6167656e745f6764625f6a756d705f7061645f627566
> 666572#e1...Packet received: 
> qSymbol:6764625f6167656e745f6764625f6a756d705f7061645f62756666
> 65725f656e64
> Sending packet: 
> $qSymbol::6764625f6167656e745f6764625f6a756d705f7061645f627566
> 6665725f656e64#ec...Packet received: 
> qSymbol:6764625f6167656e745f636f6c6c656374696e67
> 
> 
> But, Hgp0.0 means select _any_ thread in any process, not _no 
> thread at all_.
> Meaning GDBserver's current_inferior ends up set to a thread of the
> wrong process, and GDBserver is querying inferior 1's symbols on
> inferior 2!  See on GDBserver:
> 
> (gdb) p current_inferior->entry.id 
> $3 = {pid = 15952, lwp = 15952, tid = 0}

That sounds pretty bad.

Any chance of getting a fix for this in the 7_2 branch?
Which I gather would fix the problem Jan originally reported.
Having to specify the 'file' before connecting to the target
is a regression from previous versions of GDB and I was hoping
not to have to special-case it in Eclipse :-)

Thanks

Marc


> 
> So I think that we shouldn't send qSymbol at all when inferior_ptid is
> null_ptid or rather with !target_has_execution (which ends up in
> default_child_has_execution and currently resolves the same)?


> 
> -- 
> Pedro Alves
> 

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Pedro Alves]

On Friday 04 March 2011 21:24:51, Marc Khouzam wrote:

> That sounds pretty bad.
> 
> Any chance of getting a fix for this in the 7_2 branch?
> Which I gather would fix the problem Jan originally reported.
> Having to specify the 'file' before connecting to the target
> is a regression from previous versions of GDB and I was hoping
> not to have to special-case it in Eclipse :-)

Indeed.  I'll try getting to it soon if Jan doesn't.  It
should be a one liner patch to gdb.

-- 
Pedro Alves

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On Fri, 04 Mar 2011 22:32:52 +0100, Pedro Alves wrote:
> On Friday 04 March 2011 21:24:51, Marc Khouzam wrote:
> 
> > That sounds pretty bad.
> > 
> > Any chance of getting a fix for this in the 7_2 branch?
> > Which I gather would fix the problem Jan originally reported.
> > Having to specify the 'file' before connecting to the target
> > is a regression from previous versions of GDB and I was hoping
> > not to have to special-case it in Eclipse :-)
> 
> Indeed.  I'll try getting to it soon if Jan doesn't.  It
> should be a one liner patch to gdb.

That patch already IMO required a regression test.  So far I never could do
any gdbserver regression testing as gdbserver generates multiple gigabytes of:
	Remote side has terminated connection.  GDBserver will reopen the connection.
	Can't open socket: Too many open files.

I wrote some patches for it but I haven't yet got to posting them, the second
one IIRC does not yet work.  After making gdbserver regression testable
I would like to get back to the original gdbserver problem.


Thanks,
Jan

[-- Attachment #2: p0.patch --]
[-- Type: text/plain, Size: 1637 bytes --]

diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 88ef347..dd40014 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -321,7 +321,10 @@ remote_open (char *name)
 
       if (bind (listen_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
 	  || listen (listen_desc, 1))
-	perror_with_name ("Can't bind address");
+	{
+	  close (listen_desc);
+	  perror_with_name ("Can't bind address");
+	}
 
       /* If port is zero, a random port will be selected, and the
 	 fprintf below needs to know what port was selected.  */
@@ -331,7 +334,10 @@ remote_open (char *name)
 	  if (getsockname (listen_desc,
 			   (struct sockaddr *) &sockaddr, &len) < 0
 	      || len < sizeof (sockaddr))
-	    perror_with_name ("Can't determine port");
+	    {
+	      close (listen_desc);
+	      perror_with_name ("Can't determine port");
+	    }
 	  port = ntohs (sockaddr.sin_port);
 	}
 
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 0ddf9de..b04a50d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2650,6 +2650,9 @@ main (int argc, char *argv[])
 
   while (1)
     {
+      /* Wait a moment when for example the listening port is now busy.  */
+      int delay = 1;
+
       noack_mode = 0;
       multi_process = 0;
       /* Be sure we're out of tfind mode.  */
@@ -2665,7 +2668,11 @@ main (int argc, char *argv[])
 	      write_enn (own_buf);
 	      putpkt (own_buf);
 	    }
+
+	  if (delay)
+	    sleep (1);
 	}
+      delay = 0;
 
       /* Wait for events.  This will return when all event sources are
 	 removed from the event loop.  */

[-- Attachment #3: p1.patch --]
[-- Type: text/plain, Size: 1608 bytes --]

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index b04a50d..76a027f 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2312,7 +2312,8 @@ gdbserver_usage (FILE *stream)
 	   "  --debug               Enable general debugging output.\n"
 	   "  --remote-debug        Enable remote protocol debugging output.\n"
 	   "  --version             Display version information and exit.\n"
-	   "  --wrapper WRAPPER --  Run WRAPPER to start new programs.\n");
+	   "  --wrapper WRAPPER --  Run WRAPPER to start new programs.\n"
+	   "  --once                Exit after the first connection closed.\n");
   if (REPORT_BUGS_TO[0] && stream == stdout)
     fprintf (stream, "Report bugs to \"%s\".\n", REPORT_BUGS_TO);
 }
@@ -2536,6 +2537,8 @@ main (int argc, char *argv[])
 		}
 	    }
 	}
+      else if (strcmp (*next_arg, "--once") == 0)
+	exit_requested = 1;
       else
 	{
 	  fprintf (stderr, "Unknown argument: %s\n", *next_arg);
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 3a098ae..c045c5f 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -218,10 +218,16 @@ proc gdbserver_start { options arguments } {
 
 	# Fire off the debug agent.
 	set gdbserver_command "$gdbserver"
+
+	# GDB client could accidentally connect to a stale server.
+	append gdbserver_command " --once"
+
 	if { $options != "" } {
 	    append gdbserver_command " $options"
 	}
+
 	append gdbserver_command " :$portnum"
+
 	if { $arguments != "" } {
 	    append gdbserver_command " $arguments"
 	}

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Pedro Alves]

On Friday 04 March 2011 21:37:28, Jan Kratochvil wrote:
> That patch already IMO required a regression test.  So far I never could do
> any gdbserver regression testing as gdbserver generates multiple gigabytes of:
>         Remote side has terminated connection.  GDBserver will reopen the connection.
>         Can't open socket: Too many open files.

Odd, I don't see that, with FORCE_PARALLEL and -j8, even.

> I wrote some patches for it but I haven't yet got to posting them, the second
> one IIRC does not yet work.  After making gdbserver regression testable
> I would like to get back to the original gdbserver problem.

Are you saying you didn't have a chance of thinking about
my rationale, or that you don't agree with my rationale,
or that you wanted to write a better test, or?

-- 
Pedro Alves

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Michael Snyder]

Pedro Alves wrote:
> On Friday 04 March 2011 21:37:28, Jan Kratochvil wrote:
>> That patch already IMO required a regression test.  So far I never could do
>> any gdbserver regression testing as gdbserver generates multiple gigabytes of:
>>         Remote side has terminated connection.  GDBserver will reopen the connection.
>>         Can't open socket: Too many open files.
> 
> Odd, I don't see that, with FORCE_PARALLEL and -j8, even.

I run the testsuite using gdbserver -- it works for me...

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Jan Kratochvil]

On Fri, 04 Mar 2011 23:05:50 +0100, Pedro Alves wrote:
> Are you saying you didn't have a chance of thinking about
> my rationale, or that you don't agree with my rationale,
> or that you wanted to write a better test, or?

I could find some examples from the past (not sure if specifically for your
fixes or not - I do not find relevant whether such case happened or not with
your fixes) when even despite serious thinking about the problem on the list
the testsuite then found for the commit a regression and/or incomplete fix.

Given how cheap is to run the testsuite I do not think it is worth spending
any human effort on coding without having the testsuite results available.


Thanks,
Jan

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Pedro Alves]

On Saturday 05 March 2011 04:11:03, Jan Kratochvil wrote:
> Given how cheap is to run the testsuite I do not think it is worth spending
> any human effort on coding without having the testsuite results available.

This passes the testsuite without regressions for me,
and I confirmed the new test fails with the gdb hunk
omitted.

Comments?

-- 
Pedro Alves

gdb/
2011-03-05  Pedro Alves  <pedro@codesourcery.com>

	* remote.c (remote_check_symbols): Skip if the target has no
	execution.

gdb/testsuite/
2011-03-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.server/ext-run.exp
	(load new file without any gdbserver inferior): New test.

---
 gdb/remote.c                         |   11 ++++++++++-
 gdb/testsuite/gdb.server/ext-run.exp |    5 +++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2011-03-05 10:19:07.000000000 +0000
+++ src/gdb/remote.c	2011-03-05 10:54:28.582530001 +0000
@@ -3435,10 +3435,19 @@ remote_check_symbols (struct objfile *ob
   struct minimal_symbol *sym;
   int end;
 
+  /* The remote side has no concept of inferiors that aren't running
+     yet, it only knows about running processes.  If we're connected
+     but our current inferior is not running, we should not invite the
+     remote target to request symbol lookups related to its
+     (unrelated) current process.  */
+  if (!target_has_execution)
+    return;
+
   if (remote_protocol_packets[PACKET_qSymbol].support == PACKET_DISABLE)
     return;
 
-  /* Make sure the remote is pointing at the right process.  */
+  /* Make sure the remote is pointing at the right process.  Note
+     there's no way to select "no process".  */
   set_general_process ();
 
   /* Allocate a message buffer.  We can't reuse the input buffer in RS,
Index: src/gdb/testsuite/gdb.server/ext-run.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.server/ext-run.exp	2011-03-05 10:18:39.000000000 +0000
+++ src/gdb/testsuite/gdb.server/ext-run.exp	2011-03-05 10:49:00.772529995 +0000
@@ -60,4 +60,9 @@ if { [istarget *-*-linux*] } {
 }
 
 gdb_test "kill" "" "kill" "Kill the program being debugged.*" "y"
+
+gdb_load $binfile
+gdb_test "monitor help" "The following monitor commands.*" \
+        "load new file without any gdbserver inferior"
+
 gdb_test_no_output "monitor exit"

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Jan Kratochvil]

On Sat, 05 Mar 2011 12:17:48 +0100, Pedro Alves wrote:
> This passes the testsuite without regressions for me,
> and I confirmed the new test fails with the gdb hunk
> omitted.
> 
> Comments?

It looks great, please check it in.


Thanks,
Jan

Re: [patch] [gdbserver] Do not crash on file load without inferior

[Pedro Alves]

On Saturday 05 March 2011 12:19:23, Jan Kratochvil wrote:
> On Sat, 05 Mar 2011 12:17:48 +0100, Pedro Alves wrote:
> > This passes the testsuite without regressions for me,
> > and I confirmed the new test fails with the gdb hunk
> > omitted.
> > 
> > Comments?
> 
> It looks great, please check it in.

Done, 7.2 and head.

-- 
Pedro Alves