From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14269 invoked by alias); 10 Jun 2005 13:21: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 14248 invoked by uid 22791); 10 Jun 2005 13:21:20 -0000 Received: from lyle.provo.novell.com (HELO lyle.provo.novell.com) (137.65.81.174) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Fri, 10 Jun 2005 13:21:20 +0000 Received: from INET-PRV3-MTA by lyle.provo.novell.com with Novell_GroupWise; Fri, 10 Jun 2005 07:21:16 -0600 Message-Id: Date: Fri, 10 Jun 2005 13:21:00 -0000 From: "Jan Beulich" To: Cc: Subject: Re: [PATCH] ia64: unwind directive handling Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline X-SW-Source: 2005-06/txt/msg00263.txt.bz2 >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 histo= rical >mistake. I'm reading their (broken) wording "grsave saves the rp, ar.pfs, psp, and p= r register contents to the first general-purpose register" as implying a gr here, not a constant. Anyway, ias doesn't accept= a constant here, and hence the requirement to at least accept a register i= n gas. The use of the immediate form in gcc is why I made, for the time bei= ng, the deprecation warning conditional upon unwind_check_error. >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 unwin= der >supports it, then it doesn't serve much purpose for gas to support it. Hmm, I understand your concern here, but the code previously issued a psp_s= prel for a .vframepsp, which definitely is broken. So I continue to think t= he change is needed, and I clearly agree that kernel, libunwind, and whoeve= r else implements this. I actually have an unwinder that has been knowing of this encoding since it= s initial implementation. >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 docume= nted. >How do you know that P8 r=3D0 is correct? You confirmed this from ias? I= t 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. The knowledge I have is from ias output. That used to be quite helpful when= working through all the typos/omissions in the doc. I have no other explic= it confirmation from Intel, and my experience with other issues we discusse= d here and I brought up to them is that they get virtually never addressed.= So if we wanted to wait for their confirmation, we well may wait forever. = But to me this is so obvious from all the facts we have; I always considere= d this just a typographic issue with an omitted line in the relevant table. >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 al= so >be a line separator. Did you try that? If so, then that would allow us to >handle them both the same way again. Interesting, I didn't even think about also making '{' a line terminator. i= as indeed also treats it like that. I'll have to rework the patch then. >Putting a switch default case in the middle looks odd to me. Typically, i= t 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 considered that the more desirable solution over duplicating code (and I'= m a strong opponent of goto-s, so I'd rather not use them for avoiding dupl= icate code). >I notice that you allow r0 as a valid for a gr-location in a .save directi= ve. >Does this ever make sense? I noticed this because you deliberately disall= ow >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 v= alid >here. However, the docs don't appear to say anything about using r0 for t= he >destination (gr-location) of a .save directive. This is probably OK as it= is. Yes, this is intentional, for two reasons. First, this matches ias behavior= (and using r0 in .spillreg, other than in .save, actually may cause degene= rated unwind records to be emitted, because the .restorereg utilizes an r0-= encoding). Second, there indeed is a use of this in .save (and if the just = mentioned problem with the encoding didn't exist this could apply to .spill= reg as well): When you have a function at the bottom of a call tree (i.e. t= he boot code of an OS or the initial thread of a process) you can use ".sav= e rp, r0" to indicate the end of the stack to the unwinder. I used this whe= n we brought up an OS prototype during the early days of ia64, and I'm fair= ly certain other people use this, too. >I see you are adding a popcnt function into the middle of the tc-ia64.c fi= le. >This does look a little wierd. I don't have any strong objection to it, e= xcept >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. Adding a comment is no problem, and as I indicate in the comment preceding = it I'm not happy with the placement either. Just I don't know where to put = it. Of course this could live in libiberty, but I'm not sure I actually wan= t to touch this component unless I absolutely can't avoid it. >In dot_saveg, you have > grmask =3D 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. Again, that was intentional: Once converted to unsigned (the type grmask ha= s) there may (almost certainly will) have been top bits stripped off, and t= hat prevents detecting all invalid uses if these bits were non-zero. I was = actually thinking in the opposite direction - change other functions to not= use the converted values, too. >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 docume= nted >here. I'm also left wondering how you generated this file. How do you kn= ow >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.=20=20 That's actually stuff I intended to work on once this patch here has been d= ealt with. The overlaps of subsequent changes would just be too large to de= al with. The spelling used is just the one I intended to use for the diagno= stics. The meaning of the "previous spill incomplete", similar to a diagnostic iss= ued by ias, is that there cannot reasonably be unwind directives following = a .saveg/.savegf/.savef/.saveb while there weren't sufficiently many instru= ctions seen to match the number of bits set in their respective operands. I= actually suspect, without having checked, that there are even more severe = problems with multi-bit uses of these directives; checking and if necessary= solving these was my intention together with adding these new diagnostics. >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, w= hich >is a serious bug, though the linux kernel and libunwind already support un= wabi >so that isn't a problem. I can't see this missing from SCRA, it's format P10 (it just doesn't mentio= n the word unwabi anywhere). Since this is a prologue region record, the or= iginally added check was insufficient. >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). Before doing the rework, I'll wait a couple of days to see whether you have= any further comments, Jan