From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19379 invoked by alias); 11 Feb 2005 02:30:25 -0000 Mailing-List: contact binutils-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sources.redhat.com Received: (qmail 19344 invoked from network); 11 Feb 2005 02:30:18 -0000 Received: from unknown (HELO bluesmobile.specifixinc.com) (64.220.152.98) by sourceware.org with SMTP; 11 Feb 2005 02:30:18 -0000 Received: from [127.0.0.1] (bluesmobile.corp.specifixinc.com [192.168.1.2]) by bluesmobile.specifixinc.com (Postfix) with ESMTP id A802716792; Thu, 10 Feb 2005 18:30:17 -0800 (PST) Subject: Re: PATCH: Add -munwind-check=[none|warning|error] From: James E Wilson To: "H. J. Lu" Cc: binutils@sources.redhat.com In-Reply-To: <20050210172938.GA17132@lucon.org> References: <20050210172938.GA17132@lucon.org> Content-Type: text/plain Message-Id: <1108089017.24473.184.camel@aretha.corp.specifixinc.com> Mime-Version: 1.0 Date: Fri, 11 Feb 2005 10:55:00 -0000 Content-Transfer-Encoding: 7bit X-SW-Source: 2005-02/txt/msg00228.txt.bz2 On Thu, 2005-02-10 at 09:29, H. J. Lu wrote: > Here is the updated patch to add -munwind-check=[none|warning|error]. > I documented this new option. My feeling on this issue in general that we should be emitting a diagnostic here always. This new code is finding latent bugs in the unwind info. This is a correctness issue. If we let the latent bugs through, then they will only be found if an exception is generated and the stack unwound, at which point it is far too late to recover from the mistake. This makes it very important for the assembler to give a diagnostic when we find a problem. I can live with this being a warning for now, to avoid compatibility problems, but it really is important that people fix their code if they want it to work correctly. You seem to be going to a lot of unnecessary trouble here. Why not just change the as_bad calls to unwind_diagnostic, which then calls as_warn or as_bad as appropriate? That would seem to be a lot simpler, and avoid a lot of duplicated strings. As far as I can tell, most of this complexity is only to ensure that we always get an error for a .endp outside a procedure. That is easy enough to continue doing just by forcing unwind_check to error before the unwind_diagnostic call here and then restoring the original value after the call. If there is some reason why we need to do it this way, then in_prologue, in_body, and in_procedure needs comments explaining the meaning of their return values, as they are no longer obvious true/false return values. It would also be nice to document why we need to do things this way. In two places you have > + case -1: > + /* We aren't in procedure. */ > + if (md.unwind_check != error) > + return -1; But since in_procedure can return -1 only if unwind_check != error, this appears to be a redundant test. > + case -1: > + /* We can't ignore this error. */ > + as_fatal (".endp can't be outside of procedure"); I believe there is a duplicate diagnostic emitted in this case, when unwind_check == warning. First we will get a warning that endp is outside a procedure, then we get this error. If we can't ignore this error, then we shouldn't have emitted the warning in the first place. As I mentioned before, I think a better way to handle this case is to force unwind_check = error to ensure we get an error. With this change, I think all of the -1 return values are no longer needed which simplifies a lot of code. You aren't initializing md.unwind_check. It appears that you are relying on a trick, that enum warning = 0, and hence the fact that we zero this somewhere forces the default to be a warning. However, nowhere have you documented the fact that enum warning must be zero in order for this patch to work. Instead of relying on an undocumented trick here, I would just suggesting adding a line to initialize it, e.g. putting md.unwind_check = warning; in ia64_init. It would be nice if there was a comment saying that the default can be changed from warning to error at some point in the future as a reminder. Presumably this would be in 2.17 or later. The phrasing in the c-ia64.texi file is a bit awkward. I would suggest something like: These options control what the assembler will do when performing consistency checks on unwind directives. @code{-munwind-check=none} will disable the checks. @code{-munwind-check=warning} will make the assembler issue a warning when an unwind directive check fails. This is the default. @code{-munwind-check=error} will make the assembler issue an error when an unwind directive check fails. -- Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com