[PATCH master/7.12] Throw error when ptrace fail in regsets_fetch_inferior_registers

[PATCH master/7.12] Throw error when ptrace fail in regsets_fetch_inferior_registers

[Yao Qi]

When I run process-dies-while-detaching.exp with GDBserver, I see many
warnings printed by GDBserver,

ptrace(regsets_fetch_inferior_registers) PID=26183: No such process
ptrace(regsets_fetch_inferior_registers) PID=26183: No such process
ptrace(regsets_fetch_inferior_registers) PID=26184: No such process
ptrace(regsets_fetch_inferior_registers) PID=26184: No such process

regsets_fetch_inferior_registers is called when GDBserver resumes each
lwp.

 #2  0x0000000000428260 in regsets_fetch_inferior_registers (regsets_info=0x4690d0 <aarch64_regsets_info>, regcache=0x31832020)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:5412
 #3  0x00000000004070e8 in get_thread_regcache (thread=0x31832940, fetch=fetch@entry=1) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/regcache.c:58
 #4  0x0000000000429c40 in linux_resume_one_lwp_throw (info=<optimized out>, signal=0, step=0, lwp=0x31832830)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4463
 #5  linux_resume_one_lwp (lwp=0x31832830, step=<optimized out>, signal=<optimized out>, info=<optimized out>)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4573

The is the case that threads are disappeared when GDB/GDBserver resumes
them.  Our fix nowadays is to throw exception, caller
linux_resume_one_lwp catches the exception, and swallow it if the lwp
is gone.  See linux-low.c:linux_resume_one_lwp.  So this patch fixes
the problem by throwing exception in regsets_fetch_inferior_registers.
Another caller of regsets_fetch_inferior_registers, linux_fetch_registers,
needs to catch the exception the same way as linux_resume_one_lwp does.

gdb/gdbserver:

2016-08-02  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (regsets_fetch_inferior_registers): Throw
	exception on ptrace fail.
	(linux_fetch_registers): Rename to ...
	(linux_fetch_registers_throw): ... it.
	(linux_fetch_registers): Call linux_fetch_registers_throw
	and catch the exception.  If check_ptrace_stopped_lwp_gone
	is false, throw it again.
---
 gdb/gdbserver/linux-low.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e251ac4..c72b78c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5410,7 +5410,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
 	      char s[256];
 	      sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d",
 		       pid);
-	      perror (s);
+	      perror_with_name (_(s));
 	    }
 	}
       else
