PR59723: fix LTO + fortran namelist ICEs
diff mbox

Message ID 52DEF7C9.5020700@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Jan. 21, 2014, 10:42 p.m. UTC
Hi folks.

The problem here is that while streaming the DECL_NAME with 
stream_read_tree() and consequently lto_output_tree(), we trigger an ICE 
because we are recursing on the DFS walk:

   else
     {
       /* This is the first time we see EXPR, write all reachable
	 trees to OB.  */
       static bool in_dfs_walk;

       /* Protect against recursion which means disconnect between
          what tree edges we walk in the DFS walk and what edges
	 we stream out.  */
       gcc_assert (!in_dfs_walk);

In the namelist fortran testcases, this is the first time we see the 
DECL_NAMEs, so we ICE.  I fixed this by outputting the DECL_NAME's 
string with streamer_write_string() instead.

[I honestly wondered why we don't stream the entire NAMELIST_DECL the 
same way we stream IMPORTED_DECL, all in one go, instead of piecemeal 
like the present code does.  But LTO is this complicated black box in my 
head that I try not to think about too much...the current patch touches 
as little as possible.]

This change alone fixes the problems in the PR, but I also found another 
ICE now that streaming actually works: dwarf.  It turns out, that the 
way the CONSTRUCTOR elements in the NAMELIST_DECL are streamed, a 
PARM_DECL that has been previously seen (in the function's 
DECL_ARGUMENTS) will be streamed with different reference magic (or 
whatever streamed references or ids are called in LTO speak).  So when 
we read the CONSTRUCTOR elements in the LTO read phase, we end up with 
different pointers for a PARM_DECL in the constructor for the seemingly 
same PARM_DECL in the function's arguments (DECL_ARGUMENTS).

I don't understand LTO very well, but I do see that using 
stream_read_tree (lto_output_tree) caches the entries, so it seems like 
a good fit to avoid writing two distinct items for the same PARM_DECL. 
And since the constructor elements have been previously seen, we won't 
hit the aforementioned DFS recursion ICE.

It'd be great if the LTO gods could illuminate this abyss (that's you 
Mr. Biener ;-)), but the attached patch fixes all the LTO problems 
exhibited by:

make check-fortran RUNTESTFLAGS="--target_board=unix'{-flto}' 
dg.exp=*namelist*"

As an aside, we really should test some subset of the LTO tests while 
testing Fortran, or this oversight will surely repeat itself on any 
changes to the Fortran streamer bits.

Does this help?  OK?

Aldy
commit a916bfe9fec9b62971cea769ae58d4b3467e08bd
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Jan 17 08:01:44 2014 -0800

    	* lto-streamer-in.c (lto_input_tree_ref): Use streamer_read_string
    	instead of stream_read_tree.  Read in constructos with
    	stream_read_tree.
    	* lto-streamer-out.c (lto_output_tree_ref): Do not recurse on DFS
    	walk.  Output constructors correctly.

Comments

Richard Biener Jan. 22, 2014, 6:35 a.m. UTC | #1
Aldy Hernandez <aldyh@redhat.com> wrote:
>Hi folks.
>
>The problem here is that while streaming the DECL_NAME with 
>stream_read_tree() and consequently lto_output_tree(), we trigger an
>ICE 
>because we are recursing on the DFS walk:
>
>   else
>     {
>       /* This is the first time we see EXPR, write all reachable
>	 trees to OB.  */
>       static bool in_dfs_walk;
>
>       /* Protect against recursion which means disconnect between
>          what tree edges we walk in the DFS walk and what edges
>	 we stream out.  */
>       gcc_assert (!in_dfs_walk);
>
>In the namelist fortran testcases, this is the first time we see the 
>DECL_NAMEs, so we ICE.  I fixed this by outputting the DECL_NAME's 
>string with streamer_write_string() instead.
>
>[I honestly wondered why we don't stream the entire NAMELIST_DECL the 
>same way we stream IMPORTED_DECL, all in one go, instead of piecemeal 
>like the present code does.  But LTO is this complicated black box in
>my 
>head that I try not to think about too much...the current patch touches
>
>as little as possible.]
>
>This change alone fixes the problems in the PR, but I also found
>another 
>ICE now that streaming actually works: dwarf.  It turns out, that the 
>way the CONSTRUCTOR elements in the NAMELIST_DECL are streamed, a 
>PARM_DECL that has been previously seen (in the function's 
>DECL_ARGUMENTS) will be streamed with different reference magic (or 
>whatever streamed references or ids are called in LTO speak).  So when 
>we read the CONSTRUCTOR elements in the LTO read phase, we end up with 
>different pointers for a PARM_DECL in the constructor for the seemingly
>
>same PARM_DECL in the function's arguments (DECL_ARGUMENTS).
>
>I don't understand LTO very well, but I do see that using 
>stream_read_tree (lto_output_tree) caches the entries, so it seems like
>
>a good fit to avoid writing two distinct items for the same PARM_DECL. 
>And since the constructor elements have been previously seen, we won't 
>hit the aforementioned DFS recursion ICE.
>
>It'd be great if the LTO gods could illuminate this abyss (that's you 
>Mr. Biener ;-)), but the attached patch fixes all the LTO problems 
>exhibited by:
>
>make check-fortran RUNTESTFLAGS="--target_board=unix'{-flto}' 
>dg.exp=*namelist*"
>
>As an aside, we really should test some subset of the LTO tests while 
>testing Fortran, or this oversight will surely repeat itself on any 
>changes to the Fortran streamer bits.
>
>Does this help?  OK?

