Dwarf views broken on ia64?

Message ID 5B0517CD02000078001C4FDA@prv1-mh.provo.novell.com
State New
Headers show
Series
  • Dwarf views broken on ia64?
Related show

Commit Message

Jan Beulich May 23, 2018, 7:27 a.m.
Alexandre, Nick,

one of the two "view number mismatch" errors triggers all over the place
in building target libraries for ia64 in a cross build of gcc 8.1.0 on x86. I
have no idea whether gas or gcc are at fault. To distinguish the two
diagnostics I've added the patch below (may be worth to consider for
master); to allow the build to complete I've also lowered the one that
triggers to a warning (I don't think this would be an appropriate change
in general).

Attached an example of a produced assembly file triggering the
diagnostic twice.

Solution suggestions appreciated,
Jan

Comments

Alexandre Oliva May 25, 2018, 11:28 p.m. | #1
Hello, Jan, Nick,

On May 23, 2018, "Jan Beulich" <JBeulich@suse.com> wrote:

> one of the two "view number mismatch" errors triggers all over the place

> in building target libraries for ia64 in a cross build of gcc 8.1.0 on x86.


Thanks, I managed to duplicate the problem with an ia64-elf uberbaum
build, will look into it.

> Solution suggestions appreciated,


As a temporary work around, I suggest forcing locviews disabled in gcc,
configuring with gcc_cv_as_dwarf2_debug_view=no

-- 
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 Brasil GNU Toolchain Engineer
Alexandre Oliva May 26, 2018, 12:43 a.m. | #2
On May 23, 2018, "Jan Beulich" <JBeulich@suse.com> wrote:

> Solution suggestions appreciated,


The problem seems to be caused by reuse of view symbols.  The same
symbol may be copied to the debug_line associated with multiple slots.
It looks like this fixes it, but I'd appreciate if someone more familiar
with the way ia64 does insn pack bundling could check that this makes
sense.  I'm concerned that, even if this works as is, once GCC tries
again to figure out on its own where view resets should be, and issue
view asserts (view 0) at such points, placement of insns in bundles
might cause views to be issued out of order, and thus end up failing
asserts again.  Could this ever happen?


diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
index e7c0b6e..6d8be4a 100644
--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -624,6 +624,7 @@ dwarf2_consume_line_info (void)
 		     | DWARF2_FLAG_PROLOGUE_END
 		     | DWARF2_FLAG_EPILOGUE_BEGIN);
   current.discriminator = 0;
