This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Avoid producing broken non-native core files
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Wed, 23 Oct 2013 23:03:40 +0100
- Subject: Re: [PATCH] Avoid producing broken non-native core files
- Authentication-results: sourceware.org; auth=none
- References: <alpine dot DEB dot 1 dot 10 dot 1310151418580 dot 12843 at tp dot orcam dot me dot uk> <525EAF0E dot 3050801 at redhat dot com> <alpine dot DEB dot 1 dot 10 dot 1310162048030 dot 12843 at tp dot orcam dot me dot uk> <52614FE1 dot 4040404 at redhat dot com>
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
}
}