[RFA] remote debugging patches

[RFA] remote debugging patches

[Michal Ludvig]

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

Hi,
Here are two patches for remote debugging. The first one is pretty 
straightforward and just makes use of the last parameter to getpkt().
The second one changes banner printed after successful connection to 
gdbserver from "Remote debugging using :1234" to more informational
"Remote debugging from host whatever.name.com". When debugging via 
serial line, nothing changes.

Index: ChangeLog

	* remote.c: Added WAIT_FOREVER_FLAG and changed all
	calls to getpkt() to respect it.
	* gdbserver/remote-utils.c (remote_open): After successful
	connection the remote host name is printed instead of local
	port number.


Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

[-- Attachment #2: gdb-remote.diff --]
[-- Type: text/plain, Size: 12590 bytes --]

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.77
diff -u -r1.77 remote.c
--- remote.c	2002/02/27 01:18:39	1.77
+++ remote.c	2002/03/10 18:13:05
@@ -55,6 +55,8 @@
 
 #include "gdbcore.h" /* for exec_bfd */
 
+#define WAIT_FOREVER_FLAG 0
+
 /* Prototypes for local functions */
 static void cleanup_sigint_signal_handler (void *dummy);
 static void initialize_sigint_signal_handler (void);
@@ -1032,7 +1034,7 @@
   else
     sprintf (&buf[2], "%x", th);
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
   if (gen)
     general_thread = th;
   else
@@ -1052,7 +1054,7 @@
   else
     sprintf (buf, "T%08x", tid);
   putpkt (buf);
-  getpkt (buf, sizeof (buf), 0);
+  getpkt (buf, sizeof (buf), WAIT_FOREVER_FLAG);
   return (buf[0] == 'O' && buf[1] == 'K');
 }
 
@@ -1550,7 +1552,7 @@
 
   pack_threadinfo_request (threadinfo_pkt, fieldset, threadid);
   putpkt (threadinfo_pkt);
