Bug 43631 - var-tracking inserts notes with non-NULL BLOCK_FOR_INSN in between basic blocks
Summary: var-tracking inserts notes with non-NULL BLOCK_FOR_INSN in between basic blocks
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: 4.9.0
Assignee: Steven Bosscher
URL: http://gcc.gnu.org/ml/gcc-patches/201...
Keywords: wrong-debug
: 55615 55714 60141 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-04-02 15:25 UTC by Steven Bosscher
Modified: 2015-02-01 21:56 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-07-17 22:56:39


Attachments
gcc48-pr43631.patch (799 bytes, patch)
2012-11-13 10:50 UTC, Jakub Jelinek
Details | Diff
gcc48-pr43631.patch (1.09 KB, patch)
2012-12-07 12:11 UTC, Jakub Jelinek
Details | Diff
Improve handling of BLOCK_FOR_INSN on notes (2.83 KB, patch)
2013-04-06 12:38 UTC, Steven Bosscher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Bosscher 2010-04-02 15:25:26 UTC
Seen on ia64-unknown-linux, but probably reproducible elsewhere, also:

$ cat t.c
typedef unsigned int UDItype __attribute__ ((mode (DI)));
typedef int TItype __attribute__ ((mode (TI)));

struct DWstruct {UDItype low, high;};
typedef union
{
  struct DWstruct s;
  TItype ll;
} DWunion;

TItype
__multi3 (TItype u, TItype v)
{
  const DWunion uu = {.ll = u};
  const DWunion vv = {.ll = v};
  DWunion w = {.ll = ({DWunion __w; __asm__ ("xma.hu %0 = %2, %3, f0\n\txma.l %1 = %2, %3, f0" : "=&f" (__w.s.high), "=f" (__w.s.low) : "f" (uu.s.low), "f" (vv.s.low)); __w.ll; })};
  w.s.high += (uu.s.low * vv.s.high + uu.s.high * vv.s.low);
  return w.ll;
}
$ ./cc1 -quiet -g -O2 t.c
t.c: In function '__multi3':
t.c:19:1: error: insn 111 outside of basic blocks has non-NULL bb field
t.c:19:1: error: insn 110 outside of basic blocks has non-NULL bb field
t.c:19: confused by earlier errors, bailing out


Needs a patch to var-tracking.c to expose the problem:
Index: var-tracking.c
===================================================================
--- var-tracking.c      (revision 157942)
+++ var-tracking.c      (working copy)
@@ -115,6 +115,8 @@
 #include "pointer-set.h"
 #include "recog.h"
 
