Re: [PATCH] Do not pass NULL for the string in catch_errors

[Pedro Alves <palves at redhat dot com>]

On 10/22/2015 12:23 PM, Luis Machado wrote:
> On 10/22/2015 09:07 AM, Pedro Alves wrote:
>> On 10/21/2015 12:14 PM, Luis Machado wrote:
>>> On 09/10/2015 10:16 AM, Pedro Alves wrote:
>>>> On 09/09/2015 03:45 PM, Luis Machado wrote:
>>>>> I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp,
>>>>> in a mingw32 GDB, in this code path. It boils down to the code trying to
>>>>> strlen () a NULL pointer. I tracked things down and it looks like
>>>>> record_full_message_wrapper_safe is the only occurrence.
>>>>>
>>>>> We could also change catch_errors to check the char pointer and pass the
>>>>> empty string automatically if the pointer is NULL. Then again, it seems like
>>>>> catch_errors is going away at any time now, being potentially replaced
>>>>> with catch_exceptions.
>>>>
>>>> It's been marked superseded for years.  If you had fixed this by
>>>> converting this one instance, we'd be a little closer.  ;-)
>>>>
>>>
>>> Well, we shouldn't rush! :-)
>>>
>>> Seriously, i've been looking into this and it doesn't look like
>>> catch_exceptions/catch_exceptions_with_msg is something we'll want to
>>> use in the long run either. Those couple functions also do not directly
>>> replace catch_errors.
>>>
>>> I thought about replacing the remaining catch_errors occurrences with
>>> TRY/CATCH/END_CATCH blocks, which sounds better aligned with what we
>>> want to do in the future - migrating to C++ etc. Then we can finally get
>>> rid of catch_errors and a few useless wrappers. How does that sound?
>>
>> Sounds like better leave it be then.  It may be that with proper C++/RAII
>> the try/catches would disappear altogether in the end, for instance.
> 
> I see. Unfortunately, for the cases where catch_exceptions supposedly 
> acts similarly to catch_errors, it still doesn't work correctly because 
> catch_exceptions doesn't seem to cope well with error () calls, like the 
> case inside record-full.c.

Now I'm confused -- why doesn't it?

But TBC, by "leave it be", I meant "just go with your original patch".

If you do want to go through and replace all catch_errors with
TRY/CATCH, I don't oppose it at all.  I guess I was just trying to
avoid imposing extra work on you.

> 
> With catch_exceptions, instead of catching the error and letting the 
> inferior continue, it will just cause the inferior to terminate.

I don't understand.  Why do you say this will happen?

> 
> The other cases spread through breakpoint.c, infrun.c, solib.c etc, are 
> supposed to emit a message in case an error happens, as opposed to 
> passing an empty string.
> 
> catch_exceptions_with_msg only allows recording a copy of the message 
> from an exception thrown from the guarded called function. It doesn't 
> emit a message passed in as argument like catch_errors.
> 

Yeah.  I'm not exactly sure why catch_errors was marked
deprecated/superseded originally, but it does feel like
catch_exceptions_with_msg isn't ideal either.

Thanks,
Pedro Alves