Re: [PATCH] ia64 unwind directive semantics

[James E Wilson <wilson at specifixinc dot com>]

On Mon, 2005-01-24 at 03:29, Jan Beulich wrote:
> 	* config/tc-ia64.c (unwind): Remove proc_end (now an automatic
> 	variable in dot_endp). Add body and insn. Make prologue,
> 	...

GNU coding conventions require explanatory comments before every
function, though I realize most all of the unwind functions are already
missing comments.  I should really fix this before complaining about it
to others.

The testcases don't test all of the errors you added.  In particular,
the prologue/body before first insn messages.  Also the proc/endp errors
for a missing name, but those ones are obvious enough that I don't see
the need for tests for them.

Hopefully gcc emits unwind info that passes all of these tests. 
Likewise, for assembly code in glibc and the linux kernel.  I will have
to check all of this after all of these patches go in.

I am not sure about one of the proc/endp changes you added.  For proc,
you replaced unwind.proc_start with sym.  But that works only if sym is
defined immediately after the proc.  I would expect that to be true, but
don't think it is safe to assume it.  Also, if a procedure has multiple
entry points, can we really be sure that the first name listed is the
entry point with the lowest address?  Using expr_build_dot here avoids
these problems, and should be perfectly safe.  I am not sure why the
existing code sets proc_start to sym if it is zero.  I think that is a
bug, as it should never be zero here, as expr_build_dot should never
return zero.  I suspect the code originally worked as you wrote it, and
then later got fixed to use expr_build_dot instead of sym because that
didn't work, but we forgot to remove part of the old code.

> +	  if (!sym || !S_IS_DEFINED (sym))
> +	    as_bad ("`%s' was not defined within procedure", name);
> +	  else if (sym && unwind.proc_start

You have a redundant check for sym here.  It is guaranteed to be
non-zero in the second if, so we don't need to check it again.

Otherwise, this looks OK.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com