-  getpkt (threadinfo_pkt, (rs->remote_packet_size), 0);
+  getpkt (threadinfo_pkt, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
   result = remote_unpack_thread_info_response (threadinfo_pkt + 2, threadid,
					       info);
   return result;
@@ -1630,7 +1632,7 @@
   pack_threadlist_request (threadlist_packet,
			   startflag, result_limit, nextthread);
   putpkt (threadlist_packet);
-  getpkt (t_response, (rs->remote_packet_size), 0);
+  getpkt (t_response, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
   *result_count =
     parse_threadlist_response (t_response + 2, result_limit, &echo_nextthread,
@@ -1740,7 +1742,7 @@
   char *buf = alloca (rs->remote_packet_size);
 
   putpkt ("qC");
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
   if (buf[0] == 'Q' && buf[1] == 'C')
     return pid_to_ptid (strtol (&buf[2], NULL, 16));
   else
@@ -1782,7 +1784,7 @@
     {
       putpkt ("qfThreadInfo");
       bufp = buf;
-      getpkt (bufp, (rs->remote_packet_size), 0);
+      getpkt (bufp, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
       if (bufp[0] != '\0')		/* q packet recognized */
	{	
	  while (*bufp++ == 'm')	/* reply contains one or more TID */
@@ -1796,7 +1798,7 @@
	      while (*bufp++ == ',');	/* comma-separated list */
	      putpkt ("qsThreadInfo");
	      bufp = buf;
-	      getpkt (bufp, (rs->remote_packet_size), 0);
+	      getpkt (bufp, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
	    }
	  return;	/* done */
	}
@@ -1837,7 +1839,7 @@
     {
       sprintf (bufp, "qThreadExtraInfo,%x", PIDGET (tp->ptid));
       putpkt (bufp);
-      getpkt (bufp, (rs->remote_packet_size), 0);
+      getpkt (bufp, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
       if (bufp[0] != 0)
	{
	  n = min (strlen (bufp) / 2, sizeof (display_buf));
@@ -1893,7 +1895,7 @@
   /* Now query for status so this looks just like we restarted
      gdbserver from scratch.  */
   putpkt ("?");
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 }
 \f
 /* Clean up connection to a remote debugger.  */
@@ -1921,7 +1923,7 @@
 
   putpkt ("qOffsets");
 
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
   if (buf[0] == '\000')
     return;			/* Return silently.  Stub doesn't support
@@ -2202,7 +2204,7 @@
   /* Invite target to request symbol lookups. */
 
   putpkt ("qSymbol::");
-  getpkt (reply, (rs->remote_packet_size), 0);
+  getpkt (reply, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
   packet_ok (reply, &remote_protocol_qSymbol);
 
   while (strncmp (reply, "qSymbol:", 8) == 0)
@@ -2218,7 +2220,7 @@
		 paddr_nz (SYMBOL_VALUE_ADDRESS (sym)),
		 &reply[8]);
       putpkt (msg);
-      getpkt (reply, (rs->remote_packet_size), 0);
+      getpkt (reply, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
     }
 }
 
@@ -2304,7 +2306,7 @@
       /* Tell the remote that we are using the extended protocol.  */
       char *buf = alloca (rs->remote_packet_size);
       putpkt ("!");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
     }
 #ifdef SOLIB_CREATE_INFERIOR_HOOK
   /* FIXME: need a master target_open vector from which all 
@@ -2418,7 +2420,7 @@
       /* Tell the remote that we are using the extended protocol.  */
       char *buf = alloca (rs->remote_packet_size);
       putpkt ("!");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
     }
 #ifdef SOLIB_CREATE_INFERIOR_HOOK
   /* FIXME: need a master target_open vector from which all 
@@ -2601,7 +2603,7 @@
	      *p++ = 0;
 
	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
		return;
@@ -2619,7 +2621,7 @@
	      *p++ = 0;
 
	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
		return;
@@ -2690,7 +2692,7 @@
	      *p++ = 0;
 
	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
		goto register_event_loop;
@@ -2708,7 +2710,7 @@
	      *p++ = 0;
 
	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
		goto register_event_loop;
@@ -3439,7 +3441,7 @@
       if (remote_debug)
	fprintf_unfiltered (gdb_stdlog,
			    "Bad register packet; fetching a new packet\n");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
     }
 
   /* Reply describes registers byte by byte, each byte encoded as two
@@ -3695,7 +3697,7 @@
	*p = '\0';
	
	putpkt_binary (buf, (int) (p - buf));
-	getpkt (buf, (rs->remote_packet_size), 0);
+	getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
	if (buf[0] == '\0')
	  {
@@ -3837,7 +3839,7 @@
     }
   
   putpkt_binary (buf, (int) (p - buf));
-  getpkt (buf, sizeof_buf, 0);
+  getpkt (buf, sizeof_buf, WAIT_FOREVER_FLAG);
   
   if (buf[0] == 'E')
     {
@@ -3902,7 +3904,7 @@
       *p = '\0';
 
       putpkt (buf);
-      getpkt (buf, sizeof_buf, 0);
+      getpkt (buf, sizeof_buf, WAIT_FOREVER_FLAG);
 
       if (buf[0] == 'E')
	{
@@ -3980,7 +3982,7 @@
       data_long = extract_unsigned_integer (data, len);
       sprintf (buf, "t%x:%x,%x", startaddr, data_long, mask_long);
       putpkt (buf);
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
       if (buf[0] == '\0')
	{
	  /* The stub doesn't support the 't' request.	We might want to
@@ -4069,7 +4071,7 @@
	     long sizeof_buf)
 {
   putpkt (buf);
-  getpkt (buf, sizeof_buf, 0);
+  getpkt (buf, sizeof_buf, WAIT_FOREVER_FLAG);
 
   if (buf[0] == 'E')
     error ("Remote failure reply: %s", buf);
@@ -4658,7 +4660,7 @@
       sprintf (p, ",%d", bp_size);
       
       putpkt (buf);
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
       switch (packet_ok (buf, &remote_protocol_Z[Z_PACKET_SOFTWARE_BP]))
	{
@@ -4711,7 +4713,7 @@
       sprintf (p, ",%d", bp_size);
       
       putpkt (buf);
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
       return (buf[0] == 'E');
     }
@@ -4766,7 +4768,7 @@
   sprintf (p, ",%x", len);
   
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
   switch (packet_ok (buf, &remote_protocol_Z[packet]))
     {
@@ -4802,7 +4804,7 @@
   p += hexnumstr (p, (ULONGEST) addr);
   sprintf (p, ",%x", len);
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
   switch (packet_ok (buf, &remote_protocol_Z[packet]))
     {
@@ -4840,7 +4842,7 @@
   sprintf (p, ",%x", len);
 
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
 
   switch (packet_ok (buf, &remote_protocol_Z[Z_PACKET_HARDWARE_BP]))
     {
@@ -4878,7 +4880,7 @@
   sprintf (p, ",%x", len);
 
   putpkt(buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
   
   switch (packet_ok (buf, &remote_protocol_Z[Z_PACKET_HARDWARE_BP]))
     {
@@ -5011,7 +5013,7 @@
       bfd_get_section_contents (exec_bfd, s, sectdata, 0, size);
       host_crc = crc32 ((unsigned char *) sectdata, size, 0xffffffff);
 
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
       if (buf[0] == 'E')
	error ("target memory fault, section %s, range 0x%08x -- 0x%08x",
	       sectname, lma, lma + size);
@@ -5102,7 +5104,7 @@
   if (i < 0)
     return i;
 
-  getpkt (outbuf, *bufsiz, 0);
+  getpkt (outbuf, *bufsiz, WAIT_FOREVER_FLAG);
 
   return 0;
 }
@@ -5141,7 +5143,7 @@
     {
       /* XXX - see also tracepoint.c:remote_get_noisy_reply() */
       buf[0] = '\0';
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
       if (buf[0] == '\0')
	error ("Target does not support this command\n");
       if (buf[0] == 'O' && buf[1] != 'K')
@@ -5182,7 +5184,7 @@
   puts_filtered ("\n");
   putpkt (args);
 
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
   puts_filtered ("received: ");
   print_packet (buf);
   puts_filtered ("\n");
@@ -5430,7 +5432,7 @@
     error ("Command can only be used when connected to the remote target.");
 
   putpkt ("qfProcessInfo");
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
   if (buf[0] == 0)
     return;			/* Silently: target does not support this feature. */
 
@@ -5441,7 +5443,7 @@
     {
       remote_console_output (&buf[1]);
       putpkt ("qsProcessInfo");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), WAIT_FOREVER_FLAG);
     }
 }
 

Index: gdbserver/remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.8
diff -u -r1.8 remote-utils.c
--- remote-utils.c	2002/02/14 06:21:22	1.8
+++ remote-utils.c	2002/03/10 18:13:05
@@ -47,7 +47,10 @@
 void
 remote_open (char *name)
 {
-  int save_fcntl_flags;
+  int save_fcntl_flags, via_network=0;;
+  char namebuf[100];
+  
+  snprintf(namebuf, 100, "%s", name);
 
   if (!strchr (name, ':'))
     {
@@ -109,6 +112,7 @@
       int tmp;
       struct protoent *protoent;
       int tmp_desc;
+      struct hostent *hostent;
 
       port_str = strchr (name, ':');
 
@@ -154,6 +158,21 @@
 
       signal (SIGPIPE, SIG_IGN);	/* If we don't do this, then gdbserver simply
					   exits when the remote side dies.  */
+
+      /* Resolve peer's hostname or convert IP address to string.  */
+      hostent = gethostbyaddr ( (void *)&sockaddr.sin_addr.s_addr, 
+			 sizeof (sockaddr.sin_addr.s_addr), AF_INET);
+      if (hostent)
+	 snprintf(namebuf, 100, "%s", hostent->h_name);
+      else
+      {
+	 union { unsigned char ip_c[4]; int ip_i; } ip;
+	
+	 ip.ip_i = ntohl (sockaddr.sin_addr.s_addr);
+	 snprintf (namebuf, 100, "%u.%u.%u.%u", 
+	   ip.ip_c[3], ip.ip_c[2], ip.ip_c[1], ip.ip_c[0]);
+      }
+      via_network = 1;
     }
 
 #if defined(F_SETFL) && defined (FASYNC)
@@ -164,7 +184,8 @@
 #endif
 #endif
   disable_async_io ();
-  fprintf (stderr, "Remote debugging using %s\n", name);
+
+  fprintf (stderr, "Remote debugging %s %s\n", via_network?"from host":"using", namebuf);
 }
 
 void

Re: [RFA] remote debugging patches

[Daniel Jacobowitz]

On Sun, Mar 10, 2002 at 07:34:07PM +0100, Michal Ludvig wrote:
> Hi,
> Here are two patches for remote debugging. The first one is pretty 
> straightforward and just makes use of the last parameter to getpkt().
> The second one changes banner printed after successful connection to 
> gdbserver from "Remote debugging using :1234" to more informational
> "Remote debugging from host whatever.name.com". When debugging via 
> serial line, nothing changes.

Except that this introduces a second use of the NSS functions, while I
keep meaning to remove the first (fairly recent) one.  Please don't do
that; I want to keep gdbserver independent of the sick and disgusting
dynamic loading mess that is name lookup under glibc.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote debugging patches

[Michal Ludvig]

Daniel Jacobowitz wrote:
> Except that this introduces a second use of the NSS functions, while I
> keep meaning to remove the first (fairly recent) one.  Please don't do
> that; I want to keep gdbserver independent of the sick and disgusting
> dynamic loading mess that is name lookup under glibc.

OK, so what about just printing out the IP address? Or make it a 
compile-time option wrapped by #ifdef .. #endif (with default off)?

And what about the first patch to remote.c?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

Re: [RFA] remote debugging patches

[Daniel Jacobowitz]

On Sun, Mar 10, 2002 at 08:43:09PM +0100, Michal Ludvig wrote:
> Daniel Jacobowitz wrote:
> >Except that this introduces a second use of the NSS functions, while I
> >keep meaning to remove the first (fairly recent) one.  Please don't do
> >that; I want to keep gdbserver independent of the sick and disgusting
> >dynamic loading mess that is name lookup under glibc.
> 
> OK, so what about just printing out the IP address? Or make it a 
> compile-time option wrapped by #ifdef .. #endif (with default off)?

I'd prefer just IP address.

> And what about the first patch to remote.c?

Neither is up to me, but I've no objection.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote debugging patches

[Michal Ludvig]

Daniel Jacobowitz wrote:
> I'd prefer just IP address.

What about using libresolv to make an arbitrary DNS query? This way 
libnss shouldn't be linked nor dlopen()ed and only one more library 
would be used.
Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

Re: [RFA] remote debugging patches

[Daniel Jacobowitz]

On Mon, Mar 11, 2002 at 03:54:12PM +0100, Michal Ludvig wrote:
> Daniel Jacobowitz wrote:
> >I'd prefer just IP address.
> 
> What about using libresolv to make an arbitrary DNS query? This way 
> libnss shouldn't be linked nor dlopen()ed and only one more library 
> would be used.

Because then:
  - libresolv is necessary
  - making an arbitrary DNS query is complicated

I really don't see the point; printing out the IP address should be
enough information.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote debugging patches

[Andrew Cagney]

Just FYI,

Regarding the proposed gdb/gdbserver/ change, once you two have stopped 
tossing cream pies at each other [:-^] and reached a decision, it is ok 
by me :-)

enjoy,
Andrew

Re: [RFA] remote debugging patches

[Daniel Jacobowitz]

On Mon, Mar 11, 2002 at 09:21:58PM -0500, Andrew Cagney wrote:
> Just FYI,
> 
> Regarding the proposed gdb/gdbserver/ change, once you two have stopped 
> tossing cream pies at each other [:-^] and reached a decision, it is ok 
> by me :-)

:)

Michal, are you OK with just the IP address?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote debugging patches

[Michal Ludvig]

Daniel Jacobowitz wrote:
> Michal, are you OK with just the IP address?

Sure I am :-) Patch follows... Is it OK?

Index: ChangeLog
	* remote-utils.c (remote_open): Print remote-side's
	IP address when remote debugging over the network.

Index: remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.8
diff -c -3 -p -r1.8 remote-utils.c
*** remote-utils.c      2002/02/14 06:21:22     1.8
--- remote-utils.c      2002/03/13 14:11:16
*************** static int remote_desc;
*** 47,53 ****
   void
   remote_open (char *name)
   {
!   int save_fcntl_flags;

     if (!strchr (name, ':'))
       {
--- 47,56 ----
   void
   remote_open (char *name)
   {
!   int save_fcntl_flags, via_network=0;;
!   char namebuf[100];
!
!   snprintf(namebuf, 100, "%s", name);

     if (!strchr (name, ':'))
       {
*************** remote_open (char *name)
*** 154,159 ****
--- 157,166 ----

         signal (SIGPIPE, SIG_IGN);      /* If we don't do this, then 
gdbserver simply
                                            exits when the remote side 
dies.  */
+
+       /* Convert IP address to string.  */
+       snprintf (namebuf, 100, "%s", inet_ntoa (sockaddr.sin_addr.s_addr));
+       via_network = 1;
       }

   #if defined(F_SETFL) && defined (FASYNC)
*************** remote_open (char *name)
*** 164,170 ****
   #endif
   #endif
     disable_async_io ();
!   fprintf (stderr, "Remote debugging using %s\n", name);
   }

   void
--- 171,179 ----
   #endif
   #endif
     disable_async_io ();
!
!   fprintf (stderr, "Remote debugging %s %s\n",
!     via_network ? "from host" : "using", namebuf);
   }

   void

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

Re: [RFA] remote debugging patches

[Andreas Schwab]

Michal Ludvig <mludvig@suse.cz> writes:

|> Daniel Jacobowitz wrote:
|> > Michal, are you OK with just the IP address?
|> 
|> Sure I am :-) Patch follows... Is it OK?
|> 
|> Index: ChangeLog
|> 	* remote-utils.c (remote_open): Print remote-side's
|> 	IP address when remote debugging over the network.
|> 
|> Index: remote-utils.c
|> ===================================================================
|> RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
|> retrieving revision 1.8
|> diff -c -3 -p -r1.8 remote-utils.c
|> *** remote-utils.c      2002/02/14 06:21:22     1.8
|> --- remote-utils.c      2002/03/13 14:11:16
|> *************** static int remote_desc;
|> *** 47,53 ****
|>    void
|>    remote_open (char *name)
|>    {
|> !   int save_fcntl_flags;
|> 
|>      if (!strchr (name, ':'))
|>        {
|> --- 47,56 ----
|>    void
|>    remote_open (char *name)
|>    {
|> !   int save_fcntl_flags, via_network=0;;
|> !   char namebuf[100];
|> !
|> !   snprintf(namebuf, 100, "%s", name);

Why are you truncating the name to 100 characters?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] remote debugging patches

[Michal Ludvig]

Andreas Schwab wrote:
> Why are you truncating the name to 100 characters?

It is just an informational message and 100 chars shoud be enough almost 
everytime.
1) IPv4 (or IPv6 sometime in the future) address can't be as long.
2) I didn't expect to have a resolved domainname such long.
3) Neither serial port names are so long.
4) I take care about buffer-overruns using snprintf.

Should I change it?
Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

Re: [RFA] remote debugging patches

[Andrew Cagney]

> 4) I take care about buffer-overruns using snprintf.

Definitly worry about this (and is nice to see someone thinking about 
it!) (suggest changing 100 to sizeof (buf)).

Andrew

Re: [RFA] remote debugging patches

[Andreas Schwab]

Michal Ludvig <mludvig@suse.cz> writes:

|> Andreas Schwab wrote:
|> > Why are you truncating the name to 100 characters?
|> 
|> It is just an informational message and 100 chars shoud be enough almost
|> everytime.
|> 1) IPv4 (or IPv6 sometime in the future) address can't be as long.
|> 2) I didn't expect to have a resolved domainname such long.
|> 3) Neither serial port names are so long.
|> 4) I take care about buffer-overruns using snprintf.

Maybe it's just me, but I hate arbitrary limits.  There is no need for
sprintf at this point anyway.  I'd suggest to define another variable that
either points to name or to the result of inet_ntoa and use that in the
fprintf call.  No need for namebuf either.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] remote debugging patches

[Michal Ludvig]

Andreas Schwab wrote:
> Maybe it's just me, but I hate arbitrary limits.  There is no need for
> sprintf at this point anyway.  I'd suggest to define another variable that
> either points to name or to the result of inet_ntoa and use that in the
> fprintf call.  No need for namebuf either.

OK, the 100-chars limit is removed and the patch is getting smaller and 
smaller :-)) Do you like it more this way, Andreas?
Index: gdbserver/remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.8
diff -c -3 -p -B -b -r1.8 remote-utils.c
*** remote-utils.c      2002/02/14 06:21:22     1.8
--- remote-utils.c      2002/03/13 19:05:33
*************** remote_open (char *name)
*** 99,105 ****
         }
   #endif

!
       }
     else
       {
--- 99,105 ----
         }
   #endif

!       fprintf (stderr, "Remote debugging using %s\n", name);
       }
     else
       {
*************** remote_open (char *name)
*** 154,159 ****
--- 154,163 ----

         signal (SIGPIPE, SIG_IGN);      /* If we don't do this, then 
gdbserver simply
                                            exits when the remote side 
dies.  */
+
+       /* Convert IP address to string.  */
+       fprintf (stderr, "Remote debugging from host %s\n",
+          inet_ntoa (sockaddr.sin_addr));
       }

   #if defined(F_SETFL) && defined (FASYNC)
*************** remote_open (char *name)
*** 164,170 ****
   #endif
   #endif
     disable_async_io ();
-   fprintf (stderr, "Remote debugging using %s\n", name);
   }

   void
--- 168,173 ----

Re: [RFA] remote debugging patches

[Andreas Schwab]

Michal Ludvig <mludvig@suse.cz> writes:

|> Andreas Schwab wrote:
|> > Maybe it's just me, but I hate arbitrary limits.  There is no need for
|> > sprintf at this point anyway.  I'd suggest to define another variable that
|> > either points to name or to the result of inet_ntoa and use that in the
|> > fprintf call.  No need for namebuf either.
|> 
|> OK, the 100-chars limit is removed and the patch is getting smaller and
|> smaller :-)) Do you like it more this way, Andreas?

Perfect. :-)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] remote debugging patches

[Daniel Jacobowitz]

On Wed, Mar 13, 2002 at 03:13:58PM +0100, Michal Ludvig wrote:
> Daniel Jacobowitz wrote:
> >Michal, are you OK with just the IP address?
> 
> Sure I am :-) Patch follows... Is it OK?

Fine by me once the 100-character thing is settled.  I think you'll have
enough approvals on this patch now to commit it :)

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote debugging patches

[Andrew Cagney]

Michal, as an aside, these are independant changes, it is really helpful 
if they are submitted separatly (making review much easier).

    * remote.c: Added WAIT_FOREVER_FLAG and changed all
     calls to getpkt() to respect it.

> +#define WAIT_FOREVER_FLAG 0

Yes fine, er almost.  Can you change the name of this to 
``DONT_WAIT_FOREVER_FLAG'' and suggest making it an enum. (so GDB can 
print it :-)

If you're feeling really inspired (...), you could even introduce an 
enum to handle both the DO and DONT cases.

Andrew

Re: [RFA] remote debugging patches

[Michal Ludvig]

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

Andrew Cagney wrote:
> Yes fine, er almost.  Can you change the name of this to 
> ``DONT_WAIT_FOREVER_FLAG'' and suggest making it an enum. (so GDB can 
> print it :-)

Why DONT_WAIT_FOREWER? IMHO wait_forewer is correct, because 0 means 
don't wait forewer (the default) and 1 means yes, wait forever.

> If you're feeling really inspired (...), you could even introduce an 
> enum to handle both the DO and DONT cases.

OK, I changed it to enum called wait_forever_flag with values yes and 
no. Now it should be clear enough whether to wait or not. :-)
Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

[-- Attachment #2: gdb-remote-03111602.diff --]
[-- Type: text/plain, Size: 11057 bytes --]

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.77
diff -u -r1.77 remote.c
--- remote.c	2002/02/27 01:18:39	1.77
+++ remote.c	2002/03/11 15:02:18
@@ -55,6 +55,9 @@
 
 #include "gdbcore.h" /* for exec_bfd */
 
+/* Set this default to 'yes' when you don't want to timeout getpkt() */
+static enum { no, yes } wait_forever_flag=no;
+
 /* Prototypes for local functions */
 static void cleanup_sigint_signal_handler (void *dummy);
 static void initialize_sigint_signal_handler (void);
@@ -1032,7 +1035,7 @@
   else
     sprintf (&buf[2], "%x", th);
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
   if (gen)
     general_thread = th;
   else
@@ -1052,7 +1055,7 @@
   else
     sprintf (buf, "T%08x", tid);
   putpkt (buf);
-  getpkt (buf, sizeof (buf), 0);
+  getpkt (buf, sizeof (buf), wait_forever_flag);
   return (buf[0] == 'O' && buf[1] == 'K');
 }
 
@@ -1550,7 +1553,7 @@
 
   pack_threadinfo_request (threadinfo_pkt, fieldset, threadid);
   putpkt (threadinfo_pkt);
-  getpkt (threadinfo_pkt, (rs->remote_packet_size), 0);
+  getpkt (threadinfo_pkt, (rs->remote_packet_size), wait_forever_flag);
   result = remote_unpack_thread_info_response (threadinfo_pkt + 2, threadid,
 					       info);
   return result;
@@ -1630,7 +1633,7 @@
   pack_threadlist_request (threadlist_packet,
 			   startflag, result_limit, nextthread);
   putpkt (threadlist_packet);
-  getpkt (t_response, (rs->remote_packet_size), 0);
+  getpkt (t_response, (rs->remote_packet_size), wait_forever_flag);
 
   *result_count =
     parse_threadlist_response (t_response + 2, result_limit, &echo_nextthread,
@@ -1740,7 +1743,7 @@
   char *buf = alloca (rs->remote_packet_size);
 
   putpkt ("qC");
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
   if (buf[0] == 'Q' && buf[1] == 'C')
     return pid_to_ptid (strtol (&buf[2], NULL, 16));
   else
@@ -1782,7 +1785,7 @@
     {
       putpkt ("qfThreadInfo");
       bufp = buf;
-      getpkt (bufp, (rs->remote_packet_size), 0);
+      getpkt (bufp, (rs->remote_packet_size), wait_forever_flag);
       if (bufp[0] != '\0')		/* q packet recognized */
 	{	
 	  while (*bufp++ == 'm')	/* reply contains one or more TID */
@@ -1796,7 +1799,7 @@
 	      while (*bufp++ == ',');	/* comma-separated list */
 	      putpkt ("qsThreadInfo");
 	      bufp = buf;
-	      getpkt (bufp, (rs->remote_packet_size), 0);
+	      getpkt (bufp, (rs->remote_packet_size), wait_forever_flag);
 	    }
 	  return;	/* done */
 	}
@@ -1837,7 +1840,7 @@
     {
       sprintf (bufp, "qThreadExtraInfo,%x", PIDGET (tp->ptid));
       putpkt (bufp);
-      getpkt (bufp, (rs->remote_packet_size), 0);
+      getpkt (bufp, (rs->remote_packet_size), wait_forever_flag);
       if (bufp[0] != 0)
 	{
 	  n = min (strlen (bufp) / 2, sizeof (display_buf));
@@ -1893,7 +1896,7 @@
   /* Now query for status so this looks just like we restarted
      gdbserver from scratch.  */
   putpkt ("?");
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 }
 \f
 /* Clean up connection to a remote debugger.  */
@@ -1921,7 +1924,7 @@
 
   putpkt ("qOffsets");
 
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
   if (buf[0] == '\000')
     return;			/* Return silently.  Stub doesn't support
@@ -2202,7 +2205,7 @@
   /* Invite target to request symbol lookups. */
 
   putpkt ("qSymbol::");
-  getpkt (reply, (rs->remote_packet_size), 0);
+  getpkt (reply, (rs->remote_packet_size), wait_forever_flag);
   packet_ok (reply, &remote_protocol_qSymbol);
 
   while (strncmp (reply, "qSymbol:", 8) == 0)
@@ -2218,7 +2221,7 @@
 		 paddr_nz (SYMBOL_VALUE_ADDRESS (sym)),
 		 &reply[8]);
       putpkt (msg);
-      getpkt (reply, (rs->remote_packet_size), 0);
+      getpkt (reply, (rs->remote_packet_size), wait_forever_flag);
     }
 }
 
@@ -2304,7 +2307,7 @@
       /* Tell the remote that we are using the extended protocol.  */
       char *buf = alloca (rs->remote_packet_size);
       putpkt ("!");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
     }
 #ifdef SOLIB_CREATE_INFERIOR_HOOK
   /* FIXME: need a master target_open vector from which all 
@@ -2418,7 +2421,7 @@
       /* Tell the remote that we are using the extended protocol.  */
       char *buf = alloca (rs->remote_packet_size);
       putpkt ("!");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
     }
 #ifdef SOLIB_CREATE_INFERIOR_HOOK
   /* FIXME: need a master target_open vector from which all 
@@ -2601,7 +2604,7 @@
 	      *p++ = 0;
 
 	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
 	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
 		return;
@@ -2619,7 +2622,7 @@
 	      *p++ = 0;
 
 	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
 	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
 		return;
@@ -2690,7 +2693,7 @@
 	      *p++ = 0;
 
 	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
 	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
 		goto register_event_loop;
@@ -2708,7 +2711,7 @@
 	      *p++ = 0;
 
 	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+	      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
 	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
 		goto register_event_loop;
@@ -3439,7 +3442,7 @@
       if (remote_debug)
 	fprintf_unfiltered (gdb_stdlog,
 			    "Bad register packet; fetching a new packet\n");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
     }
 
   /* Reply describes registers byte by byte, each byte encoded as two
@@ -3695,7 +3698,7 @@
 	*p = '\0';
 	
 	putpkt_binary (buf, (int) (p - buf));
-	getpkt (buf, (rs->remote_packet_size), 0);
+	getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
 	if (buf[0] == '\0')
 	  {
@@ -3837,7 +3840,7 @@
     }
   
   putpkt_binary (buf, (int) (p - buf));
-  getpkt (buf, sizeof_buf, 0);
+  getpkt (buf, sizeof_buf, wait_forever_flag);
   
   if (buf[0] == 'E')
     {
@@ -3902,7 +3905,7 @@
       *p = '\0';
 
       putpkt (buf);
-      getpkt (buf, sizeof_buf, 0);
+      getpkt (buf, sizeof_buf, wait_forever_flag);
 
       if (buf[0] == 'E')
 	{
@@ -3980,7 +3983,7 @@
       data_long = extract_unsigned_integer (data, len);
       sprintf (buf, "t%x:%x,%x", startaddr, data_long, mask_long);
       putpkt (buf);
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
       if (buf[0] == '\0')
 	{
 	  /* The stub doesn't support the 't' request.  We might want to
@@ -4069,7 +4072,7 @@
 	     long sizeof_buf)
 {
   putpkt (buf);
-  getpkt (buf, sizeof_buf, 0);
+  getpkt (buf, sizeof_buf, wait_forever_flag);
 
   if (buf[0] == 'E')
     error ("Remote failure reply: %s", buf);
@@ -4658,7 +4661,7 @@
       sprintf (p, ",%d", bp_size);
       
       putpkt (buf);
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
       switch (packet_ok (buf, &remote_protocol_Z[Z_PACKET_SOFTWARE_BP]))
 	{
@@ -4711,7 +4714,7 @@
       sprintf (p, ",%d", bp_size);
       
       putpkt (buf);
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
       return (buf[0] == 'E');
     }
@@ -4766,7 +4769,7 @@
   sprintf (p, ",%x", len);
   
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
   switch (packet_ok (buf, &remote_protocol_Z[packet]))
     {
@@ -4802,7 +4805,7 @@
   p += hexnumstr (p, (ULONGEST) addr);
   sprintf (p, ",%x", len);
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
   switch (packet_ok (buf, &remote_protocol_Z[packet]))
     {
@@ -4840,7 +4843,7 @@
   sprintf (p, ",%x", len);
 
   putpkt (buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
 
   switch (packet_ok (buf, &remote_protocol_Z[Z_PACKET_HARDWARE_BP]))
     {
@@ -4878,7 +4881,7 @@
   sprintf (p, ",%x", len);
 
   putpkt(buf);
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
   
   switch (packet_ok (buf, &remote_protocol_Z[Z_PACKET_HARDWARE_BP]))
     {
@@ -5011,7 +5014,7 @@
       bfd_get_section_contents (exec_bfd, s, sectdata, 0, size);
       host_crc = crc32 ((unsigned char *) sectdata, size, 0xffffffff);
 
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
       if (buf[0] == 'E')
 	error ("target memory fault, section %s, range 0x%08x -- 0x%08x",
 	       sectname, lma, lma + size);
@@ -5102,7 +5105,7 @@
   if (i < 0)
     return i;
 
-  getpkt (outbuf, *bufsiz, 0);
+  getpkt (outbuf, *bufsiz, wait_forever_flag);
 
   return 0;
 }
@@ -5141,7 +5144,7 @@
     {
       /* XXX - see also tracepoint.c:remote_get_noisy_reply() */
       buf[0] = '\0';
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
       if (buf[0] == '\0')
 	error ("Target does not support this command\n");
       if (buf[0] == 'O' && buf[1] != 'K')
@@ -5182,7 +5185,7 @@
   puts_filtered ("\n");
   putpkt (args);
 
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
   puts_filtered ("received: ");
   print_packet (buf);
   puts_filtered ("\n");
@@ -5430,7 +5433,7 @@
     error ("Command can only be used when connected to the remote target.");
 
   putpkt ("qfProcessInfo");
-  getpkt (buf, (rs->remote_packet_size), 0);
+  getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
   if (buf[0] == 0)
     return;			/* Silently: target does not support this feature. */
 
@@ -5441,7 +5444,7 @@
     {
       remote_console_output (&buf[1]);
       putpkt ("qsProcessInfo");
-      getpkt (buf, (rs->remote_packet_size), 0);
+      getpkt (buf, (rs->remote_packet_size), wait_forever_flag);
     }
 }
 

Re: [RFA] remote debugging patches

[Daniel Jacobowitz]

On Mon, Mar 11, 2002 at 04:08:19PM +0100, Michal Ludvig wrote:
> Andrew Cagney wrote:
> >Yes fine, er almost.  Can you change the name of this to 
> >``DONT_WAIT_FOREVER_FLAG'' and suggest making it an enum. (so GDB can 
> >print it :-)
> 
> Why DONT_WAIT_FOREWER? IMHO wait_forewer is correct, because 0 means 
> don't wait forewer (the default) and 1 means yes, wait forever.

But you had #define WAIT_FOREVER_FLAG 0.

> >If you're feeling really inspired (...), you could even introduce an 
> >enum to handle both the DO and DONT cases.
> 
> OK, I changed it to enum called wait_forever_flag with values yes and 
> no. Now it should be clear enough whether to wait or not. :-)

- "no" and "yes" are useless values for a flag; they don't indicate any
meaning.

- You made wait_forever_flag a variable that was never changed, and
replaced a constant 0 with it... no point.

I think what Andrew had in mind was more like

enum {
  do_not_wait_forever = 0,
  wait_forever = 1
};

and change calls to
  getpkt (blah, do_not_wait_forever)


-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote debugging patches

[Michal Ludvig]

Daniel Jacobowitz wrote:
> But you had #define WAIT_FOREVER_FLAG 0.

During the weekend I was debugging problems in communication between 
gdb(i386) and gdbserver(x8664). Whenever I was stepping through 
gdbserver, the other side timeouted. I've found, that last argument to 
getpkt() is called 'forever', but in all calls was set to '0'. I didn't 
want these timeouts, so I changed all occurences of 0 to 
WAIT_FOREVER_FLAG, which could be set in compile-time. Most users and 
developpers (unless they will work on remote.c or alike) will leave this 
unchanged to 0, but sometimes it may be handy to set to 1 and recompile.

> - "no" and "yes" are useless values for a flag; they don't indicate any
> meaning.

Along with the name of the variable they have a meaning, I think. Of 
course, I can change them to 'wait'/'nowait' or 'forewer'/'timeout' or 
whatever else.

> - You made wait_forever_flag a variable that was never changed, and
> replaced a constant 0 with it... no point.

Who cares? In the debugger you'll see 'yes' or 'no' when you ask for the 
content of wait_forever_flag. Or do 'print /d wait_forever_flag' and you 
will se the decimal value.

> I think what Andrew had in mind was more like
> 
> enum {
>   do_not_wait_forever = 0,
>   wait_forever = 1
> };
> 
> and change calls to
>   getpkt (blah, do_not_wait_forever)

I don't understand the point of this :-( Then I'd have to change all 
occurences of do_not_wait_forever to wait_forever in the whole file to 
change the behaviour? Isn't it much easier to change just one line on 
top of the file from 'no' to 'yes' instead?
Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

Re: [RFA] remote debugging patches

[Daniel Jacobowitz]

On Mon, Mar 11, 2002 at 05:06:51PM +0100, Michal Ludvig wrote:
> Daniel Jacobowitz wrote:
> >But you had #define WAIT_FOREVER_FLAG 0.
> 
> During the weekend I was debugging problems in communication between 
> gdb(i386) and gdbserver(x8664). Whenever I was stepping through 
> gdbserver, the other side timeouted. I've found, that last argument to 
> getpkt() is called 'forever', but in all calls was set to '0'. I didn't 
> want these timeouts, so I changed all occurences of 0 to 
> WAIT_FOREVER_FLAG, which could be set in compile-time. Most users and 
> developpers (unless they will work on remote.c or alike) will leave this 
> unchanged to 0, but sometimes it may be handy to set to 1 and recompile.

> >I think what Andrew had in mind was more like
> >
> >enum {
> >  do_not_wait_forever = 0,
> >  wait_forever = 1
> >};
> >
> >and change calls to
> >  getpkt (blah, do_not_wait_forever)
> 
> I don't understand the point of this :-( Then I'd have to change all 
> occurences of do_not_wait_forever to wait_forever in the whole file to 
> change the behaviour? Isn't it much easier to change just one line on 
> top of the file from 'no' to 'yes' instead?

Ah!  This is why you should say clearly what you're trying to
accomplish instead of just posting a patch :)

The right thing to do here is more like:
  - Create the enum, as Andrew or I described it
  - Create the global variable, with a name that does not suggest its
    value, only its meaning.  Something like ``getpkt_default_timeout_flag''.
  - Change all appropriate calls to use the variable.
  - Add ``set remote wait-forever on/off'' to update the variable.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote debugging patches

[Michal Ludvig]

Daniel Jacobowitz wrote:
> The right thing to do here is more like:
>   - Create the enum, as Andrew or I described it
>   - Create the global variable, with a name that does not suggest its
>     value, only its meaning.  Something like ``getpkt_default_timeout_flag''.
>   - Change all appropriate calls to use the variable.
>   - Add ``set remote wait-forever on/off'' to update the variable.

Good! Now we understand eachother :-)
I will do it this way. Thanks for your patience. I will send the patch 
later.
Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

Re: [RFA] remote debugging patches

[Andrew Cagney]

> During the weekend I was debugging problems in communication between gdb(i386) and gdbserver(x8664). Whenever I was stepping through gdbserver, the other side timeouted. I've found, that last argument to getpkt() is called 'forever', but in all calls was set to '0'. I didn't want these timeouts, so I changed all occurences of 0 to WAIT_FOREVER_FLAG, which could be set in compile-time. Most users and developpers (unless they will work on remote.c or alike) will leave this unchanged to 0, but sometimes it may be handy to set to 1 and recompile.

It is called with both 0 and 1.  The target_wait() and target_open() (?) 
code waits forever when in synchronous mode.

> Who cares? In the debugger you'll see 'yes' or 'no' when you ask for the content of wait_forever_flag. Or do 'print /d wait_forever_flag' and you will se the decimal value.

If you found it necessary to change that parameter, then there is a good 
chance that someone else will!

In the ``bad old days'' GDB was full of #ifdef DEBUG_SOMETHING_OBSCURE 
code that ment re-compiling whenever you wanted to tweak something (the 
ARI counts these).  Such code should either be replaced with run-time 
flags such as ``set debug ...'' or deleted.

In your case, check what Daniel wrote.

> enum {
>   do_not_wait_forever = 0,
>   wait_forever = 1
> };

See my post.

enjoy,
Andrew

Re: [RFA] remote debugging patches

[Andrew Cagney]

> Andrew Cagney wrote:
> Yes fine, er almost.  Can you change the name of this to ``DONT_WAIT_FOREVER_FLAG'' and suggest making it an enum. (so GDB can print it :-)
> 
> Why DONT_WAIT_FOREWER? IMHO wait_forewer is correct, because 0 means don't wait forewer (the default) and 1 means yes, wait forever.

But you defined:

> +#define WAIT_FOREVER_FLAG 0

Anyway, I think I misunderstood the reason for the change:

> The first one is pretty straightforward and just makes use of the last parameter to getpkt().

As far as I can tell, you've just replaced the last parameter of 
getpkt() with a hardwired value - initially a macro but now an enum.  Is 
that the intent?

When I wrote:

> If you're feeling really inspired (...), you could even introduce an enum to handle both the DO and DONT cases.

I was thinking of something more like:

enum { do_wait_forever, dont_wait_forever } wait_forever_flag;

int
getpkt (..., enum wait_forever_flag forever, ...)
{
	if (forever == do_wait_forever)
	  ...
	else
	  ...
	et.al.
}

{
	getpkt (blah, dont_wait_forever);
}
Andrew

Re: [RFA] remote debugging patches

[Michal Ludvig]

Andrew Cagney wrote:
>> The first one is pretty straightforward and just makes use of the last 
>> parameter to getpkt().
> 
> As far as I can tell, you've just replaced the last parameter of 
> getpkt() with a hardwired value - initially a macro but now an enum.  Is 
> that the intent?

Yes, it was. I explained my reasons for this change more completely in a 
reply to Daniel Jacobowitz. I will consider reworking the code for using 
'set debug whatever' command as he stated.

 > It is called with both 0 and 1.  The target_wait() and target_open()
 > (?) code waits forever when in synchronous mode.

On a place where I needed it there was a hardwired '0' so I did the 
change. Of course I'm not going to fight for this patch. I'll better 
rework it.
Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz