[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 Jakub Jelinek changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #37 from Jakub Jelinek --- Fixed.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #36 from GCC Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:74ea20e16cf18b42071557b71a42ea31c8192425 commit r15-7526-g74ea20e16cf18b42071557b71a42ea31c8192425 Author: Jakub Jelinek Date: Fri Feb 14 12:01:13 2025 +0100 tree: Fix up the DECL_VALUE_EXPR GC marking [PR118790] The ggc_set_mark call in gt_value_expr_mark_2 is actually wrong, that just marks the VAR_DECL itself, but doesn't mark the subtrees of it (type etc.). So, I think we need to test gcc_marked_p for whether it is marked or not, if not marked walk the DECL_VALUE_EXPR and then gt_ggc_mx mark the VAR_DECL that was determined not marked and needs to be marked now. One option would be to call gt_ggc_mx (t) right after the DECL_VALUE_EXPR walking, but I'm a little bit worried that the subtree marking could mark other VAR_DECLs (e.g. seen from DECL_SIZE or TREE_TYPE and the like) and if they would be DECL_HAS_VALUE_EXPR_P we might not walk their DECL_VALUE_EXPR anymore later. So, the patch defers the gt_ggc_mx calls until we've walked all the DECL_VALUE_EXPRs directly or indirectly connected to already marked VAR_DECLs. 2025-02-14 Jakub Jelinek PR debug/118790 * tree.cc (struct gt_value_expr_mark_data): New type. (gt_value_expr_mark_2): Don't call ggc_set_mark, instead check ggc_marked_p. Treat data as gt_value_expr_mark_data * with pset in it rather than address of the pset itself and push to be marked VAR_DECLs into to_mark vec. (gt_value_expr_mark_1): Change argument from hash_set * to gt_value_expr_mark_data * and find pset in it. (gt_value_expr_mark): Pass to traverse_noresize address of gt_value_expr_mark_data object rather than hash_table and for all entries in the to_mark vector after the traversal call gt_ggc_mx.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #35 from GCC Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:7738c6286fba7ec2112823f53cc2cefac2c8d007 commit r15-7506-g7738c6286fba7ec2112823f53cc2cefac2c8d007 Author: Jakub Jelinek Date: Thu Feb 13 14:14:50 2025 +0100 tree, gengtype: Fix up GC issue with DECL_VALUE_EXPR [PR118790] The following testcase ICEs, because we have multiple levels of DECL_VALUE_EXPR VAR_DECLs: character(kind=1) id_string[1:.id_string] [value-expr: *id_string.55]; character(kind=1)[1:.id_string] * id_string.55 [value-expr: FRAME.107.id_string.55]; integer(kind=8) .id_string [value-expr: FRAME.107..id_string]; id_string is the user variable mentioned in BLOCK_VARS, it has DECL_VALUE_EXPR because it is a VLA, id_string.55 is a temporary created by gimplify_vla_decl as the address that points to the start of the VLA, what is normally used in the IL to access it. But as this artificial var is then used inside of a nested function, tree-nested.cc adds DECL_VALUE_EXPR to it too and moves the actual value into the FRAME.107 object's member. Now, remove_unused_locals removes id_string.55 (and various other VAR_DECLs) from cfun->local_decls, simply because it is not mentioned in the IL at all (neither is id_string itself, but that is kept in BLOCK_VARS as it has DECL_VALUE_EXPR). So, after this point, id_string.55 tree isn't referenced from anywhere but id_string's DECL_VALUE_EXPR. Next GC collection is triggered, and we are unlucky enough that in the value_expr_for_decl hash table (underlying hash map for DECL_VALUE_EXPR) the id_string.55 entry comes before the id_string entry. id_string is ggc_marked_p because it is referenced from BLOCK_VARS, but id_string.55 is not, as we don't mark DECL_VALUE_EXPR anywhere but by gt_cleare_cache on value_expr_for_decl. But gt_cleare_cache does two things, it calls clear_slots on entries where the key is not ggc_marked_p (so the id_string.55 mapping to FRAME.107.id_string.55 is lost and DECL_VALUE_EXPR (id_string.55) becomes NULL) but then later we see id_string entry, which is ggc_marked_p, so mark the whole hash table entry, which sets ggc_set_mark on id_string.55. But at this point its DECL_VALUE_EXPR is lost. Later during dwarf2out.cc we want to emit DW_AT_location for id_string, see it has DECL_VALUE_EXPR, so emit it as indirection of id_string.55 for which we again lookup DECL_VALUE_EXPR as it has DECL_HAS_VALUE_EXPR_P, but as it is NULL, we ICE, instead of finding it is a subobject of FRAME.107 for which we can find its stack location. Now, as can be seen in the PR, I've tried to tweak tree-ssa-live.cc so that it would keep id_string.55 in cfun->local_decls; that prohibits it from the DECL_VALUE_EXPR of it being GC until expansion, but then we shrink and free cfun->local_decls completely and so GC at that point still can throw it away. The following patch adds an extension to the GTY ((cache)) option, before calling the gt_cleare_cache on some hash table by specifying GTY ((cache ("somefn"))) it calls somefn on that hash table as well. And this extra hook can do any additional ggc_set_mark needed so that gt_cleare_cache preserves everything that is actually needed and throws away the rest. In order to make it just 2 pass rather than up to n passes - (if we had say id1 -> something, id2 -> x(id1), id3 -> x(id2), id4 -> x(id3), id5 -> x(id4) in the value_expr_for_decl hash table in that order (where idN are VAR_DECLs with DECL_HAS_VALUE_EXPR_P, id5 is the only one mentioned from outside and idN -> X stands for idN having DECL_VALUE_EXPR X, something for some arbitrary tree and x(idN) for some arbitrary tree which mentions idN variable) and in each pass just marked the to part of entries with ggc_marked_p base.from we'd need to repeat until we don't mark anything) the patch calls walk_tree on DECL_VALUE_EXPR of the marked trees and if it finds yet unmarked tree, it marks it and walks its DECL_VALUE_EXPR as well the same way. 2025-02-13 Jakub Jelinek PR debug/118790 * gengtype.cc (write_roots): Remove cache variable, instead break from the loop on match and test o for NULL. If the cache option has non-empty string argument, call the specified function with v->name as argument before calling gt_cleare_cache on it. * tree.cc (gt_value_expr_mark_2, gt_value_expr_mark_1, gt_value_expr_mark): New functions. (value_expr_for_decl): Use GTY ((cache ("gt_value_expr_mark"))) rather than just GTY ((cache)). * doc/gty.texi (cache): Document optional argument of cache option. * gfortran.dg/gomp/pr118790.f90: New test.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 Jakub Jelinek changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |jakub at gcc dot gnu.org --- Comment #34 from Jakub Jelinek --- Created attachment 60478 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=60478&action=edit gcc15-pr118790.patch Full untested patch.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #33 from Jakub Jelinek --- So, for the GTY fix I was thinking about something like: --- gcc/tree.cc.jj 2025-01-20 10:26:42.216048422 +0100 +++ gcc/tree.cc 2025-02-12 13:31:07.912605405 +0100 @@ -217,7 +217,7 @@ static GTY ((cache)) hash_table *debug_expr_for_decl; -static GTY ((cache)) +static GTY ((cache ("2pass"))) hash_table *value_expr_for_decl; static GTY ((cache)) --- gcc/hash-map.h.jj 2025-01-02 11:23:37.818219480 +0100 +++ gcc/hash-map.h 2025-02-12 13:33:08.003941869 +0100 @@ -308,6 +308,7 @@ private: template friend void gt_pch_nx (hash_map *); template friend void gt_pch_nx (hash_map *, gt_pointer_operator, void *); template friend void gt_cleare_cache (hash_map *); + template friend void gt_cleare_cache_2pass (hash_map *); hash_table m_table; }; @@ -337,6 +338,14 @@ gt_cleare_cache (hash_map *h) } template +inline void +gt_cleare_cache_2pass (hash_map *h) +{ + if (h) +gt_cleare_cache_2pass (&h->m_table); +} + +template inline void gt_pch_nx (hash_map *h, gt_pointer_operator op, void *cookie) { --- gcc/gengtype.cc.jj 2025-01-02 11:23:02.613710956 +0100 +++ gcc/gengtype.cc 2025-02-12 13:25:02.306650140 +0100 @@ -4656,13 +4656,12 @@ write_roots (pair_p variables, bool emit outf_p f = get_output_file_with_visibility (CONST_CAST (input_file*, v->line.file)); struct flist *fli; - bool cache = false; options_p o; for (o = v->opt; o; o = o->next) if (strcmp (o->name, "cache") == 0) - cache = true; - if (!cache) + break; + if (!o) continue; for (fli = flp; fli; fli = fli->next) @@ -4677,7 +4676,10 @@ write_roots (pair_p variables, bool emit oprintf (f, " ()\n{\n"); } - oprintf (f, " gt_cleare_cache (%s);\n", v->name); + if (o->kind == OPTION_STRING && strcmp (o->info.string, "2pass") == 0) + oprintf (f, " gt_cleare_cache_2pass (%s);\n", v->name); + else + oprintf (f, " gt_cleare_cache (%s);\n", v->name); } finish_cache_funcs (flp); --- gcc/hash-table.h.jj 2025-01-02 11:23:18.432490116 +0100 +++ gcc/hash-table.h2025-02-12 14:05:06.137219383 +0100 @@ -524,6 +524,7 @@ private: gt_pointer_operator, void *); template friend void gt_cleare_cache (hash_table *); + template friend void gt_cleare_cache_2pass (hash_table *); void empty_slow (); @@ -1318,4 +1319,26 @@ gt_cleare_cache (hash_table *h) } } +/* Variant of the above for cache ("2pass"). This version first marks + all the cache entries which should be kept and only afterwards deletes + those that shouldn't, which allows some keys in the cache to be marked + only when referenced in values of other entries in the cache. */ + +template +inline void +gt_cleare_cache_2pass (hash_table *h) +{ + typedef hash_table table; + if (!h) +return; + + for (typename table::iterator iter = h->begin (); iter != h->end (); ++iter) +if (!table::is_empty (*iter) && !table::is_deleted (*iter)) + { + int res = H::keep_cache_entry (*iter); + if (res != 0 && res != -1) + gt_ggc_mx ((*iter)->to); + } + gt_cleare_cache (h); +} #endif /* TYPED_HASHTAB_H */ This fixes the ICE, but now that I think about that, 2 passes aren't enough the way it is written, one pass will handle just one extra indirection through DECL_VALUE_EXPR (like this testcase, id_string ggc_marked_p, which has *id_string.55 has DECL_VALUE_EXPR and id_string.55 not initially marked, and id_string.55 having DECL_VALUE_EXPR to some tree referencing just VAR_DECLs already marked. But if there is an extra indirection and one is unlucky, ggc_marked_p x could have DECL_VALUE_EXPR *y ad y could have DECL_VALUE_EXPR *z and z could have DECL_VALUE_EXPR *w, then if tree_decl_map entry for z comes first, then for y and then for x, this would mark also y but not already z. So, probably instead of gt_ggc_mx ((*iter)->to); it should call some function (perhaps specified in cache's operand) on *iter and that function should actually walk_tree &e.to with callback that would for any VAR_DECLs !ggc_marked_p with DECL_HAS_VALUE_EXPR_P first walk their DECL_VALUE_EXPR and then gt_ggc_mx mark them.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #32 from rguenther at suse dot de --- On Wed, 12 Feb 2025, jakub at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 > > --- Comment #31 from Jakub Jelinek --- > (In reply to rguent...@suse.de from comment #30) > > > Note, normally these VLA addresses don't have DECL_VALUE_EXPR and so are > > > usually used in the IL. It is just that tree-nested.cc in this case added > > > DECL_VALUE_EXPR on those because they are (or were) referenced from nested > > > function. > > > > I think the intent for this is to reflect this into debug info. So, > > ... I think tree-nested.cc should put amend the related VAR which should > > be already somewhere in BLOCK_VARS? > > Note, moving it to BLOCK_VARS is actually wrong, the VAR_DECL is > DECL_ARTIFICIAL, but !DECL_IGNORED_P - we actually want to be able to use its > value. > We don't want users to see id_string.55 in the debugger and be able to ask its > value etc. We just want users to look at id_string which is a VLA. > So, from dwarf2out.cc POV, we want to know where id_string lives and go there > through > the the DECL_VALUE_EXPRs > character(kind=1) id_string[1:.id_string] [value-expr: *id_string.55]; > character(kind=1)[1:.id_string] * id_string.55 [value-expr: > FRAME.107.id_string.55]; > integer(kind=8) .id_string [value-expr: FRAME.107..id_string]; > to actual RTL and DWARF expressions for that. > So, conceptually I think we want to keep current behavior but ensure that > chains of DECL_VALUE_EXPRs still work after GC. > One way to achieve that could be some special GTY extension which does the 2 > pass swipes over the hash_map rather than one pass (could say GTY((user)) deal > with that?). > And another possibility would be to "inline" the DECL_VALUE_EXPR subtrees, > when > we note that id_string DECL_VALUE_EXPR refers to id_string.55 which is not in > any BLOCK_VARS and has DECL_VALUE_EXPR, simply take its DECL_VALUE_EXPR and > replace id_string.55 with that in DECL_VALUE_EXPR of id_string, etc. Hmm, so CFG expand pruning all local_decls interferes with GC then. Can we avoid pruning it completely? Two-stage handling of cache hash tables would work and is probably a safe fix, but then it also feels somewhat expensive (IMO with checking we should assert we're _not_ getting any unmarked entries marked with this approach). Inlining DECL_VALUE_EXPR is an interesting idea, but it feels like unchartered territory at this point.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #31 from Jakub Jelinek --- (In reply to rguent...@suse.de from comment #30) > > Note, normally these VLA addresses don't have DECL_VALUE_EXPR and so are > > usually used in the IL. It is just that tree-nested.cc in this case added > > DECL_VALUE_EXPR on those because they are (or were) referenced from nested > > function. > > I think the intent for this is to reflect this into debug info. So, > ... I think tree-nested.cc should put amend the related VAR which should > be already somewhere in BLOCK_VARS? Note, moving it to BLOCK_VARS is actually wrong, the VAR_DECL is DECL_ARTIFICIAL, but !DECL_IGNORED_P - we actually want to be able to use its value. We don't want users to see id_string.55 in the debugger and be able to ask its value etc. We just want users to look at id_string which is a VLA. So, from dwarf2out.cc POV, we want to know where id_string lives and go there through the the DECL_VALUE_EXPRs character(kind=1) id_string[1:.id_string] [value-expr: *id_string.55]; character(kind=1)[1:.id_string] * id_string.55 [value-expr: FRAME.107.id_string.55]; integer(kind=8) .id_string [value-expr: FRAME.107..id_string]; to actual RTL and DWARF expressions for that. So, conceptually I think we want to keep current behavior but ensure that chains of DECL_VALUE_EXPRs still work after GC. One way to achieve that could be some special GTY extension which does the 2 pass swipes over the hash_map rather than one pass (could say GTY((user)) deal with that?). And another possibility would be to "inline" the DECL_VALUE_EXPR subtrees, when we note that id_string DECL_VALUE_EXPR refers to id_string.55 which is not in any BLOCK_VARS and has DECL_VALUE_EXPR, simply take its DECL_VALUE_EXPR and replace id_string.55 with that in DECL_VALUE_EXPR of id_string, etc.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #29 from Jakub Jelinek --- Perhaps with a help of yet another bitmap, mark_scope_block_unused could bitmap_set_bit DECL_UID of all the BLOCK_VARS VAR_DECLs with DECL_HAS_VALUE_EXPR_P seen in the scopes, and then remove_unused_scope_block_p could when keeping such VAR_DECLs walk_tree their DECL_VALUE_EXPR and if it sees in there a VAR_DECL with DECL_HAS_VALUE_EXPR_P which is not marked in that bitmap, mark it there and queue for addition into the outermost BLOCK_VARS.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #30 from rguenther at suse dot de --- On Wed, 12 Feb 2025, jakub at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 > > --- Comment #28 from Jakub Jelinek --- > --- gcc/tree-ssa-live.cc.jj 2025-02-12 11:03:53.607556187 +0100 > +++ gcc/tree-ssa-live.cc2025-02-12 11:54:34.779131237 +0100 > @@ -369,9 +369,17 @@ mark_all_vars_used_1 (tree *tp, int *wal > { >/* When a global var becomes used for the first time also walk its > initializer (non global ones don't have any). */ > - if (set_is_used (t) && is_global_var (t) > - && DECL_CONTEXT (t) == current_function_decl) > - mark_all_vars_used (&DECL_INITIAL (t)); > + if (set_is_used (t)) > + { > + if (is_global_var (t) > + && DECL_CONTEXT (t) == current_function_decl) > + mark_all_vars_used (&DECL_INITIAL (t)); > + if (DECL_HAS_VALUE_EXPR_P (t)) > + { > + tree dve = DECL_VALUE_EXPR (t); > + mark_all_vars_used (&dve); > + } > + } > } >/* remove_unused_scope_block_p requires information about labels > which are not DECL_IGNORED_P to tell if they might be used in the IL. > */ > @@ -389,7 +397,9 @@ mark_all_vars_used_1 (tree *tp, int *wal > } > > /* Mark the scope block SCOPE and its subblocks unused when they can be > - possibly eliminated if dead. */ > + possibly eliminated if dead. Also, as remove_unused_scope_block_p is > going > + to keep all BLOCK_VARS VAR_DECLs with DECL_HAS_VALUE_EXPR_P, mark all vars > + their DECL_VALUE_EXPR refers to. */ > > static void > mark_scope_block_unused (tree scope) > @@ -398,6 +408,12 @@ mark_scope_block_unused (tree scope) >TREE_USED (scope) = false; >if (!(*debug_hooks->ignore_block) (scope)) > TREE_USED (scope) = true; > + for (t = BLOCK_VARS (scope); t; t = DECL_CHAIN (t)) > +if (VAR_P (t) && DECL_HAS_VALUE_EXPR_P (t)) > + { > + tree dve = DECL_VALUE_EXPR (t); > + mark_all_vars_used (&dve); > + } >for (t = BLOCK_SUBBLOCKS (scope); t ; t = BLOCK_CHAIN (t)) > mark_scope_block_unused (t); > } > @@ -792,9 +808,10 @@ remove_unused_locals (void) > >timevar_push (TV_REMOVE_UNUSED); > > + usedvars = BITMAP_ALLOC (NULL); > + >mark_scope_block_unused (DECL_INITIAL (current_function_decl)); > > - usedvars = BITMAP_ALLOC (NULL); >auto_bitmap useddebug; > >/* Walk the CFG marking all referenced symbols. */ > @@ -819,7 +836,7 @@ remove_unused_locals (void) > { > if (gimple_debug_bind_p (stmt)) > { > - tree var = gimple_debug_bind_get_var (stmt); > + tree var = gimple_debug_bind_get_var (stmt); > if (VAR_P (var)) > { > if (!gimple_debug_bind_get_value (stmt)) > preserves it in local_decls where it was earlier removed. > This keeps DECL_VALUE_EXPR (id_string.55) working until the expand pass. > That throws the VAR_DECL from local_decls again in expand_used_vars, e.g. > expand_one_var doesn't do anything for DECL_HAS_VALUE_EXPR_P VAR_DECLs. > And then instantiate_decls even vec_frees the whole cfun->local_decls (though > by that time DECL_VALUE_EXPR (id_string.55) has been garbage collected > already. > So perhaps instead we want to move such vars from local_decls to BLOCK_VARS of > the outermost block or something? But how to find out if it wasn't there > already? I don't think moving is feasible, but ... > Note, normally these VLA addresses don't have DECL_VALUE_EXPR and so are > usually used in the IL. It is just that tree-nested.cc in this case added > DECL_VALUE_EXPR on those because they are (or were) referenced from nested > function. I think the intent for this is to reflect this into debug info. So, ... I think tree-nested.cc should put amend the related VAR which should be already somewhere in BLOCK_VARS?
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #28 from Jakub Jelinek --- --- gcc/tree-ssa-live.cc.jj 2025-02-12 11:03:53.607556187 +0100 +++ gcc/tree-ssa-live.cc2025-02-12 11:54:34.779131237 +0100 @@ -369,9 +369,17 @@ mark_all_vars_used_1 (tree *tp, int *wal { /* When a global var becomes used for the first time also walk its initializer (non global ones don't have any). */ - if (set_is_used (t) && is_global_var (t) - && DECL_CONTEXT (t) == current_function_decl) - mark_all_vars_used (&DECL_INITIAL (t)); + if (set_is_used (t)) + { + if (is_global_var (t) + && DECL_CONTEXT (t) == current_function_decl) + mark_all_vars_used (&DECL_INITIAL (t)); + if (DECL_HAS_VALUE_EXPR_P (t)) + { + tree dve = DECL_VALUE_EXPR (t); + mark_all_vars_used (&dve); + } + } } /* remove_unused_scope_block_p requires information about labels which are not DECL_IGNORED_P to tell if they might be used in the IL. */ @@ -389,7 +397,9 @@ mark_all_vars_used_1 (tree *tp, int *wal } /* Mark the scope block SCOPE and its subblocks unused when they can be - possibly eliminated if dead. */ + possibly eliminated if dead. Also, as remove_unused_scope_block_p is going + to keep all BLOCK_VARS VAR_DECLs with DECL_HAS_VALUE_EXPR_P, mark all vars + their DECL_VALUE_EXPR refers to. */ static void mark_scope_block_unused (tree scope) @@ -398,6 +408,12 @@ mark_scope_block_unused (tree scope) TREE_USED (scope) = false; if (!(*debug_hooks->ignore_block) (scope)) TREE_USED (scope) = true; + for (t = BLOCK_VARS (scope); t; t = DECL_CHAIN (t)) +if (VAR_P (t) && DECL_HAS_VALUE_EXPR_P (t)) + { + tree dve = DECL_VALUE_EXPR (t); + mark_all_vars_used (&dve); + } for (t = BLOCK_SUBBLOCKS (scope); t ; t = BLOCK_CHAIN (t)) mark_scope_block_unused (t); } @@ -792,9 +808,10 @@ remove_unused_locals (void) timevar_push (TV_REMOVE_UNUSED); + usedvars = BITMAP_ALLOC (NULL); + mark_scope_block_unused (DECL_INITIAL (current_function_decl)); - usedvars = BITMAP_ALLOC (NULL); auto_bitmap useddebug; /* Walk the CFG marking all referenced symbols. */ @@ -819,7 +836,7 @@ remove_unused_locals (void) { if (gimple_debug_bind_p (stmt)) { - tree var = gimple_debug_bind_get_var (stmt); + tree var = gimple_debug_bind_get_var (stmt); if (VAR_P (var)) { if (!gimple_debug_bind_get_value (stmt)) preserves it in local_decls where it was earlier removed. This keeps DECL_VALUE_EXPR (id_string.55) working until the expand pass. That throws the VAR_DECL from local_decls again in expand_used_vars, e.g. expand_one_var doesn't do anything for DECL_HAS_VALUE_EXPR_P VAR_DECLs. And then instantiate_decls even vec_frees the whole cfun->local_decls (though by that time DECL_VALUE_EXPR (id_string.55) has been garbage collected already. So perhaps instead we want to move such vars from local_decls to BLOCK_VARS of the outermost block or something? But how to find out if it wasn't there already? Note, normally these VLA addresses don't have DECL_VALUE_EXPR and so are usually used in the IL. It is just that tree-nested.cc in this case added DECL_VALUE_EXPR on those because they are (or were) referenced from nested function.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #27 from rguenther at suse dot de --- On Wed, 12 Feb 2025, jakub at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 > > --- Comment #26 from Jakub Jelinek --- > So, seems id_string is mentioned in BLOCK_VARS of the ec_meminfo BLOCK: > BLOCK #0 [written] > SUPERCONTEXT: ec_meminfo > SUBBLOCKS: BLOCK #109 [written] BLOCK #166 [written] BLOCK #167 [written] > BLOCK #168 [written] BLOCK #169 [written] > VARS: rnsort check_error prt_data slash_proc i nproc error clpfx .id_string > id_string line node k n18 smallpage nnuma j bucket hugepage memfree cached > clstr coreids icoreid iorank ubound.30 irecv_status itag lastnode llfirst_time > llnocomm maxth mpi_byte mpi_comm_world mpi_integer4 myproc myth rn i > even before any unused var removals. > While id_string.55 is created in the gimplifier, gimplify_vla_decl on > id_string: > 1981 addr = create_tmp_var (ptr_type, get_name (decl)); > 1982 DECL_IGNORED_P (addr) = 0; > 1983 t = build_fold_indirect_ref (addr); > 1984 TREE_THIS_NOTRAP (t) = 1; > 1985 SET_DECL_VALUE_EXPR (decl, t); > and so it is added to cfun->local_decls. > Now, during remove_unused_locals, neither id_string nor id_string.55 are > actually marked as used, they are not used in the IL anymore. > But id_string is preserved in BLOCK_VARS due to remove_unused_scope_block_p: > /* If a decl has a value expr, we need to instantiate it > regardless of debug info generation, to avoid codegen > differences in memory overlap tests. update_equiv_regs() may > indirectly call validate_equiv_mem() to test whether a > SET_DEST overlaps with others, and if the value expr changes > by virtual register instantiation, we may get end up with > different results. */ > else if (VAR_P (*t) && DECL_HAS_VALUE_EXPR_P (*t)) > unused = false; > (added in r0-93860). > But nothing does that something similar for VAR_DECLs with > DECL_HAS_VALUE_EXPR_P in local_decls. > So, shall we always preserve all the DECL_HAS_VALUE_EXPR_P local_decls? I think handling them consistently is important. So either this or ... > Or shall we mark the DECL_HAS_VALUE_EXPR_P VAR_DECLs and their > DECL_VALUE_EXPRs > as used? ... this. At this point (and for branches) I'd prefer to not change behavior for BLOCK_VARs, so always preserving local_decls with DECL_HAS_VALUE_EXPR_P is preferred (and least likely to cause regressions?). Note RTL expansion has its own idea on what decls are used, so I'm not 100% sure what the effect is of another "unused" var in local_decls but not BLOCK_VARs.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #26 from Jakub Jelinek --- So, seems id_string is mentioned in BLOCK_VARS of the ec_meminfo BLOCK: BLOCK #0 [written] SUPERCONTEXT: ec_meminfo SUBBLOCKS: BLOCK #109 [written] BLOCK #166 [written] BLOCK #167 [written] BLOCK #168 [written] BLOCK #169 [written] VARS: rnsort check_error prt_data slash_proc i nproc error clpfx .id_string id_string line node k n18 smallpage nnuma j bucket hugepage memfree cached clstr coreids icoreid iorank ubound.30 irecv_status itag lastnode llfirst_time llnocomm maxth mpi_byte mpi_comm_world mpi_integer4 myproc myth rn i even before any unused var removals. While id_string.55 is created in the gimplifier, gimplify_vla_decl on id_string: 1981 addr = create_tmp_var (ptr_type, get_name (decl)); 1982 DECL_IGNORED_P (addr) = 0; 1983 t = build_fold_indirect_ref (addr); 1984 TREE_THIS_NOTRAP (t) = 1; 1985 SET_DECL_VALUE_EXPR (decl, t); and so it is added to cfun->local_decls. Now, during remove_unused_locals, neither id_string nor id_string.55 are actually marked as used, they are not used in the IL anymore. But id_string is preserved in BLOCK_VARS due to remove_unused_scope_block_p: /* If a decl has a value expr, we need to instantiate it regardless of debug info generation, to avoid codegen differences in memory overlap tests. update_equiv_regs() may indirectly call validate_equiv_mem() to test whether a SET_DEST overlaps with others, and if the value expr changes by virtual register instantiation, we may get end up with different results. */ else if (VAR_P (*t) && DECL_HAS_VALUE_EXPR_P (*t)) unused = false; (added in r0-93860). But nothing does that something similar for VAR_DECLs with DECL_HAS_VALUE_EXPR_P in local_decls. So, shall we always preserve all the DECL_HAS_VALUE_EXPR_P local_decls? Or shall we mark the DECL_HAS_VALUE_EXPR_P VAR_DECLs and their DECL_VALUE_EXPRs as used? Something else?
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #25 from Richard Biener --- (In reply to Jakub Jelinek from comment #24) > So > --- gcc/tree-ssa-live.cc.jj 2025-01-02 11:23:05.915664859 +0100 > +++ gcc/tree-ssa-live.cc 2025-02-11 14:44:33.940178150 +0100 > @@ -369,9 +369,17 @@ mark_all_vars_used_1 (tree *tp, int *wal > { >/* When a global var becomes used for the first time also walk its > initializer (non global ones don't have any). */ > - if (set_is_used (t) && is_global_var (t) > - && DECL_CONTEXT (t) == current_function_decl) > - mark_all_vars_used (&DECL_INITIAL (t)); > + if (set_is_used (t)) > + { > + if (is_global_var (t) > + && DECL_CONTEXT (t) == current_function_decl) > + mark_all_vars_used (&DECL_INITIAL (t)); > + if (DECL_HAS_VALUE_EXPR_P (t)) > + { > + tree dve = DECL_VALUE_EXPR (t); > + mark_all_vars_used (&dve); > + } > + } > } >/* remove_unused_scope_block_p requires information about labels > which are not DECL_IGNORED_P to tell if they might be used in the IL. > */ > ? Except this doesn't work, that new code isn't encountered on the > testcase, so it must be something else that removes the vars. Yes, sth like the above should work. It might be also inlining not copying over the vars - IIRC it also has some tricks, at least for those in local_decls.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #24 from Jakub Jelinek --- So --- gcc/tree-ssa-live.cc.jj 2025-01-02 11:23:05.915664859 +0100 +++ gcc/tree-ssa-live.cc2025-02-11 14:44:33.940178150 +0100 @@ -369,9 +369,17 @@ mark_all_vars_used_1 (tree *tp, int *wal { /* When a global var becomes used for the first time also walk its initializer (non global ones don't have any). */ - if (set_is_used (t) && is_global_var (t) - && DECL_CONTEXT (t) == current_function_decl) - mark_all_vars_used (&DECL_INITIAL (t)); + if (set_is_used (t)) + { + if (is_global_var (t) + && DECL_CONTEXT (t) == current_function_decl) + mark_all_vars_used (&DECL_INITIAL (t)); + if (DECL_HAS_VALUE_EXPR_P (t)) + { + tree dve = DECL_VALUE_EXPR (t); + mark_all_vars_used (&dve); + } + } } /* remove_unused_scope_block_p requires information about labels which are not DECL_IGNORED_P to tell if they might be used in the IL. */ ? Except this doesn't work, that new code isn't encountered on the testcase, so it must be something else that removes the vars.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #23 from Jakub Jelinek --- (In reply to Richard Biener from comment #21) > So the issue is not that we remove id_string.55 var from the hash but > that somehow marking the id_string.55 using *id_string.55 faults then? > Why? Is that really *X with X DECL_VALUE_EXPR being id_string.55 again > and X isn't marked before we process this hashtable? We first see in the DECL_VALUE_EXPR hash map an entry for id_string.55 -> FRAME_*.id_string.55, id_string.55 is not marked (nothing but DECL_VALUE_EXPR of id_string refers to it), so we delete that entry, then later on in the hash table we find id_string -> *id_string.55 mapping, id_string is marked, so we mark also *id_string.55 and therefore keep that. Except that we've deleted DECL_VALUE_EXPR for it, so now id_string.55 has DECL_HAS_VALUE_EXPR_P but DECL_VALUE_EXPR == NULL.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #22 from Richard Biener --- (In reply to Richard Biener from comment #21) > I think the VAR_DECL should be reachable from elsewhere (BLOCK_VARS ideally, > I'd like to get rid of local_decls). IIRC I wondered at some point > whether remove_unused_locals needs to walk DECL_VALUE_EXPRs but I think > I went down an alternate place for the fix/PR I'm remembering (which now > makes it difficult to track down...). PR116907 exactly.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #21 from Richard Biener --- (In reply to Jakub Jelinek from comment #12) > This looks like a GC bug. > On the id_string.55 DECL_HAS_VALUE_EXPR_P bit is set and DECL_VALUE_EXPR is > set to > COMPONENT_REF during streaming in. > But then > #0 pointer_hash::mark_deleted (e=@0x7fffea128850: 0x1) at > ../../gcc/hash-traits.h:204 > #1 0x0110d844 in hash_table xcallocator>::mark_deleted (v=@0x7fffea128850: 0x1) at > ../../gcc/hash-table.h:552 > #2 0x0110b81c in hash_table xcallocator>::clear_slot (this=0x7fffea12a070, slot=0x7fffea128850) at > ../../gcc/hash-table.h:969 > #3 0x01107f8c in gt_cleare_cache > (h=0x7fffea12a070) at ../../gcc/hash-table.h:1315 > #4 0x01104afe in gt_clear_caches_gt_tree_h () at ./gt-tree.h:497 > #5 0x0044c95a in gt_clear_caches () at ./gtype-lto.h:1926 > #6 0x00771633 in ggc_mark_roots () at ../../gcc/ggc-common.cc:112 > #7 0x00490cb8 in ggc_collect (mode=GGC_COLLECT_HEURISTIC) at > ../../gcc/ggc-page.cc:2231 > #8 0x00b0ce79 in execute_one_pass (pass= "expand"(277)>) at ../../gcc/passes.cc:2751 > marks the it in the value_expr_for_decl as deleted, because is ggc_marked_p > false on it. > But that very same id_string.55 var is then ggc_set_mark marked because > there is > id_string var with *id_string.55 DECL_VALUE_EXPR later in the > value_expr_for_decl hash table. > > gt_cleare_cache does only one pass through a hash_map: > for (typename table::iterator iter = h->begin (); iter != h->end (); > ++iter) > if (!table::is_empty (*iter) && !table::is_deleted (*iter)) > { > int res = H::keep_cache_entry (*iter); > if (res == 0) > h->clear_slot (&*iter); > else if (res != -1) > H::ggc_mx (*iter); > } So the issue is not that we remove id_string.55 var from the hash but that somehow marking the id_string.55 using *id_string.55 faults then? Why? Is that really *X with X DECL_VALUE_EXPR being id_string.55 again and X isn't marked before we process this hashtable? > So, either the VAR_DECL should be reachable from something other than > DECL_VALUE_EXPR expressions (bet it was originally in local decls of > something), or we need to have two passes over hash_maps, in the first pass > just H::ggc_mx (*iter) on entries where res is not 0 nor -1, and in the > second pass clear_slot of res is 0. > Guess that needs to be done only for hash tables where keep_cache_entry > returns 0/1 rather than 0/-1, though no idea how to that out during > instantiation. > And I wonder about references from multiple cache hash maps, say > DECL_DEBUG_EXPR refers to something which has DECL_VALUE_EXPR or vice versa. > Wouldn't we then need to do it always in two passes for all the cache hash > tables, > during proper marking walk all those hash tables and just H::ggc_mx (*iter) > when keep_cache_entry returns 1, and then once everything is marked repeat > the walks and only do the clear_slot? I think the VAR_DECL should be reachable from elsewhere (BLOCK_VARS ideally, I'd like to get rid of local_decls). IIRC I wondered at some point whether remove_unused_locals needs to walk DECL_VALUE_EXPRs but I think I went down an alternate place for the fix/PR I'm remembering (which now makes it difficult to track down...).
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #20 from sandra at gcc dot gnu.org --- Looks like other people are already investigating this? I know nothing about GC and this might not even have anything to do with the commit that caused the regression to appear (like PR118714 that turned out to a stack corruption bug in a previous commit). But I can take a look if necessary to see if I can spot something that doesn't look right.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 Jakub Jelinek changed: What|Removed |Added CC||burnus at gcc dot gnu.org, ||sandra at gcc dot gnu.org --- Comment #19 from Jakub Jelinek --- Note, the testcase doesn't use OpenMP declare variant nor metadirectives, so wonder what exactly changed with that commit.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #18 from Jakub Jelinek --- For the reproduction it is important that the var mentioned in debug info with *anothervar DECL_VALUE_EXPR value comes later in the DECL_VALUE_EXPR hash_map than anothervar with FRAME_whatever.anothervar in DECL_VALUE_EXPR. So that we first delete the hash map entry and then mark value whose entry has been deleted.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #17 from Jakub Jelinek --- No, it was r15-6895-g1294b819e1207c0ae76db29a75256f3fafd5f262
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #16 from Andrew Pinski --- I wonder if r15-4334-g60de5585812f59 exposed this issue.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #15 from Andrew Pinski --- Created attachment 60450 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=60450&action=edit new testcase `-O2 -fallow-argument-mismatch -g -fopenmp --param ggc-min-expand=0 --param ggc-min-heapsize=0 -Wfatal-errors` is enough to reproduce it now. Does not need LTO anymore.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 Andrew Pinski changed: What|Removed |Added Keywords|lto | --- Comment #14 from Andrew Pinski --- I am reducing this further now it seems like a GC issue.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #13 from Jakub Jelinek --- Note, the VAR_DECLs disappear from the local decls during ssa pass, but guess id_string keeps to be referenced from debug info while id_string.55 only from the DECL_VALUE_EXPR. If DECL_VALUE_EXPR is the only table needing two passes (or if there are just a couple), perhaps we might need some new GTY option to request two passes rather than one for those.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org, ||rguenth at gcc dot gnu.org --- Comment #12 from Jakub Jelinek --- This looks like a GC bug. On the id_string.55 DECL_HAS_VALUE_EXPR_P bit is set and DECL_VALUE_EXPR is set to COMPONENT_REF during streaming in. But then #0 pointer_hash::mark_deleted (e=@0x7fffea128850: 0x1) at ../../gcc/hash-traits.h:204 #1 0x0110d844 in hash_table::mark_deleted (v=@0x7fffea128850: 0x1) at ../../gcc/hash-table.h:552 #2 0x0110b81c in hash_table::clear_slot (this=0x7fffea12a070, slot=0x7fffea128850) at ../../gcc/hash-table.h:969 #3 0x01107f8c in gt_cleare_cache (h=0x7fffea12a070) at ../../gcc/hash-table.h:1315 #4 0x01104afe in gt_clear_caches_gt_tree_h () at ./gt-tree.h:497 #5 0x0044c95a in gt_clear_caches () at ./gtype-lto.h:1926 #6 0x00771633 in ggc_mark_roots () at ../../gcc/ggc-common.cc:112 #7 0x00490cb8 in ggc_collect (mode=GGC_COLLECT_HEURISTIC) at ../../gcc/ggc-page.cc:2231 #8 0x00b0ce79 in execute_one_pass (pass=) at ../../gcc/passes.cc:2751 marks the it in the value_expr_for_decl as deleted, because is ggc_marked_p false on it. But that very same id_string.55 var is then ggc_set_mark marked because there is id_string var with *id_string.55 DECL_VALUE_EXPR later in the value_expr_for_decl hash table. gt_cleare_cache does only one pass through a hash_map: for (typename table::iterator iter = h->begin (); iter != h->end (); ++iter) if (!table::is_empty (*iter) && !table::is_deleted (*iter)) { int res = H::keep_cache_entry (*iter); if (res == 0) h->clear_slot (&*iter); else if (res != -1) H::ggc_mx (*iter); } So, either the VAR_DECL should be reachable from something other than DECL_VALUE_EXPR expressions (bet it was originally in local decls of something), or we need to have two passes over hash_maps, in the first pass just H::ggc_mx (*iter) on entries where res is not 0 nor -1, and in the second pass clear_slot of res is 0. Guess that needs to be done only for hash tables where keep_cache_entry returns 0/1 rather than 0/-1, though no idea how to that out during instantiation. And I wonder about references from multiple cache hash maps, say DECL_DEBUG_EXPR refers to something which has DECL_VALUE_EXPR or vice versa. Wouldn't we then need to do it always in two passes for all the cache hash tables, during proper marking walk all those hash tables and just H::ggc_mx (*iter) when keep_cache_entry returns 1, and then once everything is marked repeat the walks and only do the clear_slot?
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #6 from Andrew Pinski --- Created attachment 60419 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=60419&action=edit Semi reduced single file testcase `gfortran -O2 -flto=jobserver -shared -fallow-argument-mismatch -g t.f90 -fopenmp` Still need LTO to get the ICE, have not looked into why though.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #5 from Andrew Pinski --- Reducing ...
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #4 from Andrew Pinski --- (In reply to Sam James from comment #3) > Created attachment 60418 [details] > gcc-118790.tar.xz > > Here's a reproducer. I won't reduce it though. This is just enough: ``` gfortran -O2 -flto=jobserver -shared -fallow-argument-mismatch -g -fopenmp ec_args_mod.F90.fii ec_parkind.F90.fii mpl_mpif.F90.fii ec_meminfo.F90.fii ``` The order is important for the module creation.
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 Sam James changed: What|Removed |Added CC||sjames at gcc dot gnu.org --- Comment #3 from Sam James --- Created attachment 60418 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=60418&action=edit gcc-118790.tar.xz Here's a reproducer. I won't reduce it though. ``` $ gfortran -c ec_args_mod.F90.fii $ gfortran -c ec_parkind.F90.fii $ gfortran -c mpl_mpif.F90.fii $ gfortran -O2 -flto=jobserver -shared -fallow-argument-mismatch -g -fopenmp ec_meminfo.F90.fii ec_args_mod.F90.fii ec_parkind.F90.fii mpl_mpif.F90.fii [...] /tmp/fiat/src/fiat/util/ec_meminfo.F90:485:23: internal compiler error: Segmentation fault [...] ```
[Bug debug/118790] [15 Regression] ICE when building fiat (crash in loc_list_from_tree_1)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118790 --- Comment #2 from Andrew Pinski --- case MEM_REF: if (!integer_zerop (TREE_OPERAND (loc, 1))) { have_address = 1; goto do_plus; } /* Fallthru. */ case INDIRECT_REF: list_ret = loc_list_from_tree_1 (TREE_OPERAND (loc, 0), 0, context); have_address = 1; break; And then the crash is at: switch (TREE_CODE (loc)) Which means there is a null for either MEM_REF/INDIRECT_REF