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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/