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

[Luis Machado <lgustavo@codesourcery.com>]

On 10/22/2015 09:50 AM, Pedro Alves wrote:
> 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.
>

That would be fine by me. I was just experimenting with 
TRY/CATCH/END_CATCH after my unsuccessful replacement of catch_errors 
with catch_exceptions. See below.
>>
>> 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?
>

I replaced catch_errors with catch_exceptions in record-full.c. I saw a 
bunch of failures in gdb.reverse/sigall-reverse.exp, starting at this point:

Breakpoint 142, handle_TERM (sig=15) at 
../../../gdb-head-ro/gdb/testsuite/gdb.reverse/sigall-reverse.c:378^M
378     }^M
(gdb) PASS: gdb.reverse/sigall-reverse.exp: send signal TERM
continue^M
Continuing.^M
The next instruction is syscall exit_group.  It will make the program 
exit.  Do you want to stop the program?([y] or n) yes^M
Process record: inferior program stopped.^M
^M
[process 21188] #1 stopped.^M

The above is a normal run. If i replace catch_errors with 
catch_exceptions, instead of stopping the inferior, it will terminate. 
Maybe there is a bug somewhere, or something is being mishandled.