+  current.view = NULL;
 }
 
 /* Called for each (preferably code) label.  If dwarf2_loc_mark_labels

-- 
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 Brasil GNU Toolchain Engineer
Jan Beulich May 28, 2018, 8:41 a.m. | #3
>>> On 26.05.18 at 02:43, <aoliva@redhat.com> wrote:

> On May 23, 2018, "Jan Beulich" <JBeulich@suse.com> wrote:

> 

>> Solution suggestions appreciated,

> 

> The problem seems to be caused by reuse of view symbols.  The same

> symbol may be copied to the debug_line associated with multiple slots.

> It looks like this fixes it,


Thanks, I'll give this a try (once I find time).

> but I'd appreciate if someone more familiar

> with the way ia64 does insn pack bundling could check that this makes

> sense.  I'm concerned that, even if this works as is, once GCC tries

> again to figure out on its own where view resets should be, and issue

> view asserts (view 0) at such points, placement of insns in bundles

> might cause views to be issued out of order, and thus end up failing

> asserts again.  Could this ever happen?


It's not clear to me to whom you raise this question. I'm certainly not in
the position to answer it, in particular without knowing what those views
are about and how they are supposed to work. From a general viewpoint,
I don't think the bundling of insns should affect any aspect of debug info
generation - individual insns still have separate (albeit fake) addresses
associated with them (while a bundle spans 16 bytes, the [up to] 3 insns
have addresses <bundle-base>, <bundle-base>+1, and <bundle-base>+2
- not sure if this helps).

Jan
Nick Clifton May 29, 2018, 11:20 a.m. | #4
Hi Alex, Hi Jan,

  Would one of you mind opening a PR for this issue ?

  I think that Alex's patch will work, even for bundles,
  as the DWARF2 line debug information is consumed on a
  per-instruction basis, not a per-bundle basis.  (See
  gas/config/tc-ia64.c:md_assemble() where dwarf2_consume_line()
  is called for every instruction but emit_one_bundle() is
  only called once enough instructions have been accumulated).

  So even if gcc tries to reset the view between two instructions
  that are going into the same bundle, the view tracking code in
  dwarf2dbg.c should be able to cope.

Cheers
  Nick
Alexandre Oliva May 30, 2018, 1:15 p.m. | #5
On May 29, 2018, Nick Clifton <nickc@redhat.com> wrote:

>   I think that Alex's patch will work, even for bundles,

>   as the DWARF2 line debug information is consumed on a

>   per-instruction basis, not a per-bundle basis.


My main remaining concern is reordering of view labels, e.g., given:

.loc f1 l1 c1 view .LVU1
insn1
.loc f2 l2 c2 view .LVU2
insn2

could this get processed such that insn1 appears after insn2, and the
line number program gets to (f2,l2,c2) before (f1,l1,c1).  This would
break an assumption in the locview computing logic that assumes views
(within the same subsection) are issued in order.

-- 
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 Brasil GNU Toolchain Engineer
Jan Beulich May 30, 2018, 2:41 p.m. | #6
>>> On 30.05.18 at 15:15, <aoliva@redhat.com> wrote:

> On May 29, 2018, Nick Clifton <nickc@redhat.com> wrote:

> 

>>   I think that Alex's patch will work, even for bundles,

>>   as the DWARF2 line debug information is consumed on a

>>   per-instruction basis, not a per-bundle basis.

> 

> My main remaining concern is reordering of view labels, e.g., given:

> 

> .loc f1 l1 c1 view .LVU1

> insn1

> .loc f2 l2 c2 view .LVU2

> insn2

> 

> could this get processed such that insn1 appears after insn2, and the

> line number program gets to (f2,l2,c2) before (f1,l1,c1).  This would

> break an assumption in the locview computing logic that assumes views

> (within the same subsection) are issued in order.


To my knowledge the ia64 assembler never re-orders insns (and according
to how the architecture works, it also can't imo).

Jan
Nick Clifton May 30, 2018, 2:51 p.m. | #7
Hi Jan, Hi Alex,

>> My main remaining concern is reordering of view labels, e.g., given:


> To my knowledge the ia64 assembler never re-orders insns (and according

> to how the architecture works, it also can't imo).


Right - this is my understanding as well.

Cheers
  Nick
Alexandre Oliva May 31, 2018, 3:35 p.m. | #8
On May 30, 2018, Nick Clifton <nickc@redhat.com> wrote:

> Hi Jan, Hi Alex,

>>> My main remaining concern is reordering of view labels, e.g., given:


>> To my knowledge the ia64 assembler never re-orders insns (and according

>> to how the architecture works, it also can't imo).


> Right - this is my understanding as well.


Excellent.  In this case, I guess this is ok to install, right?

for  gas/ChangeLog

	* dwarf2dbg.c (dwarf2_consume_line_info): Drop view.

diff --git a/gas/dwarf2dbg.c b/gas/dwarf2dbg.c
index e7c0b6e..6d8be4a 100644
--- a/gas/dwarf2dbg.c
+++ b/gas/dwarf2dbg.c
@@ -624,6 +624,7 @@ dwarf2_consume_line_info (void)
 		     | DWARF2_FLAG_PROLOGUE_END
 		     | DWARF2_FLAG_EPILOGUE_BEGIN);
   current.discriminator = 0;
+  current.view = NULL;
 }
 
 /* Called for each (preferably code) label.  If dwarf2_loc_mark_labels


-- 
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 Brasil GNU Toolchain Engineer
Nick Clifton June 1, 2018, 7:08 a.m. | #9
Hi Alex,

> Excellent.  In this case, I guess this is ok to install, right?

> 

> for  gas/ChangeLog

> 

> 	* dwarf2dbg.c (dwarf2_consume_line_info): Drop view.


Yes - approved.

Cheers
  Nick
Jan Beulich June 1, 2018, 7:28 a.m. | #10
>>> On 01.06.18 at 09:08, <nickc@redhat.com> wrote:

>> Excellent.  In this case, I guess this is ok to install, right?

>> 

>> for  gas/ChangeLog

>> 

>> 	* dwarf2dbg.c (dwarf2_consume_line_info): Drop view.

> 

> Yes - approved.


Does either of you have any opinion on the proposed diagnostic
improvements (without the conversion from as_bad() to as_warn()
of course) then?

Jan
Alexandre Oliva June 1, 2018, 12:47 p.m. | #11
On Jun  1, 2018, "Jan Beulich" <JBeulich@suse.com> wrote:

> Does either of you have any opinion on the proposed diagnostic

> improvements (without the conversion from as_bad() to as_warn()

> of course) then?


I agree having more information about the location of the problem is
desirable.  The problem is that the error was only expected to hit at
locview asserts, that check that the view at the point is zero.  In this
case, you don't even have a symbol name, just the literal constant zero.

That it hit in a different situations in which there was symbolic
information just goes to show how unexpected and broken it was for ia64
bundles.

-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free!         FSF Latin America board member
GNU Toolchain Engineer                Free Software Evangelist

Patch

--- binutils-2.30/gas/dwarf2dbg.c
+++ 2.30/gas/dwarf2dbg.c
@@ -368,7 +368,18 @@  set_or_check_view (struct line_entry *e,
       if (viewx.X_op == O_constant)
 	{
 	  if (!value->X_add_number != !viewx.X_add_number)
-	    as_bad (_("view number mismatch"));
+	    {
+	      char val_buf [sizeof (viewx.X_add_number) * 3 + 2];
+	      char vwx_buf [sizeof (viewx.X_add_number) * 3 + 2];
+
+	      if (sizeof (viewx.X_add_number) > sizeof (bfd_vma))
+		abort ();
+
+	      sprintf_vma (val_buf, (bfd_vma) value->X_add_number);
+	      sprintf_vma (vwx_buf, (bfd_vma) viewx.X_add_number);
+	      as_warn (_("view number mismatch (0x%s vs 0x%s)"),
+		       vwx_buf, val_buf);
+	    }
 	}
       /* Record the expression to check it later.  It is the result of
 	 a logical not, thus 0 or 1.  We just add up all such deferred
@@ -2265,7 +2276,13 @@  dwarf2dbg_final_check (void)
       failed = resolve_symbol_value (sym);
       if (!symbol_resolved_p (sym) || failed)
 	{
-	  as_bad (_("view number mismatch"));
+	  const char *name = S_GET_NAME (sym);
+
+	  if (name)
+	    as_bad (_("view number mismatch for `%s'"),
+		    decode_local_label_name ((char *) name));
+	  else
+	    as_bad (_("view number mismatch"));
 	  break;
 	}
     }