Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

[Pedro Alves <palves@redhat.com>]

On 2/28/20 8:35 PM, Sergio Durigan Junior wrote:
> On Friday, February 28 2020, Pedro Alves wrote:
> 
>> On 2/28/20 3:29 PM, Tom Tromey wrote:
>>>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>>>
>>> Sergio> Since this hunk may be a bit controversial, I decided to split it into
>>> Sergio> a separate patch.  This is going to be needed by the ptrace-error
>>> Sergio> feature; GDB will need to be able to access the value of errno even
>>> Sergio> after a call to our 'perror'-like functions.
>>>
>>> I'm in favor of this.  The existing code seems pretty ugly.
>>
>> I'm not sure in favor of relying on errno being preserved from
>> throw site to catch site, with potentially multiple try/catch hops
>> in between.  Sergio, can you point out exactly how you're
>> intending to use that?
> 
> Yeah.  I caught this problem when I was testing to see if GDB would
> print the ptrace fail reason when trying (unsuccessfully) to attach to a
> process.
> 
> The call stack looks like:
> 
>   linux_nat_target::attach
>     |
>     |--> inf_ptrace_target::attach # where ptrace fails
>          |
>          |--> throw_perror_with_name # where errno is set to 0
> 
> When 'throw_perror_with_name' calls 'error', 'linux_nat_target::attach'
> catches the exception and tries to print the reason:
> 
>   try
>     {
>       inf_ptrace_target::attach (args, from_tty);
>     }
>   catch (const gdb_exception_error &ex)
>     {
>       int saved_errno = errno;
>       pid_t pid = parse_pid_to_attach (args);
>       std::string reason = linux_ptrace_attach_fail_reason (pid, saved_errno);
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>       if (!reason.empty ())
> 	throw_error (ex.error, "warning: %s\n%s", reason.c_str (),
> 		     ex.what ());
>       else
> 	throw_error (ex.error, "%s", ex.what ());
>     }
> 
> However, at this point errno is already 0, so the function can't
> determine the exact reason for the ptrace failure.  In fact, because
> errno = 0, 'linux_ptrace_attach_fail_reason' doesn't print anything,
> because it thinks everything is OK!
> 
> IMO, it doesn't make sense to have errno = 0 while you're handling an
> exception (which, in this case, was caused exactly because a syscall
> failed, and so we expect errno to be different than 0).

This is bad design.  Exception objects should be self contained
and not rely on global state to transfer information.

E.g., it should be possible to do

void my_function ()
{
  try 
    {
       function_that_throws ();
    }
  catch (const gdb_exception &ex)
    {
       // call some function that potentially changes errno.
       function_that_changes_errno ();
  
       throw; // rethrow.
    }
}

and then:

try
  {
     my_function ();
  }
catch (const gdb_exception &ex)
  {
     // errno here is really unreliable.
  }

It's not even guaranteed that the exception thrown from
within inf_ptrace_target::attach is thrown after setting
errno, or that errno is meaningful for the exception thrown.

For example, this error:

  if (pid == getpid ())
    error (_("I refuse to debug myself!"));


I think the simpler thing to do here is to change
inf_ptrace_target::attach to return the ptrace errno
as the function's return.  I.e., rename it and change it
from:

 void inf_ptrace_target::attach (const char *args, int from_tty);

to:

 int inf_ptrace_target::ptrace_attach (const char *args, int from_tty);

And change the body like this:

  errno = 0;
  ptrace (PT_ATTACH, pid, (PTRACE_TYPE_ARG3)0, 0);
  if (errno != 0)
-   perror_with_name (("ptrace"));
+   return errno;

with a return 0 added at the end.

All other errors still throw like before.

Then add a new "attach" method that wraps the old method, so that
targets that inherit inf_ptrace_target and don't override attach
continue working:

 void
 inf_ptrace_target::attach (const char *args, int from_tty)
 {
   errno = ptrace_attach (args, from_tty);
   if (errno != 0)
     perror_with_name (("ptrace"));
 }

This avoids having to think about storing the errno number in
the exception object.

Thanks,
Pedro Alves