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

Sergio Durigan Junior
2020-03-02T20:07:17Z

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