[PATCH] Avoid producing broken non-native core files

[PATCH] Avoid producing broken non-native core files

[Maciej W. Rozycki]

Hi,

 We don't have complete non-native core file generation support across all 
the supported targets.  We have explicit support for some targets that is 
implemented differently depending on how general core file support has 
been wired in for a given target, either in GDB proper (the old scheme, 
such as with mips-linux-gnu) or in BFD (the new scheme, such as with 
i686-linux-gnu).  Unfortunately where non-native core file support is 
missing the results are inconsistent between the two cases.

 The two paths diverge in linux_make_corefile_notes_1, the old defaults to 
exec_make_note_section and fails mostly gracefully (an unfinished dump is 
left behind).  The new path does not fail, but an incomplete dump is 
produced with no register information, that GDB itself does not handle 
('"foo" is not a core dump: File format not recognized') and that scores a 
number of failures in the test suite across various cases, e.g.:

(gdb) PASS: gdb.base/auxv.exp: continue
info auxv
32   AT_SYSINFO           Special system info/entry points 0xb7fdd400
33   AT_SYSINFO_EHDR      System-supplied DSO's ELF header 0xb7fdd000
16   AT_HWCAP             Machine-dependent CPU capability hints 0xbfe9fbf7
6    AT_PAGESZ            System page size               4096
17   AT_CLKTCK            Frequency of times()           100
3    AT_PHDR              Program headers for program    0x8048034
4    AT_PHENT             Size of program header entry   32
5    AT_PHNUM             Number of program headers      8
7    AT_BASE              Base address of interpreter    0xb7fde000
8    AT_FLAGS             Flags                          0x0
9    AT_ENTRY             Entry point of program         0x8048590
11   AT_UID               Real user ID                   49869
12   AT_EUID              Effective user ID              49869
13   AT_GID               Real group ID                  280
14   AT_EGID              Effective group ID             280
23   AT_SECURE            Boolean, was exec setuid-like? 0
15   AT_PLATFORM          String identifying platform    0xbffff9cb "i686"
0    AT_NULL              End of vector                  0x0
(gdb) PASS: gdb.base/auxv.exp: info auxv on live process
gcore .../gdb.base/auxv.gcore
Saved corefile .../gdb.base/auxv.gcore
(gdb) PASS: gdb.base/auxv.exp: gcore
continue
Continuing.

Program received signal SIGABRT, Aborted.
0xb7fdd410 in __kernel_vsyscall ()
(gdb) PASS: gdb.base/auxv.exp: continue
continue
Continuing.

Program terminated with signal SIGABRT, Aborted.
The program no longer exists.
(gdb) PASS: gdb.base/auxv.exp: continue
UNSUPPORTED: gdb.base/auxv.exp: generate native core dump
Executing on build: rm -rf .../gdb.base/coredir.4310    (timeout = 3000)

Child terminated with signal = 0x6 (SIGABRT)
GDBserver exiting
UNSUPPORTED: gdb.base/auxv.exp: info auxv on native core dump
UNSUPPORTED: gdb.base/auxv.exp: matching auxv data from live and core
core .../gdb.base/auxv.gcore
".../gdb.base/auxv.gcore" is not a core dump: File format not recognized
(gdb) FAIL: gdb.base/auxv.exp: load core file for info auxv on gcore-created dump
info auxv
The program has no auxiliary information now.
(gdb) FAIL: gdb.base/auxv.exp: info auxv on gcore-created dump
FAIL: gdb.base/auxv.exp: matching auxv data from live and gcore
testcase .../gdb/testsuite/gdb.base/auxv.exp completed in 3 seconds

		=== gdb Summary ===

# of expected passes		8
# of unexpected failures	3
# of unsupported tests		3

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.

 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.

 This change removes the failure shown above:

