From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6020 invoked by alias); 18 Jul 2014 10:42:05 -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 5941 invoked by uid 89); 18 Jul 2014 10:42:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 18 Jul 2014 10:42:01 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s6IAfxBJ007950 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 18 Jul 2014 06:41:59 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s6IAfutW019588; Fri, 18 Jul 2014 06:41:57 -0400 Message-ID: <53C8F9F4.9090205@redhat.com> Date: Fri, 18 Jul 2014 10:44:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Gary Benson CC: Doug Evans , gdb-patches , Tom Tromey Subject: Re: [PATCH 01/15 v3] Introduce common/errors.h References: <1405520243-17282-1-git-send-email-gbenson@redhat.com> <1405520243-17282-2-git-send-email-gbenson@redhat.com> <20140717134728.GB31916@blade.nx> <53C7E6AB.4080703@redhat.com> <20140717153957.GA1921@blade.nx> <53C7F5D6.6060102@redhat.com> <20140718090630.GA17917@blade.nx> In-Reply-To: <20140718090630.GA17917@blade.nx> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-07/txt/msg00492.txt.bz2 On 07/18/2014 10:06 AM, Gary Benson wrote: > Pedro Alves wrote: >> On 07/17/2014 04:39 PM, Gary Benson wrote: >>> Pedro Alves wrote: >>>> On 07/17/2014 02:47 PM, Gary Benson wrote: >>>>> +/* Throw an error. The current operation will be aborted. The >>>>> + message will be issued to the user. The application will >>>>> + return to a state where it is accepting commands from the user. */ >>>> >>>> These comments aren't really true, though. We have plenty of >>>> places catching errors with TRY_CATCH and proceeding without >>>> returning to a state where we're accepting commands from the >>>> user. >>> >>> How about "Throw an error. The current operation will be aborted. >>> The application may catch and process the error, or, if not, the >>> message will be issued to the user and the application will return >>> to a state where it is accepting commands from the user." >> >> Still infers what the application does with the error. IMO, it's >> best to describe the interface. Like: >> >> Throw a generic error. This function not return, instead >> it executes a long jump, aborting the current operation. >> The function takes printf-style arguments, which are used to >> construct the error message. > > Surely "it executes a long jump" is describing what the application > does with the error? By "does with the error" I was talking about what the application does after the error is thrown, like talking about accepting commands from the user, not really the internal implementation. > Besides, it's not true for IPA, where error > just calls exit (1). Yeah, forgot that, though that's conceptually the same as the IPA having an outer TRY_CATCH and calling exit(1) there. OK, thinking this through. Conceptually, what matters to the user of the API is when to call which error function variant, and this one is to be called on generic errors. That the IPA chooses the treat generic errors as fatal errors, it's IPA's business. That's the detail of what the application decides to do with the error. > > In the context of this whole file, you have: > > - warning (one type) > - errors (three types) > - special cases (perror_with_name, malloc_failure) > > The only difference between the three types of error is what the > application does with them: > > - error (may be handled, may return to command level, may exit) > - fatal (will exit) > - internal_error (may return to command level, may exit) I think that's not the best way to look at this. We're documenting an API, the contract the shared/core code will rely on to decide which function to call. If the contract is different in GDB vs GDBserver then we shall fix up the differences first. So, what is the contract we want ? Where do we want to get to? I think it should be: - warning (something odd detected, but we'll carry on anyway.) - error (generic, non-predicatable, non-fatal condition detected. The requested operation can't proceed. E.g.: trying to compute expressions with optimized values; invalid input;invalid conversion attempted; etc.) - internal_error (faulty logic detected) and then, I think we should just not use 'fatal' as is in common code, and so not move that to common code. It's just confusing and behaves different in GDB and GDBserver. We have a bunch of callers on the GDBserver side, but can't those all be internal_error / gdb_assert? Why not? If not, then I propose really describing 'fatal' as being a non-recoverable fatal error. On the GDB side it's only used in two situations -- throw a QUIT when the user wants to try recovering from an internal error, and, throw a QUIT when the user presses ctrl-c. It's not called when a fatal / internal error is detected, like on the GDBserver side. I think we should rename the existing 'fatal' on the GDB side for clarity to something like throw_quit or some such. How GDB recovers from internal_error is GDB's business. It just happens today, that results in a QUIT being thrown. For situations in GDB where we might have a fatal error that is truly not recoverable, and that should not even consult the user on whether to quit/dump core, etc., then we wouldn't call the existing 'fatal' function as it is. E.g., if GDB's 'fatal' where truly a 'fatal'-like function like GDBserver's, this in main.c: /* Install it. */ if (!interp_set (interp, 1)) { fprintf_unfiltered (gdb_stderr, "Interpreter `%s' failed to initialize.\n", interpreter_p); exit (1); } would be written as: /* Install it. */ if (!interp_set (interp, 1)) { fatal ("Interpreter `%s' failed to initialize.\n", interpreter_p); } with 'fatal' printing the error and exiting, just like gdbserver. I'd support doing that. > I don't think you can properly document these functions without > spelling this out. > > Also, the documentation in this file is not just for callers of the > functions, it has to document them for implementors too. The way to do that is to document the contract callers use to determine the class of error at hand, not what the application does with such class of error. If we're thinking in terms of library, then listing what current applications do just results in comments getting stale as implementations change or more applications are added. > /* Throw an internal error, constructing the message using a printf- > style format string and a printf- or vprintf-style argument list. > This function does not return. Internal errors indicate > programming errors such as assertion failures, as opposed to more > general errors the user may encounter. Internal errors are treated > as fatal errors, except that the application may offer the user the > choice to return to the command level rather than exiting. */ Here we're talking about "the command level", which doesn't even exist on GDBserver or the IPA. And I'm now thinking that saying "Throw" here isn't exactly the best to say. Instead, I think it'd be better to say something like "Call when an internal error was detected.", and don't even talk about the command level. Like: /* An internal error was detected. Internal errors indicate programming errors such as assertion failures, as opposed to more general errors beyond the application's control. This function does not return, and constructs an error message using a printf-style argument list. FILE and LINE indicate the file and line number where the programming error was detected. Thanks, -- Pedro Alves