fix -fcompare-debug regression in asm locators

[Alexandre Oliva-2]

asm_operands and asm_inputs carry a “line” locator number, and this
locator number is included in output dumps.  Because -fworking-directory
implicitly adds a “line” to the preprocessor “output” (even without
separate preprocessing, thus the quotes), the numbers printed for the
locators end up different, and -fcompare-debug complains.

I haven't tested BUILD_CONFIG=bootstrap-debug-lib for quite a while, so
I can't tell how recently this regression was introduced.  Anyhow, this
patch takes care of the regression by printing file:line instead of the
locator number.

While at that, I noticed that the line locator number attached to all
asm_operands and asm_inputs is the locator of the closing brace of the
function body, rather than the locator of the end of the asm statement.
I'm not sure why we have locators in these RTXs, but I'm concerned that
perhaps we expected them to be different for different asm stmts, and
that compilation correctness might depend on it, in which case having
the same locator for all asms in a function may lead to problems.  But
even if it's only for error messages, it's still inaccurate and should
probably be fixed, no?

Anyway, that will be for another patch.  Meanwhile, is this one ok?


Index: gcc/print-rtl.c
===================================================================
--- gcc/print-rtl.c.orig 2009-10-16 03:43:42.000000000 -0300
+++ gcc/print-rtl.c 2009-10-16 03:48:48.000000000 -0300
@@ -385,6 +385,22 @@ print_rtx (const_rtx in_rtx)
       fprintf(outfile, " %s:%i", insn_file (in_rtx), insn_line (in_rtx));
 #endif
   }
