Re: [PATCH] ia64: unwind directive handling

[James E Wilson <wilson@specifixinc.com>]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

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


[-- Attachment #2: patch.comments --]
[-- Type: text/plain, Size: 4874 bytes --]

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.