(gdb) PASS: gdb.base/auxv.exp: continue
info auxv
32   AT_SYSINFO           Special system info/entry points 0xb7fdd400
33   AT_SYSINFO_EHDR      System-supplied DSO's ELF header 0xb7fdd000
16   AT_HWCAP             Machine-dependent CPU capability hints 0xbfe9fbf7
6    AT_PAGESZ            System page size               4096
17   AT_CLKTCK            Frequency of times()           100
3    AT_PHDR              Program headers for program    0x8048034
4    AT_PHENT             Size of program header entry   32
5    AT_PHNUM             Number of program headers      8
7    AT_BASE              Base address of interpreter    0xb7fde000
8    AT_FLAGS             Flags                          0x0
9    AT_ENTRY             Entry point of program         0x8048590
11   AT_UID               Real user ID                   49869
12   AT_EUID              Effective user ID              49869
13   AT_GID               Real group ID                  280
14   AT_EGID              Effective group ID             280
23   AT_SECURE            Boolean, was exec setuid-like? 0
15   AT_PLATFORM          String identifying platform    0xbffff9cb "i686"
0    AT_NULL              End of vector                  0x0
(gdb) PASS: gdb.base/auxv.exp: info auxv on live process
gcore .../gdb.base/auxv.gcore
Target does not support core file generation.
(gdb) UNSUPPORTED: gdb.base/auxv.exp: gcore
continue
Continuing.

Program received signal SIGABRT, Aborted.
0xb7fdd410 in __kernel_vsyscall ()
(gdb) PASS: gdb.base/auxv.exp: continue
continue
Continuing.

Program terminated with signal SIGABRT, Aborted.
The program no longer exists.
(gdb) PASS: gdb.base/auxv.exp: continue
UNSUPPORTED: gdb.base/auxv.exp: generate native core dump
Executing on build: rm -rf .../gdb.base/coredir.5217    (timeout = 3000)

Child terminated with signal = 0x6 (SIGABRT)
GDBserver exiting
UNSUPPORTED: gdb.base/auxv.exp: info auxv on native core dump
UNSUPPORTED: gdb.base/auxv.exp: matching auxv data from live and core
UNSUPPORTED: gdb.base/auxv.exp: info auxv on gcore-created dump
UNSUPPORTED: gdb.base/auxv.exp: matching auxv data from live and gcore
testcase .../gdb/testsuite/gdb.base/auxv.exp completed in 1 seconds

		=== gdb Summary ===

# of expected passes		7
# of unsupported tests		6

-- tested with the i686-linux-gnu target.

 OK to apply?

2013-10-15  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* linux-tdep.c (linux_corefile_thread_callback): Propagate any
	failure from register information collection.

	gdb/testsuite/
	* lib/gdb.exp (gdb_gcore_cmd): Also handle a "Target does not 
	support core file generation" reply.

  Maciej