I'll return from vacation next week and will have a closer look then.

Richard.

>Aldy
Richard Biener Jan. 28, 2014, 9:40 a.m. UTC | #2
On Tue, 21 Jan 2014, Aldy Hernandez wrote:

> Hi folks.
> 
> The problem here is that while streaming the DECL_NAME with stream_read_tree()
> and consequently lto_output_tree(), we trigger an ICE because we are recursing
> on the DFS walk:
> 
>   else
>     {
>       /* This is the first time we see EXPR, write all reachable
> 	 trees to OB.  */
>       static bool in_dfs_walk;
> 
>       /* Protect against recursion which means disconnect between
>          what tree edges we walk in the DFS walk and what edges
> 	 we stream out.  */
>       gcc_assert (!in_dfs_walk);
> 
> In the namelist fortran testcases, this is the first time we see the
> DECL_NAMEs, so we ICE.  I fixed this by outputting the DECL_NAME's string with
> streamer_write_string() instead.
> 
> [I honestly wondered why we don't stream the entire NAMELIST_DECL the same way
> we stream IMPORTED_DECL, all in one go, instead of piecemeal like the present
> code does.  But LTO is this complicated black box in my head that I try not to
> think about too much...the current patch touches as little as possible.]
> 
> This change alone fixes the problems in the PR, but I also found another ICE
> now that streaming actually works: dwarf.  It turns out, that the way the
> CONSTRUCTOR elements in the NAMELIST_DECL are streamed, a PARM_DECL that has
> been previously seen (in the function's DECL_ARGUMENTS) will be streamed with
> different reference magic (or whatever streamed references or ids are called
> in LTO speak).  So when we read the CONSTRUCTOR elements in the LTO read
> phase, we end up with different pointers for a PARM_DECL in the constructor
> for the seemingly same PARM_DECL in the function's arguments (DECL_ARGUMENTS).
> 
> I don't understand LTO very well, but I do see that using stream_read_tree
> (lto_output_tree) caches the entries, so it seems like a good fit to avoid
> writing two distinct items for the same PARM_DECL. And since the constructor
> elements have been previously seen, we won't hit the aforementioned DFS
> recursion ICE.
> 
> It'd be great if the LTO gods could illuminate this abyss (that's you Mr.
> Biener ;-)), but the attached patch fixes all the LTO problems exhibited by:
> 
> make check-fortran RUNTESTFLAGS="--target_board=unix'{-flto}'
> dg.exp=*namelist*"
> 
> As an aside, we really should test some subset of the LTO tests while testing
> Fortran, or this oversight will surely repeat itself on any changes to the
> Fortran streamer bits.
> 
> Does this help?  OK?

The patch doesn't make much sense to me.  I think the problem is that
NAMELIST_DECL is output in a ref section (LTO_namelist_decl_ref) but
the output routine writes other refs (via stream_write_tree and
outputting the constructor).  lto_output_tree_ref may not do this
kind of stuff.  Instead the contents of a NAMELIST_DECL need to be
output from the generic tree writing routines.

Where are NAMELIST_DECLs possibly refered from?

Richard.
Jakub Jelinek Jan. 28, 2014, 9:46 a.m. UTC | #3
On Tue, Jan 28, 2014 at 10:40:51AM +0100, Richard Biener wrote:
> The patch doesn't make much sense to me.  I think the problem is that
> NAMELIST_DECL is output in a ref section (LTO_namelist_decl_ref) but
> the output routine writes other refs (via stream_write_tree and
> outputting the constructor).  lto_output_tree_ref may not do this
> kind of stuff.  Instead the contents of a NAMELIST_DECL need to be
> output from the generic tree writing routines.
> 
> Where are NAMELIST_DECLs possibly refered from?

I think usually from BLOCK_VARS of some BLOCK.

	Jakub
