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

  Since this hunk may be a bit controversial, I decided to split it into
a separate patch.  This is going to be needed by the ptrace-error
feature; GDB will need to be able to access the value of errno even
after a call to our 'perror'-like functions.

The idea here is that we actually *want* errno to propagate between
our customized 'perror' calls.  We currently have this code on
'throw_perror_with_name':

  /* I understand setting these is a matter of taste.  Still, some people
     may clear errno but not know about bfd_error.  Doing this here is not
     unreasonable.  */
  bfd_set_error (bfd_error_no_error);
  errno = 0;

git blame tells me that this piece of code is pretty old; the commit
that "introduced" it is:

  commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc
  Author: Stan Shebs <shebs@codesourcery.com>
  Date:   Fri Apr 16 01:35:26 1999 +0000

      Initial creation of sourceware repository

so yeah...

If we go to the POSIX specification for 'perror', it doesn't really
say anything about whether errno should be preserved or not.  It does,
however, say that 'perror's messages should be the same as those
returned by 'strerror', and 'strerror' is not supposed to alter errno
if the call is successful.

Maybe when our wrapper was written it was OK to modify errno, I don't
know.  But I'd like to propose that we stick to POSIX in this case.

Another small hunk is the one that saves/restores errno on gdbserver's
'perror_with_name', but this one is pretty trivial, I think.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* utils.c (throw_perror_with_name): Don't reset
	errno/bfd_error.

gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* utils.cc (perror_with_name): Save/restore errno.
---
 gdb/utils.c        | 6 ------
 gdbserver/utils.cc | 2 ++
 2 files changed, 2 insertions(+), 6 deletions(-)
  

>>>>> "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'd imagine it's unlikely that any caller would rely on this.
If it tested cleanly then that is good enough for me.

Sergio> Another small hunk is the one that saves/restores errno on gdbserver's
Sergio> 'perror_with_name', but this one is pretty trivial, I think.

I didn't understand why this one was needed.
Does safe_strerror reset errno?  Maybe a comment would be in order.

Tom
  

On Friday, February 28 2020, 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'd imagine it's unlikely that any caller would rely on this.
> If it tested cleanly then that is good enough for me.

As far as I have tested (buildbot and locally), everything is OK.

> Sergio> Another small hunk is the one that saves/restores errno on gdbserver's
> Sergio> 'perror_with_name', but this one is pretty trivial, I think.
>
> I didn't understand why this one was needed.
> Does safe_strerror reset errno?  Maybe a comment would be in order.

Ah, no, safe_strerror doesn't reset errno.  As I explained in the commit
message, it is not allowed to do so.  The reason I decided to explicitly
save/restore errno is because I wanted to make sure that we (also
explicitly) follow the same rules for both GDB and gdbserver perror
functions.  But now I'm left thinking that I should have proposed the
same thing for 'throw_perror_with_name'...

Anyway, I'm fine with removing the save/restore from gdbserver's
perror.  I'll also remove the reference to it in the commit message.

Thanks,
  

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

>> I didn't understand why this one was needed.
>> Does safe_strerror reset errno?  Maybe a comment would be in order.

Sergio> Ah, no, safe_strerror doesn't reset errno.  As I explained in the commit
Sergio> message, it is not allowed to do so.  The reason I decided to explicitly
Sergio> save/restore errno is because I wanted to make sure that we (also
Sergio> explicitly) follow the same rules for both GDB and gdbserver perror
Sergio> functions.  But now I'm left thinking that I should have proposed the
Sergio> same thing for 'throw_perror_with_name'...

Sergio> Anyway, I'm fine with removing the save/restore from gdbserver's
Sergio> perror.  I'll also remove the reference to it in the commit message.

The main thing for me is that when someone sees this in 5 years, that
there's some idea of why the line is there.  Leaving the line out is
just as good :)

thanks,
Tom
  

On 2/26/20 8:05 PM, Sergio Durigan Junior wrote:
> 
> git blame tells me that this piece of code is pretty old; the commit
> that "introduced" it is:
> 
>   commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc
>   Author: Stan Shebs <shebs@codesourcery.com>
>   Date:   Fri Apr 16 01:35:26 1999 +0000
> 
>       Initial creation of sourceware repository
> 
> so yeah...

