From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27557 invoked by alias); 23 Oct 2013 22:03:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 27543 invoked by uid 89); 23 Oct 2013 22:03:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 23 Oct 2013 22:03:54 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1VZ6Ws-0003fE-6Y from Maciej_Rozycki@mentor.com ; Wed, 23 Oct 2013 15:03:50 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 23 Oct 2013 15:03:50 -0700 Received: from [172.30.64.175] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Wed, 23 Oct 2013 23:03:47 +0100 Date: Wed, 23 Oct 2013 22:03:00 -0000 From: "Maciej W. Rozycki" To: Pedro Alves CC: Subject: Re: [PATCH] Avoid producing broken non-native core files In-Reply-To: <52614FE1.4040404@redhat.com> Message-ID: References: <525EAF0E.3050801@redhat.com> <52614FE1.4040404@redhat.com> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2013-10/txt/msg00737.txt.bz2 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 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 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 } }