Re: [PATCH] Avoid producing broken non-native core files

[Pedro Alves <palves@redhat.com>]

On 10/23/2013 11:03 PM, Maciej W. Rozycki wrote:
> On Fri, 18 Oct 2013, Pedro Alves wrote:

> 
>>>>> --- gdb-fsf-trunk-quilt.orig/gdb/linux-tdep.c	2013-10-14 22:44:49.868756722 +0100
>>>>> +++ gdb-fsf-trunk-quilt/gdb/linux-tdep.c	2013-10-14 22:46:21.887601484 +0100
>>>>> @@ -1211,7 +1211,9 @@ linux_corefile_thread_callback (struct t
>>>>>  				       args->stop_signal);
>>>>>        args->num_notes++;
>>>>>  
>>>>> -      if (siginfo_data != NULL)
>>>>> +      /* Don't return anything if we got no register information above,
>>>>> +         such a core file is useless.  */
>>>>> +      if (args->note_data != NULL && siginfo_data != NULL)
>>>>
>>>> ... I was surprised to find that it took me a bit to grok the flow of
>>>> this change.  I'd prefer the more explicit:
>>>>
>>>>        args->note_data = args->collect (regcache, info->ptid, args->obfd,
>>>>  				       args->note_data, args->note_size,
>>>>  				       args->stop_signal);
>>>>
>>>>  +      if (args->note_data == NULL)
>>>>  +	{
>>>>  +        /* Don't return anything if we got no register information above,
>>>>  +           such a core file is useless.  */
>>>>  +	   do_cleanups (old_chain);
>>>>  +	   return 1;
>>>>  +	}
>>>>
>>>>        args->num_notes++;
>>>>
>>>>        if (siginfo_data != NULL)
>>>>  	{
>>>>  	  args->note_data = elfcore_write_note (args->obfd,
>>>>  						args->note_data,
>>>>  						args->note_size,
>>>>  						"CORE", NT_SIGINFO,
>>>>  						siginfo_data, siginfo_size);
>>>> 	  args->num_notes++;
>>>>   	}
>>>>
>>>>
>>>> This is OK with that change.
>>>
>>>  I don't like the second exit point and the duplicate call to do_cleanups, 
>>> such arrangements require more maintenance care and raise the risk of 
>>> being missed in future changes around this place.  
>>> I could use a `goto' or a nested `if' statement instead if that made you feel
>>> better than my original proposal -- please pick your preference.
>>
>> It's actually a style used throughout GDB, but no use fighting over it.
>> Let's go with nested if then.  No goto please.
> 
>  The use of `goto' is natural here (it's the equivalent of an exception 
> exit path, which is just a more ornate form of `goto'), but I gave you the 
> choice and will respect it.

Thanks.  The patch is OK.

The thing is cleanups helps not need gotos.  :-)

goto's make sense in C codebases when you don't have a cleanup
mechanism, as then you concentrate all the function-specific
temporary resource deallocation just below the goto label, before the
single return point, as duplicating all that function-specific cleanup
at all early return points tends to result in code duplication and over
time, in bit rot, as some of the return points are updated while
others are are forgotten.

However, once we push temporary resource deallocation to the
cleanup mechanism, what happens is we record what needs cleaning up
close to where the resource is first allocated, instead of near the
tail/return, and then early returns are no longer subject to
the bit rot mentioned above.  All the early return paths can
just do:

  if (...)
    {
      do_cleanups (old_chain);
      return;
    }

This is the style used all over the GDB tree.

(Yes, there are goto instances in the tree, but those tend to
be in old code, possibly even predating cleanups).  We've been
asking people not to add more of those if possible.)

If one still wants to concentrate all the temporary resource
deallocation up at the tail of the function, and not use cleanups,
then one can also do:

  TRY_CATCH (ex, RETURN_MASK_ALL)
    {
        ... body here ...
    }

  // temp resource deallocation here.

  if (ex.reason < 0)
    throw_exception (ex);
  return ...;

Though that style isn't so frequently used.

In a way, the cleanup mechanism maps to C++ RAII, while TRY_CATCH
obviously maps to try/catch(/finally).

-- 
Pedro Alves