This is not the oldest commit in the tree.  Using git log
starting at the hash, you should be able to find older commits
which touch the file.  The thing is that history around the time
of that "initial creation" commit, is quite messy, because the 
CVS tree back then was deleted and recreated a number of times
and merged in some odd ways.

Thanks,
Pedro Alves
  

On Friday, February 28 2020, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>>> I didn't understand why this one was needed.
>>> Does safe_strerror reset errno?  Maybe a comment would be in order.
>
> Sergio> Ah, no, safe_strerror doesn't reset errno.  As I explained in the commit
> Sergio> message, it is not allowed to do so.  The reason I decided to explicitly
> Sergio> save/restore errno is because I wanted to make sure that we (also
> Sergio> explicitly) follow the same rules for both GDB and gdbserver perror
> Sergio> functions.  But now I'm left thinking that I should have proposed the
> Sergio> same thing for 'throw_perror_with_name'...
>
> Sergio> Anyway, I'm fine with removing the save/restore from gdbserver's
> Sergio> perror.  I'll also remove the reference to it in the commit message.
>
> The main thing for me is that when someone sees this in 5 years, that
> there's some idea of why the line is there.  Leaving the line out is
> just as good :)

Right, that's an excellent argument.  It's safe to leave the line out
right now; the important thing is that we don't touch errno, anyway.

Thanks,
  

On Friday, February 28 2020, Pedro Alves wrote:

> On 2/26/20 8:05 PM, Sergio Durigan Junior wrote:
>> 
>> git blame tells me that this piece of code is pretty old; the commit
>> that "introduced" it is:
>> 
>>   commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc
>>   Author: Stan Shebs <shebs@codesourcery.com>
>>   Date:   Fri Apr 16 01:35:26 1999 +0000
>> 
>>       Initial creation of sourceware repository
>> 
>> so yeah...
>
> This is not the oldest commit in the tree.  Using git log
> starting at the hash, you should be able to find older commits
> which touch the file.  The thing is that history around the time
> of that "initial creation" commit, is quite messy, because the 
> CVS tree back then was deleted and recreated a number of times
> and merged in some odd ways.

Sorry, I didn't mean to say that this is the oldest commit in the tree.
But even doing a bit more search, I could only find this commit:

  commit 8eec331072987d38064745a33ae89cc5d195029c
  Author: Steve Chamberlain <sac@cygnus>
  Date:   Sat Mar 19 03:16:10 1994 +0000

              * utils.c (prompt_for_continue): Call readline, not gdb_readline.

which did:

  -  bfd_error = no_error;
  +  bfd_set_error (bfd_error_no_error);

This means the code is really old (pre-1994), and I could not track when
it was added.  And I don't think it matters much at this point: even if
I could find the exact commit that added it, I doubt I'd be able to find
the rationale.
  

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?

But, I'm in favor of removing this errno/bfd_error clearing too.

We used to have more global state clearing around this area,
but it's been gradually removed.  E.g.:

commit 0af679c6e0645a93d5a60ec936b94dc70a2f9e5c
Author:     Pedro Alves <palves@redhat.com>
AuthorDate: Tue Apr 12 16:49:30 2016 +0100
Commit:     Pedro Alves <palves@redhat.com>
CommitDate: Tue Apr 12 16:55:35 2016 +0100

    Don't call clear_quit_flag in prepare_to_throw_exception
    
    I think this is reminiscent of the time when a longjmp would always
    jump to the top level.  Nowaways code that throw exceptions other than
    a quit, which may even be caught and handled without reaching the top
    level.  Certainly such exceptions shouldn't clear an interrupt
    request...


I suspect this perror_with_name errno/bfd_error clearing was also
related to ancient direct longjmp to the top level.

> I'd imagine it's unlikely that any caller would rely on this.
> If it tested cleanly then that is good enough for me.




> 
> Sergio> Another small hunk is the one that saves/restores errno on gdbserver's
> Sergio> 'perror_with_name', but this one is pretty trivial, I think.
> 
> I didn't understand why this one was needed.
> Does safe_strerror reset errno?  Maybe a comment would be in order.
Thanks,
Pedro Alves
  

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).

Thanks,
  

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
  

On Friday, February 28 2020, Pedro Alves wrote:

> 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.

OK.  I implemented your idea, but I will wait until the other patches
are reviewed so I can submit a v2 of the whole series.

Thanks.