This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]