+ else if (i == 6 && GET_CODE (in_rtx) == ASM_OPERANDS)
+  {
+#ifndef GENERATOR_FILE
+    fprintf (outfile, " %s:%i",
+     locator_file (ASM_OPERANDS_SOURCE_LOCATION (in_rtx)),
+     locator_line (ASM_OPERANDS_SOURCE_LOCATION (in_rtx)));
+#endif
+  }
+ else if (i == 1 && GET_CODE (in_rtx) == ASM_INPUT)
+  {
+#ifndef GENERATOR_FILE
+    fprintf (outfile, " %s:%i",
+     locator_file (ASM_INPUT_SOURCE_LOCATION (in_rtx)),
+     locator_line (ASM_INPUT_SOURCE_LOCATION (in_rtx)));
+#endif
+  }
  else if (i == 6 && NOTE_P (in_rtx))
   {
     /* This field is only used for NOTE_INSN_DELETED_LABEL, and


--
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

[Alexandre Oliva-2]

On Oct 16, 2009, Alexandre Oliva <aoliva@...> wrote:

> Anyway, that will be for another patch.  Meanwhile, is this one ok?

... with this ChangeLog entry?

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@...>

        * print-rtl.c (print_rtx): Print locators in asm_operands
        and asm_input.

> Index: gcc/print-rtl.c

--
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

[Richard Henderson-2]

On 10/16/2009 12:51 AM, Alexandre Oliva wrote:
> * print-rtl.c (print_rtx): Print locators in asm_operands
> and asm_input.

Ok.


r~

[Alexandre Oliva-2]

On Oct 16, 2009, Alexandre Oliva <aoliva@...> wrote:

> While at that, I noticed that the line locator number attached to all
> asm_operands and asm_inputs is the locator of the closing brace of the
> function body, rather than the locator of the end of the asm statement.
> I'm not sure why we have locators in these RTXs, but I'm concerned that
> perhaps we expected them to be different for different asm stmts, and
> that compilation correctness might depend on it, in which case having
> the same locator for all asms in a function may lead to problems.  But
> even if it's only for error messages, it's still inaccurate and should
> probably be fixed, no?

I investigated further.  The only explicit use for these locators is to
issue line directives before the asm code.  They're wildly inaccurate
for this intended purpose.

Ideally, we'd issue the starting line of the string, but this
information is lost long before we reach that point: not even the
ASM_EXPR from which the GIMPLE_ASM statement is constructed gets it.
The parser calls build_asm_expr() with the locus of the asm keyword, and
gets back a tree stmt with that locus to pass to build_asm_stmt, which
doesn't get any other locus.  By that point, the location of the
STRING_CST token is already lost.

There are two ways I see to retain it:

1. pass it to build_asm_expr() instead of the locus of the ASM keyword,
and use that location for the stmt.  It will get to the gimple tuple,
and end up in the RTX insn.  It would render the loci in ASM_OPERANDS
and ASM_INPUT obsolete.

2. pass it to build_asm_expr() in addition to the locus of the ASM
keyword, and add the location_t to the ASM_EXPR tree.  Use it when
expanding GIMPLE_ASMs to RTX, so as to fill in the locus field in
ASM_OPERANDS and ASM_INPUT.

Which way should we go?


In the mean time (I'm going to be away for most of next week), here's a
patch that uses the locus of the stmt, rather than input_location, to
fill in the locus field in ASM_OPERANDS and ASM_INPUT.  Is this ok to
install?



for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@...>

        * stmt.c (expand_asm_stmt): Get locus from stmt.

Index: gcc/stmt.c
===================================================================
--- gcc/stmt.c.orig 2009-10-17 03:52:03.000000000 -0300
+++ gcc/stmt.c 2009-10-17 03:53:13.000000000 -0300
@@ -1099,6 +1099,7 @@ expand_asm_stmt (gimple stmt)
   size_t i, n;
   const char *s;
   tree str, out, in, cl, labels;
+  location_t locus = gimple_location (stmt);
 
   /* Meh... convert the gimple asm operands into real tree lists.
      Eventually we should make all routines work on the vectors instead
@@ -1144,7 +1145,7 @@ expand_asm_stmt (gimple stmt)
 
   if (gimple_asm_input_p (stmt))
     {
-      expand_asm_loc (str, gimple_asm_volatile_p (stmt), input_location);
+      expand_asm_loc (str, gimple_asm_volatile_p (stmt), locus);
       return;
     }
 
@@ -1160,7 +1161,7 @@ expand_asm_stmt (gimple stmt)
   /* Generate the ASM_OPERANDS insn; store into the TREE_VALUEs of
      OUTPUTS some trees for where the values were actually stored.  */
   expand_asm_operands (str, outputs, in, cl, labels,
-       gimple_asm_volatile_p (stmt), input_location);
+       gimple_asm_volatile_p (stmt), locus);
 
   /* Copy all the intermediate outputs into the specified outputs.  */
   for (i = 0, tail = outputs; tail; tail = TREE_CHAIN (tail), i++)


--
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

[Alexandre Oliva-2]

On Oct 17, 2009, Alexandre Oliva <aoliva@...> wrote:

> 1. pass it to build_asm_expr() instead of the locus of the ASM keyword,
> and use that location for the stmt.  It will get to the gimple tuple,
> and end up in the RTX insn.  It would render the loci in ASM_OPERANDS
> and ASM_INPUT obsolete.

> 2. pass it to build_asm_expr() in addition to the locus of the ASM
> keyword, and add the location_t to the ASM_EXPR tree.  Use it when
> expanding GIMPLE_ASMs to RTX, so as to fill in the locus field in
> ASM_OPERANDS and ASM_INPUT.

> Which way should we go?


> In the mean time (I'm going to be away for most of next week), here's a
> patch that uses the locus of the stmt, rather than input_location, to
> fill in the locus field in ASM_OPERANDS and ASM_INPUT.  Is this ok to
> install?

Ping?

> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@...>

> * stmt.c (expand_asm_stmt): Get locus from stmt.

http://gcc.gnu.org/ml/gcc-patches/2009-10/msg01112.html

--
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

[Richard Henderson-2]

> * stmt.c (expand_asm_stmt): Get locus from stmt.

This is ok.

Longer term, I don't see any point to retaining a locus for each asm
operand; it would seem like the locus for the INSN containing the asm
should be good enough.


r~