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

[Pedro Alves <palves at redhat dot com>]

On 10/16/2013 09:09 PM, Maciej W. Rozycki wrote:
> On Wed, 16 Oct 2013, Pedro Alves wrote:
> 
>>> The cause of missing register information is elfcore_write_prstatus in BFD 
>>> that writes no data (and returns NULL) on non-native targets that have no 
>>> explicit support (bed->elf_backend_write_core_note is NULL), because 
>>> HAVE_PRSTATUS_T and HAVE_PRSTATUS32_T are both forcibly undefined for 
>>> non-native BFD configurations.
>>
>> And if cross debugging, and bed->elf_backend_write_core_note is NULL for the
>> current target, but HAVE_PRSTATUS_T/HAVE_PRSTATUS32_T are defined (for the
>> native target), then gcore will generate bogus notes.  :-/
> 
>  As I say these macros are forcibly undefined for non-native BFD -- see 
> its configure.in.

You seem to forget --enable-targets=all.  In that case, the
HAVE_... bits will be defined for the native target, but that might
not be the target the core is being generated for.

> 
>> It probably will be a long time before bfd's core generation is
>> host-independent everywhere, unfortunately.  As future improvement, maybe
>> we should try _only_ bed->elf_backend_write_core_note, and skip the
>> HAVE_... bits, unless debugging with the native target.  Anyway,
> 
>  This is effectively already the case.

I don't think it is.

>>> --- 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.

>  I'd also prefer to keep the handling of args->num_notes consistent across 
> the two cases -- currently we increment it if elfcore_write_note fails, 
> so let's keep them as they are or change them both at once.

OK...

> We could as well dump the struct member altogether as it doesn't appear used beyond its
> preinitialisation and the two incrementations seen here.

Yeah, I'd prefer getting rid of it.

Thanks,
-- 
Pedro Alves