Re: [PATCH 2/2] Avoid letting exceptions escape gdb_bfd_iovec_fileio_close (PR gdb/28080)

[Pedro Alves <pedro@palves.net>]

On 2021-07-13 2:13 p.m., Simon Marchi wrote:
>>>> since in this spot we already close returning error.  That's what this
>>>
>>> I don't understand this sentence, "in this spot we already close
>>> returning error".
>>
>> That's because the "ignore" word is missing... :-P
>>
>> I've tweaked it to:
>>
>>     Thus frame #8 seems like a good place to swallow exceptions.  More so
>>     since in this spot we already ignore target_fileio_close return
>>     errors.  That's what this commit does.  Without the previous fix, we'd
>>     see:
> 
> Ah, I see.  Well then it sounds like if target_fileio_close returns an
> error, that error should be returned by gdb_bfd_iovec_fileio_close as
> well (eventually, not in this patch).
> 
>>> Maybe the more correct thing to do would be to make
>>> gdb_bfd_iovec_fileio_close return an error when it can't properly close
>>> the file.  That error would be communicated as a bfd return status to
>>> gdb_bfd_close_or_warn, which would warn.
>>
>> The problem with that is that it would require losing the exception
>> message, because the warning gdb_bfd_close_or_warn prints is just the
>> text version of a bfd error:
>>
>>   if (!ret)
>>     warning (_("cannot close \"%s\": %s"),
>> 	     name, bfd_errmsg (bfd_get_error ()));
>>
>> Maybe we could add a new type of bfd error, a custom error, where the error
>> reason is given by callback (which gdb would install) or a string or some such, so
>> we could pass the gdb error through bfd.  Not that different from how
>> bfd_error_on_input is a special error code too.  But I'd rather not do this now.
> 
> Ah, I see.  A proper solution involving BFD would be good, but we could
> also bypass BFD and store the error string somewhere where we can pick
> it up again in gdb_bfd_close_or_warn.

And just pick a bfd_error_xxx constant, like e.g., bfd_error_system_call?
Then, if the GDB global is set, we know it's really a GDB error.  That would
work, I think, yeah.  As long as BFD itself doesn't look at the error to
decide anything, I guess.

> 
> But yeah I'm fine with what you have, it's certainly not worse than the
> current state.
> 

OK, I'll go merge the patches then.

>>
>> I've tweaked the patch to ensure that the new warning looks exactly like the
>> one in gdb_bfd_close_or_warn, though.  See below.
>>
>>>
>>> If we need to catch exceptions for the close operations, I suppose we
>>> need to do that as well for the other BFD iovec operations: open, pread
>>> and stat?  A remote communication error could happen in any of those.
>>
>> Yes, though I don't think they're as likely to be in dtor/noexcept paths,
>> and it isn't as obvious to me what the return code should be.  With close,
>> there's not much else we can do other than ignore the error.  I'd rather
>> not touch them for now, for time constraint reasons.  :-/
> 
> I thought an issue (which you talk about in your commit message) was for
> the exception to go over BFD frames, which may not have been compiled
> with -fasynchronous-unwind-tables.  That could happens with any of the
> callbacks.

Yeah, there's that issue, and the issue with reaching the ~objfile dtor.
The BFD issue doesn't trigger on x86-64, while the dtor one triggers everywhere.

> 
>> If you'd prefer, I can drop this patch for now.  The issue addressed by this
>> patch isn't a regression.
> 
> I'm fine with what you have, it makes the situation a bit better.  I
> just wanted to know whether the other callbacks should eventually be
> converted to do the same thing, to document the situation for future us.