Richard Biener Feb. 4, 2014, 9:44 a.m. UTC | #4
On Tue, 28 Jan 2014, Jakub Jelinek wrote:

> On Tue, Jan 28, 2014 at 10:40:51AM +0100, Richard Biener wrote:
> > The patch doesn't make much sense to me.  I think the problem is that
> > NAMELIST_DECL is output in a ref section (LTO_namelist_decl_ref) but
> > the output routine writes other refs (via stream_write_tree and
> > outputting the constructor).  lto_output_tree_ref may not do this
> > kind of stuff.  Instead the contents of a NAMELIST_DECL need to be
> > output from the generic tree writing routines.
> > 
> > Where are NAMELIST_DECLs possibly refered from?
> 
> I think usually from BLOCK_VARS of some BLOCK.

The following seems to fix things for me - bootstrap / regtest and
SPEC 2k6 LTO -g build in progress.

Now somebody could verify if LTO produces sensible debuginfo for
NAMELIST_DECLs.

Richard.

2014-02-04  Richard Biener  <rguenther@suse.de>

	PR lto/59723
	* lto-streamer-out.c (lto_output_tree_ref): Do not write
	trees from lto_output_tree_ref.

Index: gcc/lto-streamer-out.c
===================================================================
*** gcc/lto-streamer-out.c	(revision 207455)
--- gcc/lto-streamer-out.c	(working copy)
*************** lto_output_tree_ref (struct output_block
*** 255,273 ****
        break;
  
      case NAMELIST_DECL:
!       {
! 	unsigned i;
! 	tree value, tmp;
! 
! 	streamer_write_record_start (ob, LTO_namelist_decl_ref);
! 	stream_write_tree (ob, DECL_NAME (expr), true);
! 	tmp = NAMELIST_DECL_ASSOCIATED_DECL (expr);
! 	gcc_assert (tmp != NULL_TREE);
! 	streamer_write_uhwi (ob, CONSTRUCTOR_ELTS (tmp)->length());
! 	FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (tmp), i, value)
! 	  lto_output_var_decl_index (ob->decl_state, ob->main_stream, value);
! 	break;
!       }
  
      case NAMESPACE_DECL:
        streamer_write_record_start (ob, LTO_namespace_decl_ref);
--- 261,269 ----
        break;
  
      case NAMELIST_DECL:
!       streamer_write_record_start (ob, LTO_namelist_decl_ref);
!       lto_output_var_decl_index (ob->decl_state, ob->main_stream, expr);
!       break;
  
      case NAMESPACE_DECL:
        streamer_write_record_start (ob, LTO_namespace_decl_ref);

Patch
diff mbox

diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c
index df32a6b..9eec472 100644
--- a/gcc/lto-streamer-in.c
+++ b/gcc/lto-streamer-in.c
@@ -256,12 +256,12 @@  lto_input_tree_ref (struct lto_input_block *ib, struct data_in *data_in,
 
 	result = make_node (NAMELIST_DECL);
 	TREE_TYPE (result) = void_type_node;
-	DECL_NAME (result) = stream_read_tree (ib, data_in);
+	DECL_NAME (result) = get_identifier (streamer_read_string (data_in,
+								   ib));
 	n = streamer_read_uhwi (ib);
 	for (i = 0; i < n; i++)
 	  {
-	    ix_u = streamer_read_uhwi (ib);
-	    tmp = lto_file_decl_data_get_var_decl (data_in->file_data, ix_u);
+	    tmp = stream_read_tree (ib, data_in);
 	    gcc_assert (tmp != NULL_TREE);
 	    CONSTRUCTOR_APPEND_ELT (nml_decls, NULL_TREE, tmp);
 	  }
diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c
index 5d6aed5..dc48dd4 100644
--- a/gcc/lto-streamer-out.c
+++ b/gcc/lto-streamer-out.c
@@ -260,12 +260,13 @@  lto_output_tree_ref (struct output_block *ob, tree expr)
 	tree value, tmp;
 
 	streamer_write_record_start (ob, LTO_namelist_decl_ref);
-	stream_write_tree (ob, DECL_NAME (expr), true);
+	streamer_write_string (ob, ob->main_stream,
+			       IDENTIFIER_POINTER (DECL_NAME (expr)), true);
 	tmp = NAMELIST_DECL_ASSOCIATED_DECL (expr);
 	gcc_assert (tmp != NULL_TREE);
 	streamer_write_uhwi (ob, CONSTRUCTOR_ELTS (tmp)->length());
 	FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (tmp), i, value)
-	  lto_output_var_decl_index (ob->decl_state, ob->main_stream, value);
+	  stream_write_tree (ob, value, true);
 	break;
       }