From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2890 invoked by alias); 10 Jun 2005 00:07:04 -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 2835 invoked by uid 22791); 10 Jun 2005 00:06:51 -0000 Received: from w098.z064220152.sjc-ca.dsl.cnc.net (HELO bluesmobile.specifixinc.com) (64.220.152.98) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Fri, 10 Jun 2005 00:06:51 +0000 Received: from [127.0.0.1] (bluesmobile.corp.specifix.com [192.168.1.2]) by bluesmobile.specifixinc.com (Postfix) with ESMTP id 4D3E616AA1; Thu, 9 Jun 2005 17:06:49 -0700 (PDT) Subject: Re: [PATCH] ia64: unwind directive handling From: James E Wilson To: Jan Beulich Cc: binutils@sources.redhat.com In-Reply-To: References: Content-Type: multipart/mixed; boundary="=-CzZyJR47Kqkjg88Xwsjz" Message-Id: <1118362009.30038.60.camel@aretha.corp.specifixinc.com> Mime-Version: 1.0 Date: Fri, 10 Jun 2005 00:07:00 -0000 X-SW-Source: 2005-06/txt/msg00244.txt.bz2 --=-CzZyJR47Kqkjg88Xwsjz Content-Type: text/plain Content-Transfer-Encoding: 7bit Content-length: 465 On Mon, 2005-05-23 at 03:00, Jan Beulich wrote: > The main purpose of this patch is to add parser level support for the > optional tags various of the unwind directives permit and to make '}' a > statement separator as is done in ias. Along with that it consolidates > some redundant functionality, tightens operand checking for many unwind > directives, and fixes some other bugs. Comments attached. -- Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com --=-CzZyJR47Kqkjg88Xwsjz Content-Disposition: attachment; filename=patch.comments Content-Type: text/plain; name=patch.comments; charset=utf-8 Content-Transfer-Encoding: 7bit Content-length: 4874 For .prologue, the assembler docs appear to be ambiguous. It states that the second operand to .prologue is grsave, and then in the following section it defines what grsave means, but no where does it clearly define how grsave is represented in the assembler file. Still, it does seem a bit odd that we ended up using a constant instead of a register name. That was probably a historical mistake. You are changing unwind-ia64.c. There are also copies of this code in the linux kernel and in the libunwind package. So if we change this file, we need to let David Mosberger know, so he can fix the other two occurances. And there is also the gcc unwinder that would need to be fixed. There is the more general question of whether any unwinder supports psp_psprel. If no unwinder supports it, then it doesn't serve much purpose for gas to support it. I checked the SCRA. Appendix B constains tables listing all of the valid unwind encodings. This does not include psp_psprel. I see that it comes from the .vframepsp directive. Curiously, the asm language manual has two typos in the vframepsp docs. It emits two unwind records, and both of them are mispelled. Maybe this is a late change that was never properly documented. How do you know that P8 r=0 is correct? You confirmed this from ias? It would be useful to get confirmation that this is actually supposed to exist, and that we are actually supposed to implement it. We should report these doc bugs to Intel. The asm language manual has so many bugs that probably no one cares about it, but bugs in the SCRA are serious, as it is used for ABI conformance. The SCRA must be fixed if it is wrong. As for '}' as a line separator, as usual, this isn't documented in the asm language manual. Unfortunately, this means that we are now handling '{' and '}' differently. This begs the question of whether perhaps '{' can also be a line separator. Did you try that? If so, then that would allow us to handle them both the same way again. Putting a switch default case in the middle looks odd to me. Typically, it is at the end, after all of the cases. That makes it easier to read the code, since it is easy to see that the default case is there. This is done in dot_save and dot_savemem. I notice that you allow r0 as a valid for a gr-location in a .save directive. Does this ever make sense? I noticed this because you deliberately disallow r0 in other places, such as the first option to .spillreg, but checking the docs, I see that the docs explicitly say that only certain registers are valid here. However, the docs don't appear to say anything about using r0 for the destination (gr-location) of a .save directive. This is probably OK as it is. I see you are adding a popcnt function into the middle of the tc-ia64.c file. This does look a little wierd. I don't have any strong objection to it, except to point out that the GNU coding standards require that every function have an explanatory comment that describes what the function does and you didn't add one. In dot_saveg, you have grmask = e.X_add_number; then you test the value in e.X_add_number. I think that makes more sense as tests again grmask, and that saves some memory references. Similarly in dot_saveb. In dot_spillmem, you have what appears to be an obvious bug. + po = (psprel = ~psprel) ? "spillpsp.p" : "spillsp.p"; I realize it isn't a bug, but the next person looking at the code might think otherwise. The assignment should be a separate statement to avoid confusion. The "first" code in ia64_start_line needs a comment explaining why it is there. You put this in the Changelog entry, but the "why" info always belongs in the source code, not the ChangeLog file. The "what" info goes in the ChangeLog. I see that you have bugs documented as comments in the file gas/testsuite/gas/ia64/unwind-bad.l. That is a very unusual place and way to document bugs. It would be better if this was documented someplace more visible. Probably no one is ever going to notice that the bugs are documented here. I'm also left wondering how you generated this file. How do you know that these error messages are missing? What does the "previous spill incomplete" message mean? It looks almost like you were copying from something else, which might be copyright infringement. I noticed that you change unwabi from in_procedure to in_prologue. Do you have a reason for that? I also noticed that unwabi is missing from the SCRA, which is a serious bug, though the linux kernel and libunwind already support unwabi so that isn't a problem. Otherwise this looks reasonable to me. I think most of these issues are pretty minor except the psp_psprel, and perhaps the unwind-bad.l issue (depending on what the answer is). I'm going to do test builds of the linux kernel, glibc, and gcc to check for problems. --=-CzZyJR47Kqkjg88Xwsjz--