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

[Luis Machado <lgustavo@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]

On 10/23/2015 02:48 PM, Luis Machado wrote:
> On 10/22/2015 11:43 AM, Pedro Alves wrote:
>> On 10/22/2015 01:36 PM, Luis Machado wrote:
>>> On 10/22/2015 09:50 AM, Pedro Alves wrote:
>>>> On 10/22/2015 12:23 PM, Luis Machado wrote:
>>
>>> 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.
>>
>> It just sounds to me that you didn't take into account
>> that the return values of catch_errors and catch_exceptions
>> differ.
>>
>> while one does:
>>
>>    if (exception.reason < 0)
>>      {
>> ...
>>        return exception.reason;
>>      }
>>
>> the other does:
>>
>>    if (exception.reason != 0)
>>      return 0;
>>
>> This matters because the result is returned by
>> record_full_message_wrapper_safe, and checked here:
>>
>>               if (!record_full_message_wrapper_safe (regcache,
>>                                  GDB_SIGNAL_0))
>>                {
>>                             status->kind = TARGET_WAITKIND_STOPPED;
>>                             status->value.sig = GDB_SIGNAL_0;
>>                             break;
>>                }
>>
>
> Indeed this is the case. I think i'll keep catch_errors and only fix the
> NULL parameter then. Having to adjust return values from unrelated
> functions sounds error-prone and maybe not worth it if we're moving away
> from these types of constructs in the future.
>
>

I've pushed the following now as 7cc53fba0a4e5c316a6e86fdae28f8cc9d0f9a68.

[-- Attachment #2: null_ptr.diff --]
[-- Type: text/x-patch, Size: 607 bytes --]

2015-10-26  Luis Machado  <lgustavo@codesourcery.com>
    
    	* record-full.c (record_full_message_wrapper_safe): Pass empty string to
    	catch_errors call instead of NULL.

diff --git a/gdb/record-full.c b/gdb/record-full.c
index cd47dfa..595e357 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -667,7 +667,7 @@ record_full_message_wrapper_safe (struct regcache *regcache,
   args.regcache = regcache;
   args.signal = signal;
 
-  return catch_errors (record_full_message_wrapper, &args, NULL,
+  return catch_errors (record_full_message_wrapper, &args, "",
 		       RETURN_MASK_ALL);
 }