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

[Yao Qi <qiyaoltc at gmail dot com>]

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];