Message ID | 20121206190628.GN2315@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
> Hi! > > This patch fixes both a correctness problem (bootstrap failure in > libgfortran for a couple of targets) and debug info quality issue. > The correctness issue is about the fact that clearing DECL_INITIAL > (well, setting it to error_mark_node is the same) in varpool_remove_node > is changing the decl's bss_initializer_p (decl) status, thus can affect > section flags of it for DWARF2 if an unused decl has > e.g. make_decl_rtl_for_debug called from implicit pointer expansion > before varpool_remove_node and then e.g. during dwarf2out_finish after it. > This function intentionally doesn't remember the RTL, that could affect > code generation. > And for the debug info quality issue see the next patch that actually > improves use of DECL_INITIAL for debug info generation. > > Even if we add some param to limit the size of the initializers we want to > emit into debug info, still the varpool_remove_node clearing of the > initializer would need to be done (if we want to try hard to save compile > time memory) in a way to keep the bss_initializer_p the same as before, > so call it before clearing, if it was set, setting it to error_mark_node > is perhaps fine, if it was set, it needs to be set to some other magic > value and everything that uses DECL_INITIAL afterwards need to be taught > about that (including bss_initializer_p not to treat that other magic value > as bss initializer). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2012-12-06 Jakub Jelinek <jakub@redhat.com> > > PR fortran/55395 > * varpool.c (varpool_remove_node): Don't drop DECL_INITIAL > if -g and emitting DWARF2+. OK. How we want to handle this with LTO streaming? There we also put in error_mark_node: if (TREE_CODE (expr) == VAR_DECL && (TREE_STATIC (expr) || DECL_EXTERNAL (expr)) && initial) { lto_symtab_encoder_t encoder; struct varpool_node *vnode; encoder = ob->decl_state->symtab_node_encoder; vnode = varpool_get_node (expr); if (!vnode || !lto_symtab_encoder_encode_initializer_p (encoder, vnode)) initial = error_mark_node; } stream_write_tree (ob, initial, ref_p); Honza
On Thu, Dec 06, 2012 at 09:12:48PM +0100, Jan Hubicka wrote: > > 2012-12-06 Jakub Jelinek <jakub@redhat.com> > > > > PR fortran/55395 > > * varpool.c (varpool_remove_node): Don't drop DECL_INITIAL > > if -g and emitting DWARF2+. > > OK. How we want to handle this with LTO streaming? Don't know. From debug info quality POV right now, LTO has lots of other more important issues first. And from the invalid error POV, that only matters if the initializer changes in between RTL expansion of some function and end of compilation, so could be fine for now. If not C++-style const, the DECL_INITIAL is used only in the CUs where the definition of the symbol is emitted or would be emitted anyway. > There we also put in error_mark_node: > if (TREE_CODE (expr) == VAR_DECL > && (TREE_STATIC (expr) || DECL_EXTERNAL (expr)) > && initial) > { > lto_symtab_encoder_t encoder; > struct varpool_node *vnode; > > encoder = ob->decl_state->symtab_node_encoder; > vnode = varpool_get_node (expr); > if (!vnode > || !lto_symtab_encoder_encode_initializer_p (encoder, > vnode)) > initial = error_mark_node; > } > > stream_write_tree (ob, initial, ref_p); Jakub
On Thu, 6 Dec 2012, Jakub Jelinek wrote: > Hi! > > This patch fixes both a correctness problem (bootstrap failure in > libgfortran for a couple of targets) and debug info quality issue. > The correctness issue is about the fact that clearing DECL_INITIAL > (well, setting it to error_mark_node is the same) in varpool_remove_node > is changing the decl's bss_initializer_p (decl) status, thus can affect > section flags of it for DWARF2 if an unused decl has > e.g. make_decl_rtl_for_debug called from implicit pointer expansion > before varpool_remove_node and then e.g. during dwarf2out_finish after it. > This function intentionally doesn't remember the RTL, that could affect > code generation. > And for the debug info quality issue see the next patch that actually > improves use of DECL_INITIAL for debug info generation. > > Even if we add some param to limit the size of the initializers we want to > emit into debug info, still the varpool_remove_node clearing of the > initializer would need to be done (if we want to try hard to save compile > time memory) in a way to keep the bss_initializer_p the same as before, > so call it before clearing, if it was set, setting it to error_mark_node > is perhaps fine, if it was set, it needs to be set to some other magic > value and everything that uses DECL_INITIAL afterwards need to be taught > about that (including bss_initializer_p not to treat that other magic value > as bss initializer). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2012-12-06 Jakub Jelinek <jakub@redhat.com> > > PR fortran/55395 > * varpool.c (varpool_remove_node): Don't drop DECL_INITIAL > if -g and emitting DWARF2+. > > --- gcc/varpool.c.jj 2012-11-19 14:41:27.000000000 +0100 > +++ gcc/varpool.c 2012-12-04 17:42:41.228752645 +0100 > @@ -65,7 +65,10 @@ varpool_remove_node (struct varpool_node > && !DECL_VIRTUAL_P (node->symbol.decl) > /* dbxout output constant initializers for readonly vars. */ > && (!host_integerp (DECL_INITIAL (node->symbol.decl), 0) > - || !TREE_READONLY (node->symbol.decl))) > + || !TREE_READONLY (node->symbol.decl)) > + /* dwarf2out can use most of the initializers. */ > + && write_symbols != DWARF2_DEBUG > + && write_symbols != VMS_AND_DWARF2_DEBUG) Eh, shouldn't we "abstract" this properly? Like with a bool flag in debug_hooks? Or with a hook? I see now we special-case dbxout and dwarf2out ... (can all dwarf versions express the initializers?) Thanks, Richard.
On Fri, Dec 07, 2012 at 09:38:06AM +0100, Richard Biener wrote: > > 2012-12-06 Jakub Jelinek <jakub@redhat.com> > > > > PR fortran/55395 > > * varpool.c (varpool_remove_node): Don't drop DECL_INITIAL > > if -g and emitting DWARF2+. > > > > --- gcc/varpool.c.jj 2012-11-19 14:41:27.000000000 +0100 > > +++ gcc/varpool.c 2012-12-04 17:42:41.228752645 +0100 > > @@ -65,7 +65,10 @@ varpool_remove_node (struct varpool_node > > && !DECL_VIRTUAL_P (node->symbol.decl) > > /* dbxout output constant initializers for readonly vars. */ > > && (!host_integerp (DECL_INITIAL (node->symbol.decl), 0) > > - || !TREE_READONLY (node->symbol.decl))) > > + || !TREE_READONLY (node->symbol.decl)) > > + /* dwarf2out can use most of the initializers. */ > > + && write_symbols != DWARF2_DEBUG > > + && write_symbols != VMS_AND_DWARF2_DEBUG) > > Eh, shouldn't we "abstract" this properly? Like with a bool > flag in debug_hooks? Or with a hook? I see now we special-case > dbxout and dwarf2out ... (can all dwarf versions express the > initializers?) Debug hook for this sounds like overkill. If you prefer, as debug info formats other than dwarf2 are just legacy anyway, we could drop the dbxout special case and just do && write_symbols == NO_DEBUG or && debug_info_level == DINFO_LEVEL_NONE. And yes, all dwarf versions can express at least some of the initializers. Jakub
On Fri, 7 Dec 2012, Jakub Jelinek wrote: > On Fri, Dec 07, 2012 at 09:38:06AM +0100, Richard Biener wrote: > > > 2012-12-06 Jakub Jelinek <jakub@redhat.com> > > > > > > PR fortran/55395 > > > * varpool.c (varpool_remove_node): Don't drop DECL_INITIAL > > > if -g and emitting DWARF2+. > > > > > > --- gcc/varpool.c.jj 2012-11-19 14:41:27.000000000 +0100 > > > +++ gcc/varpool.c 2012-12-04 17:42:41.228752645 +0100 > > > @@ -65,7 +65,10 @@ varpool_remove_node (struct varpool_node > > > && !DECL_VIRTUAL_P (node->symbol.decl) > > > /* dbxout output constant initializers for readonly vars. */ > > > && (!host_integerp (DECL_INITIAL (node->symbol.decl), 0) > > > - || !TREE_READONLY (node->symbol.decl))) > > > + || !TREE_READONLY (node->symbol.decl)) > > > + /* dwarf2out can use most of the initializers. */ > > > + && write_symbols != DWARF2_DEBUG > > > + && write_symbols != VMS_AND_DWARF2_DEBUG) > > > > Eh, shouldn't we "abstract" this properly? Like with a bool > > flag in debug_hooks? Or with a hook? I see now we special-case > > dbxout and dwarf2out ... (can all dwarf versions express the > > initializers?) > > Debug hook for this sounds like overkill. If you prefer, as debug info > formats other than dwarf2 are just legacy anyway, we could drop the dbxout > special case and just do && write_symbols == NO_DEBUG or > && debug_info_level == DINFO_LEVEL_NONE. > > And yes, all dwarf versions can express at least some of the initializers. Then I'd say do && debug_info_level <= DINFO_LEVEL_TERSE. Richard.
On Fri, Dec 07, 2012 at 11:39:42AM +0100, Richard Biener wrote: > > > > --- gcc/varpool.c.jj 2012-11-19 14:41:27.000000000 +0100 > > > > +++ gcc/varpool.c 2012-12-04 17:42:41.228752645 +0100 > > > > @@ -65,7 +65,10 @@ varpool_remove_node (struct varpool_node > > > > && !DECL_VIRTUAL_P (node->symbol.decl) > > > > /* dbxout output constant initializers for readonly vars. */ > > > > && (!host_integerp (DECL_INITIAL (node->symbol.decl), 0) > > > > - || !TREE_READONLY (node->symbol.decl))) > > > > + || !TREE_READONLY (node->symbol.decl)) > > > > + /* dwarf2out can use most of the initializers. */ > > > > + && write_symbols != DWARF2_DEBUG > > > > + && write_symbols != VMS_AND_DWARF2_DEBUG) > > > > > > Eh, shouldn't we "abstract" this properly? Like with a bool > > > flag in debug_hooks? Or with a hook? I see now we special-case > > > dbxout and dwarf2out ... (can all dwarf versions express the > > > initializers?) > > > > Debug hook for this sounds like overkill. If you prefer, as debug info > > formats other than dwarf2 are just legacy anyway, we could drop the dbxout > > special case and just do && write_symbols == NO_DEBUG or > > && debug_info_level == DINFO_LEVEL_NONE. > > > > And yes, all dwarf versions can express at least some of the initializers. > > Then I'd say do && debug_info_level <= DINFO_LEVEL_TERSE. That would still mean we can fail bootstrap with -O2 -g1, if there is a DEBUG_IMPLICIT_PTR created during RTL expansion to some unused var, then that var's varpool node is varpool_remove_node removed and initializer cleared, and dwarf2out_finish would want to make_decl_rtl_for_debug again. Jakub
On Fri, 7 Dec 2012, Jakub Jelinek wrote: > On Fri, Dec 07, 2012 at 11:39:42AM +0100, Richard Biener wrote: > > > > > --- gcc/varpool.c.jj 2012-11-19 14:41:27.000000000 +0100 > > > > > +++ gcc/varpool.c 2012-12-04 17:42:41.228752645 +0100 > > > > > @@ -65,7 +65,10 @@ varpool_remove_node (struct varpool_node > > > > > && !DECL_VIRTUAL_P (node->symbol.decl) > > > > > /* dbxout output constant initializers for readonly vars. */ > > > > > && (!host_integerp (DECL_INITIAL (node->symbol.decl), 0) > > > > > - || !TREE_READONLY (node->symbol.decl))) > > > > > + || !TREE_READONLY (node->symbol.decl)) > > > > > + /* dwarf2out can use most of the initializers. */ > > > > > + && write_symbols != DWARF2_DEBUG > > > > > + && write_symbols != VMS_AND_DWARF2_DEBUG) > > > > > > > > Eh, shouldn't we "abstract" this properly? Like with a bool > > > > flag in debug_hooks? Or with a hook? I see now we special-case > > > > dbxout and dwarf2out ... (can all dwarf versions express the > > > > initializers?) > > > > > > Debug hook for this sounds like overkill. If you prefer, as debug info > > > formats other than dwarf2 are just legacy anyway, we could drop the dbxout > > > special case and just do && write_symbols == NO_DEBUG or > > > && debug_info_level == DINFO_LEVEL_NONE. > > > > > > And yes, all dwarf versions can express at least some of the initializers. > > > > Then I'd say do && debug_info_level <= DINFO_LEVEL_TERSE. > > That would still mean we can fail bootstrap with -O2 -g1, if there is a > DEBUG_IMPLICIT_PTR created during RTL expansion to some unused var, then > that var's varpool node is varpool_remove_node removed and initializer > cleared, and dwarf2out_finish would want to make_decl_rtl_for_debug again. "fail"? Well, dwarf2out_finish definitely has to care for no initializer being in-place. Does it somehow record that it was present somewhen early and is confused later when it got dropped? If so, why not always decide late if there is an initializer? Richard.
On Fri, Dec 07, 2012 at 12:23:03PM +0100, Richard Biener wrote: > > That would still mean we can fail bootstrap with -O2 -g1, if there is a > > DEBUG_IMPLICIT_PTR created during RTL expansion to some unused var, then > > that var's varpool node is varpool_remove_node removed and initializer > > cleared, and dwarf2out_finish would want to make_decl_rtl_for_debug again. > > "fail"? Well, dwarf2out_finish definitely has to care for no initializer > being in-place. Does it somehow record that it was present somewhen > early and is confused later when it got dropped? If so, why not always > decide late if there is an initializer? How? During expansion to create DEBUG_IMPLICIT_PTR argument, we need some RTL for the vars (and can't make it using make_decl_rtl, as that would affect code generation). So, we use make_decl_rtl_for_debug that doesn't remember the RTL, but still it does all the processing make_decl_rtl normally does. bss_initializer_p is called deeply from that somewhere. As I said in the PR/initial mail, the other option would be to come up with some other special value for this DECL_INITIAL has been forgotten, but it wasn't bss_initializer_p. But that seems to be lots of work for very little gain (rarely used -g1). Jakub
On Fri, 7 Dec 2012, Jakub Jelinek wrote: > On Fri, Dec 07, 2012 at 12:23:03PM +0100, Richard Biener wrote: > > > That would still mean we can fail bootstrap with -O2 -g1, if there is a > > > DEBUG_IMPLICIT_PTR created during RTL expansion to some unused var, then > > > that var's varpool node is varpool_remove_node removed and initializer > > > cleared, and dwarf2out_finish would want to make_decl_rtl_for_debug again. > > > > "fail"? Well, dwarf2out_finish definitely has to care for no initializer > > being in-place. Does it somehow record that it was present somewhen > > early and is confused later when it got dropped? If so, why not always > > decide late if there is an initializer? > > How? During expansion to create DEBUG_IMPLICIT_PTR argument, we need some > RTL for the vars (and can't make it using make_decl_rtl, as that would > affect code generation). So, we use make_decl_rtl_for_debug that doesn't > remember the RTL, but still it does all the processing make_decl_rtl > normally does. bss_initializer_p is called deeply from that somewhere. > As I said in the PR/initial mail, the other option would be to come up with > some other special value for this DECL_INITIAL has been forgotten, but > it wasn't bss_initializer_p. But that seems to be lots of work for very > little gain (rarely used -g1). Hmm. Then make it DINFO_LEVEL_NONE and put in a fixme comment refering to the bugreport. Richard.
--- gcc/varpool.c.jj 2012-11-19 14:41:27.000000000 +0100 +++ gcc/varpool.c 2012-12-04 17:42:41.228752645 +0100 @@ -65,7 +65,10 @@ varpool_remove_node (struct varpool_node && !DECL_VIRTUAL_P (node->symbol.decl) /* dbxout output constant initializers for readonly vars. */ && (!host_integerp (DECL_INITIAL (node->symbol.decl), 0) - || !TREE_READONLY (node->symbol.decl))) + || !TREE_READONLY (node->symbol.decl)) + /* dwarf2out can use most of the initializers. */ + && write_symbols != DWARF2_DEBUG + && write_symbols != VMS_AND_DWARF2_DEBUG) DECL_INITIAL (node->symbol.decl) = error_mark_node; ggc_free (node); }