+#undef ENABLE_CHECKING
+#define ENABLE_CHECKING 1
 /* var-tracking.c assumes that tree code with the same value as VALUE rtx code
    has no chance to appear in REG_EXPR/MEM_EXPRs and isn't a decl.
    Currently the value is the same as IDENTIFIER_NODE, which has such
@@ -8489,6 +8491,7 @@ variable_tracking_main (void)
 
   flag_var_tracking_assignments = save;
 
+  verify_flow_info ();
   return ret;
 }
 

This comes from emit_notes_for_differences. I have no idea what the purpose is of the notes between basic blocks. Apparently the CFG has to be frozen at this point, or the notes make no sense to begin with. But it's also not clear to me where the notes should be emitted.

Help sought from a VTA guy, i.e. Jakub or Alexandre :-)
Comment 1 Steven Bosscher 2010-07-17 22:56:39 UTC
Jakub, please do not forget about this one for stage1 GCC 4.6.
Comment 2 Steven Bosscher 2010-12-17 21:43:13 UTC
Jakub, ping?
Comment 3 Steven Bosscher 2011-03-28 17:18:25 UTC
Jakub, please do not forget about this one for stage1 GCC 4.7.
Comment 4 Steven Bosscher 2012-03-17 00:05:48 UTC
(In reply to comment #3)
> Jakub, please do not forget about this one for stage1 GCC 4.7.

Jakub, please do not forget about this one for stage1 GCC 4.8.
Comment 5 Jakub Jelinek 2012-11-13 10:50:17 UTC
Created attachment 28673 [details]
gcc48-pr43631.patch

Is this what you meant?  make check-gcc RUNTESTFLAGS=guality.exp doesn't ICE with it anymore with verify_flow_info (); at the beginning as well as end of variable_tracking_main.
The notes are intentionally in between bbs, and it is assumed that the cfg doesn't change after var-tracking pass, otherwise targets should disable var-tracking at its standard spot and perform it at the end of machine reorg pass (as e.g. ia64 and a few other targets do).
Comment 6 Steven Bosscher 2012-11-13 23:38:55 UTC
(In reply to comment #5)
> Created attachment 28673 [details]
> gcc48-pr43631.patch
> 
> Is this what you meant?

Yes, that's exactly what I meant, thanks!
Comment 7 Jakub Jelinek 2012-12-06 14:38:10 UTC
Author: jakub
Date: Thu Dec  6 14:37:59 2012
New Revision: 194252

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=194252
Log:
	PR middle-end/43631
	* var-tracking.c (emit_note_insn_var_location, emit_notes_in_bb):
	Clear BLOCK_FOR_INSN on notes emitted in between basic blocks,
	don't adjust BB_END when inserting note after BB_END of some bb.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/var-tracking.c
Comment 8 Jakub Jelinek 2012-12-06 14:47:26 UTC
Fixed.
Comment 9 Igor Zamyatin 2012-12-07 10:11:28 UTC
Fix led to bootstrap failure with --with-arch=core2 --with-cpu=atom flags. It happens for
/export/gnu/import/git/gcc-test-ia32/bld/./gcc/xgcc -shared-libgcc -B/export/gnu/import/git/gcc-test-ia32/bld/./gcc -nostdinc++ -L/export/gnu/import/git/gcc-test-ia32/bld/i686-linux/libstdc++-v3/src -L/export/gnu/import/git/gcc-test-ia32/bld/i686-linux/libstdc++-v3/src/.libs -B/usr/local/i686-linux/bin/ -B/usr/local/i686-linux/lib/ -isystem /usr/local/i686-linux/include -isystem /usr/local/i686-linux/sys-include -I/export/gnu/import/git/gcc-test-ia32/src-trunk/libstdc++-v3/../libgcc -I/export/gnu/import/git/gcc-test-ia32/bld/i686-linux/libstdc++-v3/include/i686-linux -I/export/gnu/import/git/gcc-test-ia32/bld/i686-linux/libstdc++-v3/include -I/export/gnu/import/git/gcc-test-ia32/src-trunk/libstdc++-v3/libsupc++ -D_GLIBCXX_SHARED -Wall -Wextra -Wwrite-strings -Wcast-qual -Wabi -fdiagnostics-show-location=once -ffunction-sections -fdata-sections -frandom-seed=eh_personality.lo -g -O2 -D_GNU_SOURCE -c ../../../../src-trunk/libstdc++-v3/libsupc++/eh_personality.cc  -fPIC -DPIC -D_GLIBCXX_SHARED -o eh_personality.o

Stack is

0x8b290da distance_non_agu_define_in_bb
	../../src-trunk/gcc/config/i386/i386.c:16945
0x8b2928f distance_non_agu_define
	../../src-trunk/gcc/config/i386/i386.c:17003
0x8b2968f ix86_lea_outperforms
	../../src-trunk/gcc/config/i386/i386.c:17169
0x8b29994 ix86_use_lea_for_mov(rtx_def*, rtx_def**)
	../../src-trunk/gcc/config/i386/i386.c:17279
0x8c5704c output_78
	../../src-trunk/gcc/config/i386/i386.md:2191
0x85a6ae7 get_insn_template(int, rtx_def*)
	../../src-trunk/gcc/final.c:1939
0x85a7ee0 final_scan_insn(rtx_def*, _IO_FILE*, int, int, int*)
	../../src-trunk/gcc/final.c:2792
0x85a69b1 final(rtx_def*, _IO_FILE*, int)
	../../src-trunk/gcc/final.c:1908
0x85aa397 rest_of_handle_final
	../../src-trunk/gcc/final.c:4271
Please submit a full bug report,

Looks like there is some garbage in BLOCK_FOR_INSN field for barrier instruction...
Comment 10 stevenb.gcc@gmail.com 2012-12-07 10:16:49 UTC
On Fri, Dec 7, 2012 at 11:11 AM, izamyatin at gmail dot com wrote:
> Looks like there is some garbage in BLOCK_FOR_INSN field for barrier
> instruction...

A BARRIER  doesn't even have a BLOCK_FOR_INSN. It's not an insn, after all.

Where/how do you end up looking at BLOCK_FOR_INSN for a BARRIER?
Comment 11 Jakub Jelinek 2012-12-07 10:22:05 UTC
Yeah, BLOCK_FOR_INSN is invalid on BARRIER, reproduced with a RTL checking build now, where RTL checking fails.  Looking into it.
Comment 12 Igor Zamyatin 2012-12-07 10:24:04 UTC
Oh, right, in this case just more checks are needed for distance_non_agu_define_in_bb in i386.c. I'll add them. Thanks!
Comment 13 Jakub Jelinek 2012-12-07 10:30:30 UTC
Please hold on with that, seems BB_END is BARRIER, which is wrong.  Starting with distilling a testcase...
Comment 14 Steven Bosscher 2012-12-07 11:59:15 UTC
(In reply to comment #13)
> Please hold on with that, seems BB_END is BARRIER, which is wrong.
> Starting with distilling a testcase...

Perhaps Alex' is right that the real problem is in how BLOCK_FOR_INSN
is handled in add_insn_after and add_insn_before.  Jakub, does the
following make sense to you?

Index: emit-rtl.c
===================================================================
--- emit-rtl.c  (revision 194229)
+++ emit-rtl.c  (working copy)
@@ -3845,19 +3845,25 @@ add_insn_after (rtx insn, rtx after, bas
       gcc_assert (stack);
     }

-  if (!BARRIER_P (after)
-      && !BARRIER_P (insn)
-      && (bb = BLOCK_FOR_INSN (after)))
+  /* If the new insn is a barrier, bb should be NULL because a barrier
+     is never inside a basic block.  Also avoid emitting double barriers.  */
+  if (BARRIER_P (insn))
+    gcc_checking_assert (bb == NULL && !BARRIER_P (after));
+  /* Otherwise, try to deduce a basic block from AFTER.  */
+  else if (!bb
+          && !BARRIER_P (after)
+          && BLOCK_FOR_INSN (after) != NULL
+          && after != BB_END (BLOCK_FOR_INSN (after)))
+    bb = BLOCK_FOR_INSN (after);
+
+  if (bb)
     {
       set_block_for_insn (insn, bb);
       if (INSN_P (insn))
        df_insn_rescan (insn);
-      /* Should not happen as first in the BB is always
-        either NOTE or LABEL.  */
-      if (BB_END (bb) == after
-         /* Avoid clobbering of structure when creating new BB.  */
-         && !BARRIER_P (insn)
-         && !NOTE_INSN_BASIC_BLOCK_P (insn))
+      /* Move BB_END from AFTER to INSN unless INSN is a note (a note cannot
+        be the end a basic block).  */
+      if (BB_END (bb) == after && !NOTE_P (insn))
        BB_END (bb) = insn;
     }
