obj_coff_loc Internal Error

obj_coff_loc Internal Error

[Tracy Kuhrt]

The following creates an internal error in obj_coff_loc:
        .loc 1, 200

/home/users/kuhrtt/tmp/loc.s: Assembler messages:
/home/users/kuhrtt/tmp/loc.s:1: Error: Rest of line ignored. First
ignored character is `,'.
/home/users/kuhrtt/tmp/loc.s:1: Internal error, aborting at
/opt/gnudev/roadrunner/src/gnu/acme/gas/config/obj-coff.c line 446 in
add_lineno

The following configure line was used to configure binutils:
../src/configure --target=sparc-sun-coff

There are actually two problems here
1.  Even though loc accepts two arguments, it does not allow for the
arguments to be separated by a comma.
2.  The internal error.

Following is a patch that I think solves the problems.  NOTE:  I have
moved the add_lineno before the listing code.  I am not sure if this is
correct.

Index: gas/config/obj-coff.c
===================================================================
RCS file: /cvs/src/gas/config/obj-coff.c,v
retrieving revision 1.1.1.4
diff -u -p -r1.1.1.4 obj-coff.c
--- gas/config/obj-coff.c       27 Jul 2001 21:46:28 -0000      1.1.1.4
+++ gas/config/obj-coff.c       7 Sep 2001 18:14:11 -0000
@@ -545,10 +545,24 @@ obj_coff_loc (ignore)
   /* Skip the file number.  */
   SKIP_WHITESPACE ();
   get_absolute_expression ();
+
+  /* Skip the comma if it exists; otherwise, give warning.  */
+  if (*input_line_pointer == ',')
+    input_line_pointer++;
+  else
+    as_warn (_("missing comma assumed."));
+
   SKIP_WHITESPACE ();

   lineno = get_absolute_expression ();

+  /* If there is no lineno symbol, treat a .loc directive as if it were
a
+     .appline directive.  */
+  if (current_lineno_sym == NULL)
+    new_logical_line ((char *) NULL, lineno - 1);
+  else
+    add_lineno (frag_now, frag_now_fix (), lineno);
+
 #ifndef NO_LISTING
   {
     extern int listing;
@@ -562,8 +576,6 @@ obj_coff_loc (ignore)
 #endif

   demand_empty_rest_of_line ();
-
-  add_lineno (frag_now, frag_now_fix (), lineno);
 }

 /* Handle the .ident pseudo-op.  */

Re: obj_coff_loc Internal Error

[Nick Clifton]

Hi Tracy,

> Following is a patch that I think solves the problems.

First of all - do you have a binutils copyright assignment on file
with the FSF ?  Without such an assignment we cannot accept your
patch.  (I know that the patch is small, but I consider it to be
non-trivial and not-obvious, so we do need the assignment).


> +  /* Skip the comma if it exists; otherwise, give warning.  */
> +  if (*input_line_pointer == ',')
> +    input_line_pointer++;
> +  else
> +    as_warn (_("missing comma assumed."));

Secondly I am not sure that it is correct to issue a warning if the
comma is missing.  The code before your patch parsed the expression
without requiring an line number and I feel that we should continue to
support that behaviour unless there is a good reason not too.


> NOTE:  I have moved the add_lineno before the listing code.  I am
> not sure if this is correct.

This does look wrong, or at least different.  The listing code does
change the variable 'lineno' which is used by the call to add_lineno()
so the behaviour will change.  Why did you move the call ?  Were you
detecting incorrect line numbers in your output ?


Finally, since you are changing the behaviour of the .loc directive, it
would be great if you could add some documentation about it to the
assembler doc file (gas/doc/as.texinfo).  This is optional, but nice.

Cheers
        Nick

Re: obj_coff_loc Internal Error

[Tracy Kuhrt]

Nick,


> Hi Tracy,
>
> > Following is a patch that I think solves the problems.
>
> First of all - do you have a binutils copyright assignment on file
> with the FSF ?  Without such an assignment we cannot accept your
> patch.  (I know that the patch is small, but I consider it to be
> non-trivial and not-obvious, so we do need the assignment).

I am working on this.  I have yet to receive the form from FSF.

>
>
> > +  /* Skip the comma if it exists; otherwise, give warning.  */
> > +  if (*input_line_pointer == ',')
> > +    input_line_pointer++;
> > +  else
> > +    as_warn (_("missing comma assumed."));
>
> Secondly I am not sure that it is correct to issue a warning if the
> comma is missing.  The code before your patch parsed the expression
> without requiring an line number and I feel that we should continue to
> support that behaviour unless there is a good reason not too.

We can remove the warning, but I thought it strange for the directive to
accept two arguments that did not require a comma.

>
>
> > NOTE:  I have moved the add_lineno before the listing code.  I am
> > not sure if this is correct.
>
> This does look wrong, or at least different.  The listing code does
> change the variable 'lineno' which is used by the call to add_lineno()
> so the behaviour will change.  Why did you move the call ?  Were you
> detecting incorrect line numbers in your output ?

I could not find any documentation on this directive except for the
comment block before the directive.

/* .loc is essentially the same as .ln; parse it for assembler
   compatibility.  */

I made an assumption that if it were like .ln, then we would want it to
perform like .ln.  Maybe "essentially" means that it actually acts
differently (?)

>
>
> Finally, since you are changing the behaviour of the .loc directive, it
> would be great if you could add some documentation about it to the
> assembler doc file (gas/doc/as.texinfo).  This is optional, but nice.

I do not see any documentation for this directive in gas/doc/as.texinfo.
I wanted to make sure I understood the directive (and was suggesting the
correct patch) before adding any documentation.

Tracy

Re: obj_coff_loc Internal Error

[Nick Clifton]

Hi Tracy,

> > First of all - do you have a binutils copyright assignment on file
> > with the FSF ?  Without such an assignment we cannot accept your
> > patch.  (I know that the patch is small, but I consider it to be
> > non-trivial and not-obvious, so we do need the assignment).
> 
> I am working on this.  I have yet to receive the form from FSF.

Is this the form you fill out to request copyright assignment ?  I can
send you a copy directly if you wish.


> > Secondly I am not sure that it is correct to issue a warning if the
> > comma is missing.  The code before your patch parsed the expression
> > without requiring an line number and I feel that we should continue to
> > support that behaviour unless there is a good reason not too.
> 
> We can remove the warning, but I thought it strange for the
> directive to accept two arguments that did not require a comma.

True, but it is possible that there are assemblers out there that
support this syntax.  Certainly this is what is implied by the
current code.

> > This does look wrong, or at least different.  The listing code does
> > change the variable 'lineno' which is used by the call to add_lineno()
> > so the behaviour will change.  Why did you move the call ?  Were you
> > detecting incorrect line numbers in your output ?
> 
> I could not find any documentation on this directive except for the
> comment block before the directive.
> 
> /* .loc is essentially the same as .ln; parse it for assembler
>    compatibility.  */
> 
> I made an assumption that if it were like .ln, then we would want it to
> perform like .ln.  Maybe "essentially" means that it actually acts
> differently (?)

It could :-)  Operating on the principle of "if ain't broke, don;t fix
it" however, I would suggest leaving the current code ordering alone
until such time as someone can prove that it is wrong.

> > Finally, since you are changing the behaviour of the .loc directive, it
> > would be great if you could add some documentation about it to the
> > assembler doc file (gas/doc/as.texinfo).  This is optional, but nice.
> 
> I do not see any documentation for this directive in gas/doc/as.texinfo.
> I wanted to make sure I understood the directive (and was suggesting the
> correct patch) before adding any documentation.

Fair enough.  Of course by writing the documentation you also get to
define the behaviour. :-)  Actually the lack of documentation on this
and similar directives was why I suggested that you write something in
the first place.  If you document this directive you, or someone else
might be motivated to document the others, which in my opinion at
least, would be a good thing.

Cheers
        Nick

Re: obj_coff_loc Internal Error

[Tracy Kuhrt]

Nick,

> Hi Tracy,
>
> > > First of all - do you have a binutils copyright assignment on file
> > > with the FSF ?  Without such an assignment we cannot accept your
> > > patch.  (I know that the patch is small, but I consider it to be
> > > non-trivial and not-obvious, so we do need the assignment).
> >
> > I am working on this.  I have yet to receive the form from FSF.
>
> Is this the form you fill out to request copyright assignment ?  I can
> send you a copy directly if you wish.

I actually just received it in the mail.  Now I have to get my employer to
review and sign.

> > > Secondly I am not sure that it is correct to issue a warning if the
> > > comma is missing.  The code before your patch parsed the expression
> > > without requiring an line number and I feel that we should continue to
> > > support that behaviour unless there is a good reason not too.
> >
> > We can remove the warning, but I thought it strange for the
> > directive to accept two arguments that did not require a comma.
>
> True, but it is possible that there are assemblers out there that
> support this syntax.  Certainly this is what is implied by the
> current code.

So if we accept a comma, but don't warn if there is not one that would be
acceptable?

> > > This does look wrong, or at least different.  The listing code does
> > > change the variable 'lineno' which is used by the call to add_lineno()
> > > so the behaviour will change.  Why did you move the call ?  Were you
> > > detecting incorrect line numbers in your output ?
> >
> > I could not find any documentation on this directive except for the
> > comment block before the directive.
> >
> > /* .loc is essentially the same as .ln; parse it for assembler
> >    compatibility.  */
> >
> > I made an assumption that if it were like .ln, then we would want it to
> > perform like .ln.  Maybe "essentially" means that it actually acts
> > differently (?)
>
> It could :-)  Operating on the principle of "if ain't broke, don;t fix
> it" however, I would suggest leaving the current code ordering alone
> until such time as someone can prove that it is wrong.

Gotcha.

> > > Finally, since you are changing the behaviour of the .loc directive, it
> > > would be great if you could add some documentation about it to the
> > > assembler doc file (gas/doc/as.texinfo).  This is optional, but nice.
> >
> > I do not see any documentation for this directive in gas/doc/as.texinfo.
> > I wanted to make sure I understood the directive (and was suggesting the
> > correct patch) before adding any documentation.
>
> Fair enough.  Of course by writing the documentation you also get to
> define the behaviour. :-)  Actually the lack of documentation on this
> and similar directives was why I suggested that you write something in
> the first place.  If you document this directive you, or someone else
> might be motivated to document the others, which in my opinion at
> least, would be a good thing.

I will see what I can do when I get my copyright assignment on file.

Tracy

Re: obj_coff_loc Internal Error

[Nick Clifton]

Hi Tracy,

> > True, but it is possible that there are assemblers out there that
> > support this syntax.  Certainly this is what is implied by the
> > current code.
> 
> So if we accept a comma, but don't warn if there is not one that would be
> acceptable?

Yes.

Cheers
        Nick