[ping] [PATCH 1/2] gdbserver: catch fetch registers error

[ping] [PATCH 1/2] gdbserver: catch fetch registers error

[Metzger, Markus T]

ping

> -----Original Message-----
> From: Metzger, Markus T
> Sent: Tuesday, December 6, 2016 4:55 PM
> To: gdb-patches@sourceware.org
> Cc: Daniel Jacobowitz <drow@false.org>
> Subject: [PATCH 1/2] gdbserver: catch fetch registers error
> 
> When the PTRACE_PEEKUSER ptrace request to read registers fails, gdbserer
> throws
> an error that is caught in captured_main, where it causes a E01 error packet to
> be sent and gdbserer to quit (if --once was specified) or the event loop to be
> re-started (otherwise).
> 
> We may get such ptrace errors when trying to fetch registers for an exited or
> running thread.  There are checks in GDB that check those conditions and throw
> meaningful error messages before we could run into the above ptrace error,
> e.g. thread.c:validate_registers_access.
> 
> I ran into a new case and, rather than adding another call to
> validate_registers_access in GDB, I propose to catch the error already when
> handling the 'g' packet in gdbserver and reply with an error packet - assuming
> that gdbserver's internal state is still intact.
> 
> To not replace a meaningful error message with E01, I'm trying to generate a
> useful error message when the error is detected and the exception is thrown.
> 
> It would look like this ...
> 
> gdb) PASS: gdb.btrace/enable-running.exp: continue to breakpoint: cont to 44
> cont&
> Continuing.
> (gdb) PASS: gdb.btrace/enable-running.exp: cont&
> record btrace
> warning: Remote failure reply: E.Selected thread is running.
> warning: Remote failure reply: E.Selected thread is running.
> 
> ... although in this particular case, I'm going to suppress the warning.
> 
> To make this look a bit nicer, we could consider stripping the "E." or the
> entire "Remote failure reply: E." when (re-)throwing the error inside GDB in
> remote.c.
> 
> CC:  Daniel Jacobowitz	<drow@false.org>
> 
> 2016-12-06  Markus Metzger  <markus.t.metzger@intel.com>
> 
> gdbserver/
> 	* server.c (process_serial_event): Add TRY/CATCH.
> 	* linux-low.c (fetch_register): Improve error message.
> ---
>  gdb/gdbserver/linux-low.c | 19 ++++++++++++++++++-
>  gdb/gdbserver/server.c    | 18 ++++++++++++++++--
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index e3e372c..a942b87 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -5692,7 +5692,24 @@ fetch_register (const struct usrregs_info *usrregs,
>  		(PTRACE_TYPE_ARG3) (uintptr_t) regaddr, (PTRACE_TYPE_ARG4)
> 0);
>        regaddr += sizeof (PTRACE_XFER_TYPE);
>        if (errno != 0)
> -	error ("reading register %d: %s", regno, strerror (errno));
> +	{
> +	  /* ESRCH could mean that the thread is not traced, exited, or is not
> +	     stopped.  */
> +	  if (errno == ESRCH)
> +	    {
> +	      struct lwp_info *lwp = get_thread_lwp (current_thread);
> +
> +	      if (!lwp_is_stopped (lwp))
> +		error (_("Selected thread is running."));
> +
> +	      if (lwp_is_marked_dead (lwp))
> +		error (_("Selected thread has terminated."));
> +	    }
> +
> +	  /* Report a generic error if we could not determine the exact
> +	     reason.  */
> +	  error (_("Could not read register %d: %s."), regno, strerror (errno));
> +	}
>      }
> 
>    if (the_low_target.supply_ptrace_register)
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index ef8dd03..3064b4f 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -4132,8 +4132,22 @@ process_serial_event (void)
>  	    write_enn (own_buf);
>  	  else
>  	    {
> -	      regcache = get_thread_regcache (current_thread, 1);
> -	      registers_to_string (regcache, own_buf);
> +	      TRY
> +		{
> +		  regcache = get_thread_regcache (current_thread, 1);
> +		  registers_to_string (regcache, own_buf);
> +		}
> +	      CATCH (exception, RETURN_MASK_ALL)
> +		{
> +		  const char *message;
> +
> +		  message = exception.message;
> +		  if (message == NULL)
> +		    message = _("Reading registers failed.");
> +
> +		  sprintf (own_buf, "E.%s", message);
> +		}
> +	      END_CATCH
>  	    }
>  	}
>        break;
> --
> 1.8.3.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [ping] [PATCH 1/2] gdbserver: catch fetch registers error

[Luis Machado]

On 01/03/2017 07:14 AM, Metzger, Markus T wrote:
> ping
>
>> -----Original Message-----
>> From: Metzger, Markus T
>> Sent: Tuesday, December 6, 2016 4:55 PM
>> To: gdb-patches@sourceware.org
>> Cc: Daniel Jacobowitz <drow@false.org>
>> Subject: [PATCH 1/2] gdbserver: catch fetch registers error
>>
>> When the PTRACE_PEEKUSER ptrace request to read registers fails, gdbserer
>> throws
>> an error that is caught in captured_main, where it causes a E01 error packet to
>> be sent and gdbserer to quit (if --once was specified) or the event loop to be
>> re-started (otherwise).
>>
>> We may get such ptrace errors when trying to fetch registers for an exited or
>> running thread.  There are checks in GDB that check those conditions and throw
>> meaningful error messages before we could run into the above ptrace error,
>> e.g. thread.c:validate_registers_access.
>>
>> I ran into a new case and, rather than adding another call to
>> validate_registers_access in GDB, I propose to catch the error already when
>> handling the 'g' packet in gdbserver and reply with an error packet - assuming
>> that gdbserver's internal state is still intact.
>>
>> To not replace a meaningful error message with E01, I'm trying to generate a
>> useful error message when the error is detected and the exception is thrown.
>>
>> It would look like this ...
>>
>> gdb) PASS: gdb.btrace/enable-running.exp: continue to breakpoint: cont to 44
>> cont&
>> Continuing.
>> (gdb) PASS: gdb.btrace/enable-running.exp: cont&
>> record btrace
>> warning: Remote failure reply: E.Selected thread is running.
>> warning: Remote failure reply: E.Selected thread is running.
>>
>> ... although in this particular case, I'm going to suppress the warning.
>>
>> To make this look a bit nicer, we could consider stripping the "E." or the
>> entire "Remote failure reply: E." when (re-)throwing the error inside GDB in
>> remote.c.
>>
>> CC:  Daniel Jacobowitz	<drow@false.org>
>>
>> 2016-12-06  Markus Metzger  <markus.t.metzger@intel.com>
>>
>> gdbserver/
>> 	* server.c (process_serial_event): Add TRY/CATCH.
>> 	* linux-low.c (fetch_register): Improve error message.
>> ---
>>  gdb/gdbserver/linux-low.c | 19 ++++++++++++++++++-
>>  gdb/gdbserver/server.c    | 18 ++++++++++++++++--
>>  2 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index e3e372c..a942b87 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -5692,7 +5692,24 @@ fetch_register (const struct usrregs_info *usrregs,
>>  		(PTRACE_TYPE_ARG3) (uintptr_t) regaddr, (PTRACE_TYPE_ARG4)
>> 0);
>>        regaddr += sizeof (PTRACE_XFER_TYPE);
>>        if (errno != 0)
>> -	error ("reading register %d: %s", regno, strerror (errno));
>> +	{
>> +	  /* ESRCH could mean that the thread is not traced, exited, or is not
>> +	     stopped.  */
>> +	  if (errno == ESRCH)
>> +	    {
>> +	      struct lwp_info *lwp = get_thread_lwp (current_thread);
>> +
>> +	      if (!lwp_is_stopped (lwp))
>> +		error (_("Selected thread is running."));
>> +
>> +	      if (lwp_is_marked_dead (lwp))
>> +		error (_("Selected thread has terminated."));
>> +	    }
>> +
>> +	  /* Report a generic error if we could not determine the exact
>> +	     reason.  */
>> +	  error (_("Could not read register %d: %s."), regno, strerror (errno));
>> +	}
>>      }
>>
>>    if (the_low_target.supply_ptrace_register)
>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index ef8dd03..3064b4f 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -4132,8 +4132,22 @@ process_serial_event (void)
>>  	    write_enn (own_buf);
>>  	  else
>>  	    {
>> -	      regcache = get_thread_regcache (current_thread, 1);
>> -	      registers_to_string (regcache, own_buf);
>> +	      TRY
>> +		{
>> +		  regcache = get_thread_regcache (current_thread, 1);
>> +		  registers_to_string (regcache, own_buf);
>> +		}
>> +	      CATCH (exception, RETURN_MASK_ALL)
>> +		{
>> +		  const char *message;
>> +
>> +		  message = exception.message;
>> +		  if (message == NULL)
>> +		    message = _("Reading registers failed.");
>> +
>> +		  sprintf (own_buf, "E.%s", message);
>> +		}
>> +	      END_CATCH
>>  	    }
>>  	}
>>        break;

Is this a guaranteed recoverable scenario? I've seen GDB get confused 
and mess up its internal state multiple times when it can't fetch 
something essential like memory or registers.

So, even if we handle things gracefully in gdbserver, does GDB handle 
that gracefully enough to carry on with a debugging session?

RE: [ping] [PATCH 1/2] gdbserver: catch fetch registers error

[Metzger, Markus T]

> -----Original Message-----
> From: Luis Machado [mailto:lgustavo@codesourcery.com]
> Sent: Tuesday, January 3, 2017 7:29 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
> patches@sourceware.org
> Cc: Daniel Jacobowitz <drow@false.org>; Pedro Alves (palves@redhat.com)
> <palves@redhat.com>
> Subject: Re: [ping] [PATCH 1/2] gdbserver: catch fetch registers error

Hi Luis,

Thanks for your feedback.

> >> When the PTRACE_PEEKUSER ptrace request to read registers fails, gdbserer
> >> throws
> >> an error that is caught in captured_main, where it causes a E01 error packet to
> >> be sent and gdbserer to quit (if --once was specified) or the event loop to be
> >> re-started (otherwise).
> >>
> >> We may get such ptrace errors when trying to fetch registers for an exited or
> >> running thread.  There are checks in GDB that check those conditions and
> throw
> >> meaningful error messages before we could run into the above ptrace error,
> >> e.g. thread.c:validate_registers_access.
> >>
> >> I ran into a new case and, rather than adding another call to
> >> validate_registers_access in GDB, I propose to catch the error already when
> >> handling the 'g' packet in gdbserver and reply with an error packet - assuming
> >> that gdbserver's internal state is still intact.
> >>
> >> To not replace a meaningful error message with E01, I'm trying to generate a
> >> useful error message when the error is detected and the exception is thrown.
> >>
> >> It would look like this ...
> >>
> >> gdb) PASS: gdb.btrace/enable-running.exp: continue to breakpoint: cont to 44
> >> cont&
> >> Continuing.
> >> (gdb) PASS: gdb.btrace/enable-running.exp: cont&
> >> record btrace
> >> warning: Remote failure reply: E.Selected thread is running.
> >> warning: Remote failure reply: E.Selected thread is running.
> >>
> >> ... although in this particular case, I'm going to suppress the warning.
> >>
> >> To make this look a bit nicer, we could consider stripping the "E." or the
> >> entire "Remote failure reply: E." when (re-)throwing the error inside GDB in
> >> remote.c.
[...] 
> Is this a guaranteed recoverable scenario? I've seen GDB get confused
> and mess up its internal state multiple times when it can't fetch
> something essential like memory or registers.
> 
> So, even if we handle things gracefully in gdbserver, does GDB handle
> that gracefully enough to carry on with a debugging session?

At the moment, GDBserver quits or starts over.  This is not recoverable as
GDB doesn't have a clue that GDBserver just started over.

This patch improves the situation by making GDBserver send an appropriate error
message to GDB and resume normally.  It suffices in one concrete case (see next
patch in series).  If in other cases GDB does not handle the error, I'd say we fix GDB
for those cases.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [ping] [PATCH 1/2] gdbserver: catch fetch registers error

[Luis Machado]

On 01/04/2017 01:20 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Luis Machado [mailto:lgustavo@codesourcery.com]
>> Sent: Tuesday, January 3, 2017 7:29 PM
>> To: Metzger, Markus T <markus.t.metzger@intel.com>; gdb-
>> patches@sourceware.org
>> Cc: Daniel Jacobowitz <drow@false.org>; Pedro Alves (palves@redhat.com)
>> <palves@redhat.com>
>> Subject: Re: [ping] [PATCH 1/2] gdbserver: catch fetch registers error
>
> Hi Luis,
>
> Thanks for your feedback.
>
>>>> When the PTRACE_PEEKUSER ptrace request to read registers fails, gdbserer
>>>> throws
>>>> an error that is caught in captured_main, where it causes a E01 error packet to
>>>> be sent and gdbserer to quit (if --once was specified) or the event loop to be
>>>> re-started (otherwise).
>>>>
>>>> We may get such ptrace errors when trying to fetch registers for an exited or
>>>> running thread.  There are checks in GDB that check those conditions and
>> throw
>>>> meaningful error messages before we could run into the above ptrace error,
>>>> e.g. thread.c:validate_registers_access.
>>>>
>>>> I ran into a new case and, rather than adding another call to
>>>> validate_registers_access in GDB, I propose to catch the error already when
>>>> handling the 'g' packet in gdbserver and reply with an error packet - assuming
>>>> that gdbserver's internal state is still intact.
>>>>
>>>> To not replace a meaningful error message with E01, I'm trying to generate a
>>>> useful error message when the error is detected and the exception is thrown.
>>>>
>>>> It would look like this ...
>>>>
>>>> gdb) PASS: gdb.btrace/enable-running.exp: continue to breakpoint: cont to 44
>>>> cont&
>>>> Continuing.
>>>> (gdb) PASS: gdb.btrace/enable-running.exp: cont&
>>>> record btrace
>>>> warning: Remote failure reply: E.Selected thread is running.
>>>> warning: Remote failure reply: E.Selected thread is running.
>>>>
>>>> ... although in this particular case, I'm going to suppress the warning.
>>>>
>>>> To make this look a bit nicer, we could consider stripping the "E." or the
>>>> entire "Remote failure reply: E." when (re-)throwing the error inside GDB in
>>>> remote.c.
> [...]
>> Is this a guaranteed recoverable scenario? I've seen GDB get confused
>> and mess up its internal state multiple times when it can't fetch
>> something essential like memory or registers.
>>
>> So, even if we handle things gracefully in gdbserver, does GDB handle
>> that gracefully enough to carry on with a debugging session?
>
> At the moment, GDBserver quits or starts over.  This is not recoverable as
> GDB doesn't have a clue that GDBserver just started over.
>
> This patch improves the situation by making GDBserver send an appropriate error
> message to GDB and resume normally.  It suffices in one concrete case (see next
> patch in series).  If in other cases GDB does not handle the error, I'd say we fix GDB
> for those cases.

I missed the second entry in the series. I'll check it.