Comment 15 Jakub Jelinek 2012-12-07 12:11:24 UTC
Created attachment 28890 [details]
gcc48-pr43631.patch

No, initially I've fixed this up in cleanup_barriers, to allow NOTE_INSN_CALL_ARG_LOCATION in between noreturn CALL_INSN and following BARRIER, which is what var-tracking was emitting.
But, double checking dwarf2out.c reveals that we could indeed emit the notes after BARRIER instead if there is any.  So I'm leaning towards this patch.
Comment 16 Uroš Bizjak 2012-12-07 12:46:45 UTC
*** Bug 55615 has been marked as a duplicate of this bug. ***
Comment 17 Igor Zamyatin 2012-12-10 08:02:54 UTC
(In reply to comment #15)
> But, double checking dwarf2out.c reveals that we could indeed emit the notes
> after BARRIER instead if there is any.  So I'm leaning towards this patch.

Atom bootstrap is ok with this patch
Comment 18 Jakub Jelinek 2012-12-11 10:41:55 UTC
Author: jakub
Date: Tue Dec 11 10:41:44 2012
New Revision: 194392

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=194392
Log:
	PR middle-end/43631
	PR bootstrap/55615
	* var-tracking.c (emit_note_insn_var_location): If insn is followed
	by BARRIER, put note after the BARRIER.
	(next_non_note_insn_var_location): Skip over BARRIERs.
	(emit_notes_in_bb): If call is followed by BARRIER, put note after
	the BARRIER.

	* g++.dg/other/pr43631.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/other/pr43631.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/var-tracking.c
Comment 19 Jakub Jelinek 2012-12-11 10:49:24 UTC
Fixed.
Comment 20 Jakub Jelinek 2012-12-12 09:56:28 UTC
Author: jakub
Date: Wed Dec 12 09:56:22 2012
New Revision: 194442

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=194442
Log:
	PR target/55659
	Revert
	2012-12-11  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/43631
	* var-tracking.c (emit_note_insn_var_location): If insn is followed
	by BARRIER, put note after the BARRIER.
	(next_non_note_insn_var_location): Skip over BARRIERs.
	(emit_notes_in_bb): If call is followed by BARRIER, put note after
	the BARRIER.

	2012-12-06  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/43631
	* var-tracking.c (emit_note_insn_var_location, emit_notes_in_bb):
	Clear BLOCK_FOR_INSN on notes emitted in between basic blocks,
	don't adjust BB_END when inserting note after BB_END of some bb.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/var-tracking.c
Comment 21 Jakub Jelinek 2012-12-12 10:00:55 UTC
Reopening, as the patch has been reverted.
Comment 22 Uroš Bizjak 2012-12-17 08:48:24 UTC
*** Bug 55714 has been marked as a duplicate of this bug. ***
Comment 23 Steven Bosscher 2013-04-03 20:29:58 UTC
Time for another attempt please? Now that stage1 is open?
Comment 24 Steven Bosscher 2013-04-06 12:38:54 UTC
Created attachment 29814 [details]
Improve handling of BLOCK_FOR_INSN on notes

This is what I'm testing now. Looks good so far, so attaching here
in anticipation of success :-)
Comment 25 Steven Bosscher 2013-04-16 20:12:47 UTC
Three years. Three years! Apparently "wrong-debug" is more important
to fix than "wrong compiler"...  But oh well.  Fixed now.  var-tracking.c
now runs with TODO_verify_flow_info, so that the machine dependent reorg
passes don't inherit the messy excuse for a CFG that var-tracking used to
leave behind!
Comment 26 tejohnson 2014-02-13 21:15:38 UTC
Author: tejohnson
Date: Thu Feb 13 21:15:06 2014
New Revision: 207766

