This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PATCH: Add -munwind-check=[none|warning|error]


On Thu, 2005-02-10 at 09:29, H. J. Lu wrote:
> Here is the updated patch to add -munwind-check=[none|warning|error].
> I documented this new option.

My feeling on this issue in general that we should be emitting a
diagnostic here always.  This new code is finding latent bugs in the
unwind info.  This is a correctness issue.  If we let the latent bugs
through, then they will only be found if an exception is generated and
the stack unwound, at which point it is far too late to recover from the
mistake.  This makes it very important for the assembler to give a
diagnostic when we find a problem.  I can live with this being a warning
for now, to avoid compatibility problems, but it really is important
that people fix their code if they want it to work correctly.

You seem to be going to a lot of unnecessary trouble here.  Why not just
change the as_bad calls to unwind_diagnostic, which then calls as_warn
or as_bad as appropriate?  That would seem to be a lot simpler, and
avoid a lot of duplicated strings.

As far as I can tell, most of this complexity is only to ensure that we
always get an error for a .endp outside a procedure.  That is easy
enough to continue doing just by forcing unwind_check to error before
the unwind_diagnostic call here and then restoring the original value
after the call.

If there is some reason why we need to do it this way, then in_prologue,
in_body, and in_procedure needs comments explaining the meaning of their
return values, as they are no longer obvious true/false return values. 
It would also be nice to document why we need to do things this way.

In two places you have

> +    case -1:
> +      /* We aren't in procedure.  */
> +      if (md.unwind_check != error)
> +	return -1;

But since in_procedure can return -1 only if unwind_check != error, this
appears to be a redundant test.

> +    case -1:
> +      /* We can't ignore this error.  */
> +      as_fatal (".endp can't be outside of procedure");

I believe there is a duplicate diagnostic emitted in this case, when
unwind_check == warning.  First we will get a warning that endp is
outside a procedure, then we get this error.  If we can't ignore this
error, then we shouldn't have emitted the warning in the first place.

As I mentioned before, I think a better way to handle this case is to
force unwind_check = error to ensure we get an error.  With this change,
I think all of the -1 return values are no longer needed which
simplifies a lot of code.

You aren't initializing md.unwind_check.  It appears that you are
relying on a trick, that enum warning = 0, and hence the fact that we
zero this somewhere forces the default to be a warning.  However,
nowhere have you documented the fact that enum warning must be zero in
order for this patch to work.  Instead of relying on an undocumented
trick here, I would just suggesting adding a line to initialize it, e.g.
putting
  md.unwind_check = warning;
in ia64_init.

It would be nice if there was a comment saying that the default can be
changed from warning to error at some point in the future as a
reminder.  Presumably this would be in 2.17 or later.

The phrasing in the c-ia64.texi file is a bit awkward.  I would suggest
something like:

These options control what the assembler will do when performing
consistency checks on unwind directives.  @code{-munwind-check=none}
will disable the checks.  @code{-munwind-check=warning} will make the
assembler issue a warning when an unwind directive check fails.  This is
the default.  @code{-munwind-check=error} will make the assembler issue
an error when an unwind directive check fails.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]