Re: [PATCH] ia64: unwind directive handling

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ia64: unwind directive handling

James E Wilson
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


patch.comments (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ia64: unwind directive handling

Jan Beulich
>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.

I'm reading their (broken) wording "grsave saves the rp, ar.pfs, psp, and pr 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 in gas. The use of the immediate form in gcc is why I made, for the time being, 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 unwinder
>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_sprel for a .vframepsp, which definitely is broken. So I continue to think the change is needed, and I clearly agree that kernel, libunwind, and whoever else implements this.

I actually have an unwinder that has been knowing of this encoding since its 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 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.

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 explicit confirmation from Intel, and my experience with other issues we discussed 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 considered 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 also
>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. ias 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, 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 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 duplicate code).

>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.

Yes, this is intentional, for two reasons. First, this matches ias behavior (and using r0 in .spillreg, other than in .save, actually may cause degenerated 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 .spillreg as well): When you have a function at the bottom of a call tree (i.e. the boot code of an OS or the initial thread of a process) you can use ".save rp, r0" to indicate the end of the stack to the unwinder. I used this when we brought up an OS prototype during the early days of ia64, and I'm fairly certain other people use this, too.

>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.

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 want to touch this component unless I absolutely can't avoid it.

>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.

Again, that was intentional: Once converted to unsigned (the type grmask has) there may (almost certainly will) have been top bits stripped off, and that 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 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.  

That's actually stuff I intended to work on once this patch here has been dealt with. The overlaps of subsequent changes would just be too large to deal with. The spelling used is just the one I intended to use for the diagnostics.
The meaning of the "previous spill incomplete", similar to a diagnostic issued by ias, is that there cannot reasonably be unwind directives following a .saveg/.savegf/.savef/.saveb while there weren't sufficiently many instructions 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, which
>is a serious bug, though the linux kernel and libunwind already support unwabi
>so that isn't a problem.

I can't see this missing from SCRA, it's format P10 (it just doesn't mention the word unwabi anywhere). Since this is a prologue region record, the originally 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ia64: unwind directive handling

Cary Coutant
In reply to this post by James E Wilson
> 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?

Jim is right -- there is no psp_psprel unwind directive, and Format P8,
r=0 is unassigned.

If you think about it, storing the previous stack pointer at a location
relative to psp is pointless at best, since you would need to know the
psp in order to recover the psp!

Cary Coutant
IA-64 Runtime Architect
Hewlett-Packard Co.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ia64: unwind directive handling

Jan Beulich
In reply to this post by James E Wilson
>>> Cary Coutant <[hidden email]> 10.06.05 23:52:01 >>>
>> 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?
>
>Jim is right -- there is no psp_psprel unwind directive, and Format P8,
>r=0 is unassigned.

As I said, ias uses this, so it can't be completely invalid.

>If you think about it, storing the previous stack pointer at a location
>relative to psp is pointless at best, since you would need to know the
>psp in order to recover the psp!

Hmm, indeed. This, however, still doesn't mean no change is needed; .vframepsp itself is pointless, then, and support for it should be removed.

Jan

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ia64: unwind directive handling

Jan Beulich
In reply to this post by James E Wilson
So Jim, should I remove .vframepsp altogether in the to-be-updated the patch? Jan

>>> "Jan Beulich" <[hidden email]> 13.06.05 09:06:52 >>>
>>> Cary Coutant <[hidden email]> 10.06.05 23:52:01 >>>
>> 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?
>
>Jim is right -- there is no psp_psprel unwind directive, and Format P8,
>r=0 is unassigned.

As I said, ias uses this, so it can't be completely invalid.

>If you think about it, storing the previous stack pointer at a location
>relative to psp is pointless at best, since you would need to know the
>psp in order to recover the psp!

Hmm, indeed. This, however, still doesn't mean no change is needed; .vframepsp itself is pointless, then, and support for it should be removed.

Jan