From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27224 invoked by alias); 16 Oct 2013 20:09:36 -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 27213 invoked by uid 89); 16 Oct 2013 20:09:35 -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, 16 Oct 2013 20:09:35 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1VWXPO-0002XN-0y from Maciej_Rozycki@mentor.com ; Wed, 16 Oct 2013 13:09:30 -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, 16 Oct 2013 13:09:30 -0700 Received: from [172.30.64.75] (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, 16 Oct 2013 21:09:08 +0100 Date: Wed, 16 Oct 2013 20:09:00 -0000 From: "Maciej W. Rozycki" To: Pedro Alves CC: Subject: Re: [PATCH] Avoid producing broken non-native core files In-Reply-To: <525EAF0E.3050801@redhat.com> Message-ID: References: <525EAF0E.3050801@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/msg00499.txt.bz2 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