| Message ID | 52DEF7C9.5020700@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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);
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; }
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.