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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

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.

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

> >  Given that such core files produced are useless anyway I propose that for 
> > targets where elfcore_write_prstatus is indeed used and returns NULL an 
> > error message was printed and core file preparation aborted.  This is 
> > implemented in linux_corefile_thread_callback where signal information is 
> > also stored and currently overwrites any unsuccessful return status from 
> > the register store worker function (linux_collect_thread_registers).  The 
> > test framework is updated accordingly to handle the alternative error 
> > message produced in that case.
> 
> > --- 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.

 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.  We could as well 
dump the struct member altogether as it doesn't appear used beyond its 
preinitialisation and the two incrementations seen here.

  Maciej