URL: http://gcc.gnu.org/viewcvs?rev=207766&root=gcc&view=rev
Log:
2014-02-13  Teresa Johnson  <tejohnson@google.com>

        For Google b/12971524, backport r197994 to fix PR60141.

        2013-04-16  Steven Bosscher  <steven@gcc.gnu.org>

	PR middle-end/43631
	* emit-rtl.c (make_note_raw): New function.
	(link_insn_into_chain): New static inline function.
	(add_insn): Use it.
	(add_insn_before, add_insn_after): Factor insn chain linking code...
	(add_insn_before_nobb, add_insn_after_nobb): ...here, new functions
	using link_insn_into_chain.
	(note_outside_basic_block_p): New helper function for emit_note_after
	and emit_note_before.
	(emit_note_after): Use nobb variant of add_insn_after if the note
	should not be contained in a basic block.
	(emit_note_before): Use nobb variant of add_insn_before if the note
	should not be contained in a basic block.
	(emit_note_copy): Use make_note_raw.
	(emit_note): Likewise.
	* bb-reorder.c (insert_section_boundary_note): Remove hack to set
	BLOCK_FOR_INSN to NULL manually for NOTE_INSN_SWITCH_TEXT_SECTIONS.
	* jump.c (cleanup_barriers): Use reorder_insns_nobb to avoid making
	the moved barrier the tail of the basic block it follows.
	* var-tracking.c (pass_variable_tracking): Add TODO_verify_flow.

Modified:
    branches/google/gcc-4_8/gcc/bb-reorder.c
    branches/google/gcc-4_8/gcc/emit-rtl.c
    branches/google/gcc-4_8/gcc/jump.c
    branches/google/gcc-4_8/gcc/var-tracking.c
Comment 27 Uroš Bizjak 2015-01-20 15:32:45 UTC
*** Bug 60141 has been marked as a duplicate of this bug. ***
Comment 28 Jakub Jelinek 2015-02-01 21:56:35 UTC
Author: jakub
Date: Sun Feb  1 21:56:03 2015
New Revision: 220336

URL: https://gcc.gnu.org/viewcvs?rev=220336&root=gcc&view=rev
Log:
	Backported from mainline
	2015-01-27  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/61058
	* jump.c (cleanup_barriers): Update basic block boundaries
	if BLOCK_FOR_INSN is non-NULL on PREV.

	* gcc.dg/pr61058.c: New test.

	2013-04-16  Steven Bosscher  <steven@gcc.gnu.org>

	PR middle-end/43631
	* jump.c (cleanup_barriers): Use reorder_insns_nobb to avoid making
	the moved barrier the tail of the basic block it follows.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr61058.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/jump.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog