[RFC,1/2] Remove linux-waitpid.c debugging code

Message ID 20190530213046.20542-2-tom@tromey.com
State New
Headers show
Series
  • Let's discuss moving gdbserver to top-level
Related show

Commit Message

Tom Tromey May 30, 2019, 9:30 p.m.
The debugging code in linux-waitpid.c is one of the few remaining
spots that depends on the gdb/gdbserver difference.

My first thought was that this code is not extremely useful, so this
patch removes this code.  (However, if it is actually useful to
someone, we could make it work by introducing a new abstraction.)

gdb/ChangeLog
2019-05-30  Tom Tromey  <tom@tromey.com>

	* nat/linux-waitpid.c: Don't include server.h.
	(linux_debug): Remove.
	(my_waitpid): Update.
---
 gdb/ChangeLog           |  6 ++++++
 gdb/nat/linux-waitpid.c | 34 +---------------------------------
 2 files changed, 7 insertions(+), 33 deletions(-)

-- 
2.17.2

Comments

Simon Marchi June 3, 2019, 2:57 p.m. | #1
On 2019-05-30 5:30 p.m., Tom Tromey wrote:
> The debugging code in linux-waitpid.c is one of the few remaining

> spots that depends on the gdb/gdbserver difference.

> 

> My first thought was that this code is not extremely useful, so this

> patch removes this code.  (However, if it is actually useful to

> someone, we could make it work by introducing a new abstraction.)

> 

> gdb/ChangeLog

> 2019-05-30  Tom Tromey  <tom@tromey.com>

> 

> 	* nat/linux-waitpid.c: Don't include server.h.

> 	(linux_debug): Remove.

> 	(my_waitpid): Update.

> ---

>  gdb/ChangeLog           |  6 ++++++

>  gdb/nat/linux-waitpid.c | 34 +---------------------------------

>  2 files changed, 7 insertions(+), 33 deletions(-)

> 

> diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c

> index a7d11ab8d32..298032dff9a 100644

> --- a/gdb/nat/linux-waitpid.c

> +++ b/gdb/nat/linux-waitpid.c

> @@ -19,35 +19,10 @@

>  

>  #include "common/common-defs.h"

>  

> -#ifdef GDBSERVER

> -/* FIXME: server.h is required for the definition of debug_threads

> -   which is used in the gdbserver-specific debug printing in

> -   linux_debug.  This code should be made available to GDB also,

> -   but the lack of a suitable flag to enable it prevents this.  */

> -#include "server.h"

> -#endif

> -

>  #include "linux-nat.h"

>  #include "linux-waitpid.h"

>  #include "common/gdb_wait.h"

>  

> -/* Print debugging output based on the format string FORMAT and

> -   its parameters.  */

> -

> -static inline void ATTRIBUTE_PRINTF (1,2)

> -linux_debug (const char *format, ...)

> -{

> -#ifdef GDBSERVER

> -  if (debug_threads)

> -    {

> -      va_list args;

> -      va_start (args, format);

> -      debug_vprintf (format, args);

> -      va_end (args);

> -    }

> -#endif

> -}

> -

>  /* Convert wait status STATUS to a string.  Used for printing debug

>     messages only.  */

>  

> @@ -79,20 +54,13 @@ status_to_str (int status)

>  int

>  my_waitpid (int pid, int *status, int flags)

>  {

> -  int ret, out_errno;

> -

> -  linux_debug ("my_waitpid (%d, 0x%x)\n", pid, flags);

> +  int ret;

>  

>    do

>      {

>        ret = waitpid (pid, status, flags);

>      }

>    while (ret == -1 && errno == EINTR);

> -  out_errno = errno;

> -

> -  linux_debug ("my_waitpid (%d, 0x%x): status(%x), %d\n",

> -	       pid, flags, (ret > 0 && status != NULL) ? *status : -1, ret);

>  

> -  errno = out_errno;

>    return ret;

>  }

> 


Pedro probably has a stronger opinion about this (as he is probably the one who
has spent the most time staring at those logs), but I would think that this
logging is useful when debugging interactions with the kernel.