@@ -5702,7 +5702,7 @@ usr_store_inferior_registers (const struct regs_info *regs_info,
 
 
 static void
-linux_fetch_registers (struct regcache *regcache, int regno)
+linux_fetch_registers_throw (struct regcache *regcache, int regno)
 {
   int use_regsets;
   int all = 0;
@@ -5735,6 +5735,23 @@ linux_fetch_registers (struct regcache *regcache, int regno)
 }
 
 static void
+linux_fetch_registers (struct regcache *regcache, int regno)
+{
+  TRY
+    {
+      linux_fetch_registers_throw (regcache, regno);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      struct lwp_info *lwp = get_thread_lwp (current_thread);
+
+      if (!check_ptrace_stopped_lwp_gone (lwp))
+	throw_exception (ex);
+    }
+  END_CATCH
+}
+
+static void
 linux_store_registers (struct regcache *regcache, int regno)
 {
   int use_regsets;
-- 
1.9.1

Re: [PATCH master/7.12] Throw error when ptrace fail in regsets_fetch_inferior_registers

[Pedro Alves]

On 08/03/2016 02:19 PM, Yao Qi wrote:
> When I run process-dies-while-detaching.exp with GDBserver, I see many
> warnings printed by GDBserver,
> 
> ptrace(regsets_fetch_inferior_registers) PID=26183: No such process
> ptrace(regsets_fetch_inferior_registers) PID=26183: No such process
> ptrace(regsets_fetch_inferior_registers) PID=26184: No such process
> ptrace(regsets_fetch_inferior_registers) PID=26184: No such process
> 
> regsets_fetch_inferior_registers is called when GDBserver resumes each
> lwp.
> 
>  #2  0x0000000000428260 in regsets_fetch_inferior_registers (regsets_info=0x4690d0 <aarch64_regsets_info>, regcache=0x31832020)
>     at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:5412
>  #3  0x00000000004070e8 in get_thread_regcache (thread=0x31832940, fetch=fetch@entry=1) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/regcache.c:58
>  #4  0x0000000000429c40 in linux_resume_one_lwp_throw (info=<optimized out>, signal=0, step=0, lwp=0x31832830)
>     at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4463
>  #5  linux_resume_one_lwp (lwp=0x31832830, step=<optimized out>, signal=<optimized out>, info=<optimized out>)
>     at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4573
> 
> The is the case that threads are disappeared when GDB/GDBserver resumes
> them.  Our fix nowadays is to throw exception, caller
> linux_resume_one_lwp catches the exception, and swallow it if the lwp
> is gone.  See linux-low.c:linux_resume_one_lwp.  So this patch fixes
> the problem by throwing exception in regsets_fetch_inferior_registers.
> Another caller of regsets_fetch_inferior_registers, linux_fetch_registers,
> needs to catch the exception the same way as linux_resume_one_lwp does.
> 

Throwing is useful when the exception needs to cross several layers.
Originally that was added because the exceptions had to cross through
multiple layers, including inf-ptrace.c and the regcache.

In cases where we have the ptrace errno code at hand, we can simply
check for ESRCH.  That is, in fact what regsets_store_inferior_registers
does.

	  else if (errno == ESRCH)
	    {
	      /* At this point, ESRCH should mean the process is
		 already gone, in which case we simply ignore attempts
		 to change its registers.  See also the related
		 comment in linux_resume_one_lwp.  */
	      free (buf);
	      return 0;
	    }


(store_register checks ESRCH too instead of throwing, as well
as linux_detach_one_lwp and attach_proc_task_lwp_callback.)

However, I don't think swallowing a register/write error is always the
best to do.

If we're resuming the program, then yes, we should swallow the error,
because now that the thread is resumed, we'll collect the exit
status shortly.

But if the thread is _not_ resumed, the user has the now-dead thread
selected, and does "info registers" or "p $somereg = 0xf00" -- then I
think we should report back the error all the way back to the user,
not swallow it and display random garbage for register contents, or
pretend the write succeeded.

So if we go the throw route, I think the catch should be somewhere
higher level, in the code that is reading/writing registers because
it is resuming the thread.  See how linux-nat.c:resume_stopped_resumed_lwps's
TRY block encloses more than one call that might throw.

It may be that in this case the register read is coming from gdb
directly (really no idea), which would mean that it's gdb that
would have to ignore the error, which would complicate things.

But until that is gone, I see no reason to prefer a throw/catch
instead of simply checking for ESRCH, mirroring
regsets_store_inferior_registers?

Thanks,
Pedro Alves

Re: [PATCH master/7.12] Throw error when ptrace fail in regsets_fetch_inferior_registers

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Throwing is useful when the exception needs to cross several layers.
> Originally that was added because the exceptions had to cross through
> multiple layers, including inf-ptrace.c and the regcache.
>
> In cases where we have the ptrace errno code at hand, we can simply
> check for ESRCH.  That is, in fact what regsets_store_inferior_registers
> does.
>
> 	  else if (errno == ESRCH)
> 	    {
> 	      /* At this point, ESRCH should mean the process is
> 		 already gone, in which case we simply ignore attempts
> 		 to change its registers.  See also the related
> 		 comment in linux_resume_one_lwp.  */
> 	      free (buf);
> 	      return 0;
> 	    }
>
>
> (store_register checks ESRCH too instead of throwing, as well
> as linux_detach_one_lwp and attach_proc_task_lwp_callback.)

The reason I choose throw exception is that
regsets_fetch_inferior_registers is called by linux_resume_one_lwp_throw
which throws exception when process is gone.  Even if
regsets_fetch_inferior_registers handles ESRCH quietly, the exception
will be thrown out somewhere else in the callees of
linux_resume_one_lwp_throw.  It is better to throw exception earlier to
avoid some unnecessary operations, for example, in
linux_resume_one_lwp_throw,

      struct regcache *regcache = get_thread_regcache (current_thread, 1);

      lwp->stop_pc = (*the_low_target.get_pc) (regcache);

      if (debug_threads)
	{
	  debug_printf ("  %s from pc 0x%lx\n", step ? "step" : "continue",
			(long) lwp->stop_pc);
	}

if we don't throw exception in regsets_fetch_inferior_registers,
lwp->stop_pc will have the garbage value, unless we check the status of
pc register in regcache in each backend, see whether its status is
REG_UNAVAILABLE or not.  Maybe, since lwp is gone, the value
lwp->stop_pc doesn't matter too much.

>
> However, I don't think swallowing a register/write error is always the
> best to do.
>
> If we're resuming the program, then yes, we should swallow the error,
> because now that the thread is resumed, we'll collect the exit
> status shortly.
>
> But if the thread is _not_ resumed, the user has the now-dead thread
> selected, and does "info registers" or "p $somereg = 0xf00" -- then I
> think we should report back the error all the way back to the user,
> not swallow it and display random garbage for register contents, or
> pretend the write succeeded.

nowadays, GDBserver does this in get_thread_regcache,

      /* Invalidate all registers, to prevent stale left-overs.  */
      memset (regcache->register_status, REG_UNAVAILABLE,
	      regcache->tdesc->num_registers);
      fetch_inferior_registers (regcache, -1);

if the thread is gone, the status of registers is REG_UNAVAILABLE.

>
> So if we go the throw route, I think the catch should be somewhere
> higher level, in the code that is reading/writing registers because
> it is resuming the thread.  See how linux-nat.c:resume_stopped_resumed_lwps's
> TRY block encloses more than one call that might throw.
>
> It may be that in this case the register read is coming from gdb
> directly (really no idea), which would mean that it's gdb that
> would have to ignore the error, which would complicate things.

Yes, I agree.

>
> But until that is gone, I see no reason to prefer a throw/catch
> instead of simply checking for ESRCH, mirroring
> regsets_store_inferior_registers?

OK, how about the patch below?

-- 
Yao (齐尧)
From e5585c0c5544fc2ed3b8aa3b1d709530c9adda84 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 29 Jul 2016 09:59:40 +0100
Subject: [PATCH] Quiet ptrace error ESRCH in regsets_fetch_inferior_registers

When I run process-dies-while-detaching.exp with GDBserver, I see many
warnings printed by GDBserver,

ptrace(regsets_fetch_inferior_registers) PID=26183: No such process
ptrace(regsets_fetch_inferior_registers) PID=26183: No such process
ptrace(regsets_fetch_inferior_registers) PID=26184: No such process
ptrace(regsets_fetch_inferior_registers) PID=26184: No such process

regsets_fetch_inferior_registers is called when GDBserver resumes each
lwp.

 #2  0x0000000000428260 in regsets_fetch_inferior_registers (regsets_info=0x4690d0 <aarch64_regsets_info>, regcache=0x31832020)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:5412
 #3  0x00000000004070e8 in get_thread_regcache (thread=0x31832940, fetch=fetch@entry=1) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/regcache.c:58
 #4  0x0000000000429c40 in linux_resume_one_lwp_throw (info=<optimized out>, signal=0, step=0, lwp=0x31832830)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4463
 #5  linux_resume_one_lwp (lwp=0x31832830, step=<optimized out>, signal=<optimized out>, info=<optimized out>)
    at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4573

The is the case that threads are disappeared when GDB/GDBserver resumes
them.  We check errno for ESRCH, and don't print error messages, like
what we are doing in regsets_store_inferior_registers.

gdb/gdbserver:

2016-08-03  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (regsets_fetch_inferior_registers): Check
	errno is ESRCH or not.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e251ac4..1839f99 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5405,6 +5405,12 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
 		 not "active".  This can happen in normal operation,
 		 so suppress the warning in this case.  */
 	    }
+	  else if (errno == ESRCH)
+	    {
+	      /* At this point, ESRCH should mean the process is
+		 already gone, in which case we simply ignore attempts
+		 to read its registers.  */
+	    }
 	  else
 	    {
 	      char s[256];

Re: [PATCH master/7.12] Throw error when ptrace fail in regsets_fetch_inferior_registers

[Pedro Alves]

On 08/03/2016 04:42 PM, Yao Qi wrote:

> OK, how about the patch below?

LGTM.

Thanks,
Pedro Alves

Re: [PATCH master/7.12] Throw error when ptrace fail in regsets_fetch_inferior_registers

[Yao Qi]

On Wed, Aug 3, 2016 at 6:54 PM, Pedro Alves <palves@redhat.com> wrote:
> On 08/03/2016 04:42 PM, Yao Qi wrote:
>
>> OK, how about the patch below?
>
> LGTM.
>

Thanks, patch is pushed in to master and 7.12.

-- 
Yao (齐尧)