gdb-core-note-data.diff
Index: gdb-fsf-trunk-quilt/gdb/linux-tdep.c
===================================================================
--- 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)
 	{
 	  args->note_data = elfcore_write_note (args->obfd,
 						args->note_data,
Index: gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdb.exp
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/testsuite/lib/gdb.exp	2013-10-14 22:44:44.867662030 +0100
+++ gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdb.exp	2013-10-14 22:46:21.887601484 +0100
@@ -3183,7 +3183,7 @@ proc gdb_gcore_cmd {core test} {
 	    verbose -log "'gcore' command undefined in gdb_gcore_cmd"
 	}
 
-	-re "Can't create a corefile\[\r\n\]+$gdb_prompt $" {
+	-re "(?:Can't create a corefile|Target does not support core file generation\\.)\[\r\n\]+$gdb_prompt $" {
 	    unsupported $test
 	}
     }

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

[Pedro Alves]

On 10/15/2013 03:20 PM, Maciej W. Rozycki 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.  :-/
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,

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

Thanks,
-- 
Pedro Alves

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

[Maciej W. Rozycki]

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

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

[Pedro Alves]

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

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

[Maciej W. Rozycki]

On Fri, 18 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.

 Hmm, indeed, that complicates things a bit.  I think we don't want to 
regress native support, so --enable-targets=all shouldn't be disabling 
these HAVE_PRSTATUS_T/HAVE_PRSTATUS32_T checks.  Then in this case the 
native BFD does not necessarily have to be the default or one chosen by 
the user of a particular session, so we'd have to tell the native and 
non-native case apart in elfcore_write_prstatus (and elsewhere around) 
somehow.

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

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

 Here it is.  An update of the original change follows.  OK to apply?

  Maciej

2013-10-23  "Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* linux-tdep.c (linux_corefile_thread_data): Remove `num_notes'
	member.
	(linux_corefile_thread_callback): Update accordingly.
	(linux_make_corefile_notes): Likewise.

gdb-core-num-notes.diff
Index: gdb-fsf-trunk-quilt/gdb/linux-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/linux-tdep.c	2013-10-21 12:15:14.717958850 +0100
+++ gdb-fsf-trunk-quilt/gdb/linux-tdep.c	2013-10-21 12:15:29.727655132 +0100
@@ -1176,7 +1176,6 @@ struct linux_corefile_thread_data
   bfd *obfd;
   char *note_data;
   int *note_size;
-  int num_notes;
   enum gdb_signal stop_signal;
   linux_collect_thread_registers_ftype collect;
 };
@@ -1209,7 +1208,6 @@ linux_corefile_thread_callback (struct t
       args->note_data = args->collect (regcache, info->ptid, args->obfd,
 				       args->note_data, args->note_size,
 				       args->stop_signal);
-      args->num_notes++;
 
       if (siginfo_data != NULL)
 	{
@@ -1218,7 +1216,6 @@ linux_corefile_thread_callback (struct t
 						args->note_size,
 						"CORE", NT_SIGINFO,
 						siginfo_data, siginfo_size);
-	  args->num_notes++;
 	}
 
       do_cleanups (old_chain);
@@ -1467,7 +1464,6 @@ linux_make_corefile_notes (struct gdbarc
   thread_args.obfd = obfd;
   thread_args.note_data = note_data;
   thread_args.note_size = note_size;
-  thread_args.num_notes = 0;
   thread_args.stop_signal = find_stop_signal ();
   thread_args.collect = collect;
   iterate_over_threads (linux_corefile_thread_callback, &thread_args);

2013-10-23  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* linux-tdep.c (linux_corefile_thread_callback): Propagate any
	failure from register information collection.

	gdb/testsuite/
	* lib/gdb.exp (gdb_gcore_cmd): Also handle a "Target does not
	support core file generation" reply.

gdb-core-note-data.diff
Index: gdb-fsf-trunk-quilt/gdb/linux-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/linux-tdep.c	2013-10-21 12:15:29.727655132 +0100
+++ gdb-fsf-trunk-quilt/gdb/linux-tdep.c	2013-10-21 12:15:53.727656870 +0100
@@ -1209,14 +1209,15 @@ linux_corefile_thread_callback (struct t
 				       args->note_data, args->note_size,
 				       args->stop_signal);
 
-      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)
+	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);
-	}
 
       do_cleanups (old_chain);
     }
Index: gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdb.exp
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/testsuite/lib/gdb.exp	2013-10-21 12:15:14.717958850 +0100
+++ gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdb.exp	2013-10-21 12:15:53.727656870 +0100
@@ -3183,7 +3183,7 @@ proc gdb_gcore_cmd {core test} {
 	    verbose -log "'gcore' command undefined in gdb_gcore_cmd"
 	}
 
-	-re "Can't create a corefile\[\r\n\]+$gdb_prompt $" {
+	-re "(?:Can't create a corefile|Target does not support core file generation\\.)\[\r\n\]+$gdb_prompt $" {
 	    unsupported $test
 	}
     }

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

[Pedro Alves]

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

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

[Steve Ellcey]

On Wed, 2013-10-23 at 23:03 +0100, Maciej W. Rozycki wrote:


> Index: gdb-fsf-trunk-quilt/gdb/linux-tdep.c
> ===================================================================
> --- gdb-fsf-trunk-quilt.orig/gdb/linux-tdep.c	2013-10-21 12:15:29.727655132 +0100
> +++ gdb-fsf-trunk-quilt/gdb/linux-tdep.c	2013-10-21 12:15:53.727656870 +0100
> @@ -1209,14 +1209,15 @@ linux_corefile_thread_callback (struct t
>  				       args->note_data, args->note_size,
>  				       args->stop_signal);
>  
> -      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)
> +	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);
> -	}
>  
>        do_cleanups (old_chain);
>      }

