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

  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?
  

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

> OK, how about the patch below?

LGTM.

Thanks,
Pedro Alves
  

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.