Simon
Tom Tromey June 3, 2019, 4:32 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


>> My first thought was that this code is not extremely useful, so this

>> patch removes this code.  (However, if it is actually useful to

>> someone, we could make it work by introducing a new abstraction.)


Simon> Pedro probably has a stronger opinion about this (as he is probably the one who
Simon> has spent the most time staring at those logs), but I would think that this
Simon> logging is useful when debugging interactions with the kernel.

I should have mentioned that the reason I figured it wasn't that useful
is that it's not available in gdb proper, only gdbserver.

Tom
Pedro Alves June 5, 2019, 9:32 a.m. | #3
On 6/3/19 5:32 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> 

>>> My first thought was that this code is not extremely useful, so this

>>> patch removes this code.  (However, if it is actually useful to

>>> someone, we could make it work by introducing a new abstraction.)

> 

> Simon> Pedro probably has a stronger opinion about this (as he is probably the one who

> Simon> has spent the most time staring at those logs), but I would think that this

> Simon> logging is useful when debugging interactions with the kernel.

> 

> I should have mentioned that the reason I figured it wasn't that useful

> is that it's not available in gdb proper, only gdbserver.


A bit of history here:

IIRC, the original reason my_waitpid in gdbserver printed the result, is
that I found that useful when I made my_waitpid emulate __WALL:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/linux-low.c;h=ba1d7b46bb96e4cec83ea698e40511d284f16532;hb=bd99dc858385792aad304d42f4a47791fd8d3272#l2110

Over the years, the function moved to gdb/nat/, gdb started using it too,
and then the __WALL emulation disappeared, making my_waitpid a simple
EINTR wrapper again.

Since the emulation is gone, and, both gdb and gdbserver print the result
of waitpid in the most interesting waitpid call spot:

      if (debug_threads)
	debug_printf ("LWFE: waitpid(-1, ...) returned %d, %s\n",
		      ret, errno ? strerror (errno) : "ERRNO-OK");


      if (debug_linux_nat)
	fprintf_unfiltered (gdb_stdlog,
			    "LNW: waitpid(-1, ...) returned %d, %s\n",
			    lwpid, errno ? safe_strerror (errno) : "ERRNO-OK");

... I guess I won't terribly miss it.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c
index a7d11ab8d32..298032dff9a 100644
--- a/gdb/nat/linux-waitpid.c
+++ b/gdb/nat/linux-waitpid.c
@@ -19,35 +19,10 @@ 
 
 #include "common/common-defs.h"
 
-#ifdef GDBSERVER
-/* FIXME: server.h is required for the definition of debug_threads
-   which is used in the gdbserver-specific debug printing in
-   linux_debug.  This code should be made available to GDB also,
-   but the lack of a suitable flag to enable it prevents this.  */
-#include "server.h"
-#endif
-
 #include "linux-nat.h"
 #include "linux-waitpid.h"
 #include "common/gdb_wait.h"
 
-/* Print debugging output based on the format string FORMAT and
-   its parameters.  */
-
-static inline void ATTRIBUTE_PRINTF (1,2)
-linux_debug (const char *format, ...)
-{
-#ifdef GDBSERVER
-  if (debug_threads)
-    {
-      va_list args;
-      va_start (args, format);
-      debug_vprintf (format, args);
-      va_end (args);
-    }
-#endif
-}
-
 /* Convert wait status STATUS to a string.  Used for printing debug
    messages only.  */
 
@@ -79,20 +54,13 @@  status_to_str (int status)
 int
 my_waitpid (int pid, int *status, int flags)
 {
-  int ret, out_errno;
-
-  linux_debug ("my_waitpid (%d, 0x%x)\n", pid, flags);
+  int ret;
 
   do
     {
       ret = waitpid (pid, status, flags);
     }
   while (ret == -1 && errno == EINTR);
-  out_errno = errno;
-
-  linux_debug ("my_waitpid (%d, 0x%x): status(%x), %d\n",
-	       pid, flags, (ret > 0 && status != NULL) ? *status : -1, ret);
 
-  errno = out_errno;
   return ret;
 }