diff mbox

Don't drop DECL_INITIAL if -g for DWARF2+ (PR fortran/55395)

Message ID 20121206190628.GN2315@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Dec. 6, 2012, 7:06 p.m. UTC 
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+.


	Jakub

Comments

Jan Hubicka Dec. 6, 2012, 8:12 p.m. UTC | #1 
> 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
Jakub Jelinek Dec. 6, 2012, 8:33 p.m. UTC | #2 
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
Richard Biener Dec. 7, 2012, 8:38 a.m. UTC | #3 
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.
Jakub Jelinek Dec. 7, 2012, 9:50 a.m. UTC | #4 
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
Richard Biener Dec. 7, 2012, 10:39 a.m. UTC | #5 
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.
Jakub Jelinek Dec. 7, 2012, 10:54 a.m. UTC | #6 
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
Richard Biener Dec. 7, 2012, 11:23 a.m. UTC | #7 
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.
Jakub Jelinek Dec. 7, 2012, 11:33 a.m. UTC | #8 
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
Richard Biener Dec. 7, 2012, 11:36 a.m. UTC | #9 
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.
diff mbox

Patch

--- 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);
 }