Maciej,  I believe this change is the cause of a build failure for me
when using an old GCC (4.1.2) to build gdb.  I get:

cc1: warnings being treated as errors
/scratch/gcc/nightly/src/binutils-gdb/gdb/linux-tdep.c: In function
'linux_corefile_thread_callback':
/scratch/gcc/nightly/src/binutils-gdb/gdb/linux-tdep.c:1196: warning:
'siginfo_size' may be used uninitialized in this function

I do not get this error when using a newer GCC (like 4.4.3 or above).  I
can work around it by setting siginfo_size to 0 when it is declared.  I
don't know if there is a minimum GCC version required for building gdb
but my build with GCC 4.1.2 used to work.  Do you think initializing
siginfo_size is reasonable to allow us to use older GCC's to build gdb?

Steve Ellcey
sellcey@mips.com

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

[Maciej W. Rozycki]

On Tue, 29 Oct 2013, Steve Ellcey wrote:

> Maciej,  I believe this change is the cause of a build failure for me
> when using an old GCC (4.1.2) to build gdb.  I get:
> 
> cc1: warnings being treated as errors
> /scratch/gcc/nightly/src/binutils-gdb/gdb/linux-tdep.c: In function
> 'linux_corefile_thread_callback':
> /scratch/gcc/nightly/src/binutils-gdb/gdb/linux-tdep.c:1196: warning:
> 'siginfo_size' may be used uninitialized in this function
> 
> I do not get this error when using a newer GCC (like 4.4.3 or above).  I
> can work around it by setting siginfo_size to 0 when it is declared.  I
> don't know if there is a minimum GCC version required for building gdb
> but my build with GCC 4.1.2 used to work.  Do you think initializing
> siginfo_size is reasonable to allow us to use older GCC's to build gdb?

 You're right, I noticed this problem in a build I did after I had already 
committed that change -- it looks like GCC tries to recurse into the 
static function being called (that is passed a pointer to siginfo_size) to 
figure out when the variable is initialised and when it is not.  And for 
some compiler's versions (4.3.3 in my case) it gets confused and 
complains.  Sorry to have caused this regression.

 Here's a change I made last week before I ran out of time; I gather it is 
the same as yours and it fixes the problem for me.  Any objections before 
I apply it?

 Do we have an established practice of making comments within the source 
around compiler shortcoming/bug workarounds -- so that we can remove such
clutter later on when we no longer support a given compiler's version?

2013-10-29  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* linux-tdep.c (linux_corefile_thread_callback): Preinitialize
	siginfo_size.

  Maciej

gdb-core-siginfo-size-init.diff
Index: gdb-fsf-trunk-quilt/gdb/linux-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/linux-tdep.c	2013-10-26 20:59:40.000000000 +0100
+++ gdb-fsf-trunk-quilt/gdb/linux-tdep.c	2013-10-26 21:01:02.848280142 +0100
@@ -1193,7 +1193,7 @@ linux_corefile_thread_callback (struct t
       struct cleanup *old_chain;
       struct regcache *regcache;
       gdb_byte *siginfo_data;
-      LONGEST siginfo_size;
+      LONGEST siginfo_size = 0;
 
       regcache = get_thread_arch_regcache (info->ptid, args->gdbarch);
 

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

[Steve Ellcey]

On Tue, 2013-10-29 at 17:20 +0000, Maciej W. Rozycki wrote:

>  Here's a change I made last week before I ran out of time; I gather it is 
> the same as yours and it fixes the problem for me.  Any objections before 
> I apply it?

That is the same change I tested here and it worked fine for me.

Steve Ellcey
sellcey@mips.com

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

[Tom Tromey]

Steve> I do not get this error when using a newer GCC (like 4.4.3 or above).  I
Steve> can work around it by setting siginfo_size to 0 when it is declared.  I
Steve> don't know if there is a minimum GCC version required for building gdb
Steve> but my build with GCC 4.1.2 used to work.  Do you think initializing
Steve> siginfo_size is reasonable to allow us to use older GCC's to build gdb?

FWIW, it's routine to commit changes like that in response to any
complaint.

Tom