From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25715 invoked by alias); 24 Oct 2013 14:32:53 -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 25705 invoked by uid 89); 24 Oct 2013 14:32:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 24 Oct 2013 14:32:52 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9OEWmKP023409 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 24 Oct 2013 10:32:49 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r9OEWkXg014697; Thu, 24 Oct 2013 10:32:47 -0400 Message-ID: <52692F8E.9080304@redhat.com> Date: Thu, 24 Oct 2013 14:32:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: "Maciej W. Rozycki" CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Avoid producing broken non-native core files References: <525EAF0E.3050801@redhat.com> <52614FE1.4040404@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-10/txt/msg00756.txt.bz2 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