Re: [PATCH] Fix late dwarf generated early from optimized out globals

[Richard Biener <rguenther at suse dot de>]

On Wed, 4 Jan 2017, Andreas Tobler wrote:

> On 04.01.17 10:21, Richard Biener wrote:
> > On Wed, 28 Dec 2016, Andreas Tobler wrote:
> > 
> > > On 28.12.16 19:24, Richard Biener wrote:
> > > > On December 27, 2016 11:17:00 PM GMT+01:00, Andreas Tobler
> > > > <andreast-list@fgznet.ch> wrote:
> > > > > On 16.09.16 13:30, Richard Biener wrote:
> > > > > > On Thu, 15 Sep 2016, Richard Biener wrote:
> > > > > > 
> > > > > > > 
> > > > > > > This addresses sth I needed to address with the early LTO debug
> > > > > patches
> > > > > > > (you might now figure I'm piecemail merging stuff from that
> > > > > > > patch).
> > > > > > > 
> > > > > > > When the cgraph code optimizes out a global we call the
> > > > > late_global_decl
> > > > > > > debug hook to eventually add a DW_AT_const_value to its DIE (we
> > > > > don't
> > > > > > > really expect a location as that will be invalid after optimizing
> > > > > out
> > > > > > > and will be pruned).
> > > > > > > 
> > > > > > > With the early LTO debug patches I have introduced a
> > > > > early_dwarf_finished
> > > > > > > flag (mainly for consistency checking) and I figured I can use
> > > > > > > that
> > > > > to
> > > > > > > detect the call to the late hook during the early phase and
> > > > > > > provide
> > > > > > > the following cleaned up variant of avoiding to create locations
> > > > > that
> > > > > > > require later pruning (which doesn't work with emitting the early
> > > > > DIEs).
> > > > > > > 
> > > > > > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> > > > > > > 
> > > > > > > I verified it does the correct thing for a unit like
> > > > > > > 
> > > > > > > static const int i = 2;
> > > > > > > 
> > > > > > > (but ISTR we do have at least one testcase in the testsuite as
> > > > > well).
> > > > > > > 
> > > > > > > Will commit if testing finishes successfully.
> > > > > > 
> > > > > > Ok, so it showed issues when merging that back to early-LTO-debug.
> > > > > > Turns out in LTO we never call early_finish and thus
> > > > > early_dwarf_finished
> > > > > > was never set.  Also dwarf2out_late_global_decl itself is a better
> > > > > > place to constrain generating locations.
> > > > > > 
> > > > > > The following variant is in very late stage of testing.
> > > > > > 
> > > > > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> > > > > > LTO bootstrap on x86_64-unknown-linux-gnu in stage3.  LTO bootstrap
> > > > > > with early-LTO-debug in stage3, bootstraped with early-LTO-debug,
> > > > > > testing in progress.
> > > > > 
> > > > > Any chance to backport this commit (r240228) to 6.x?
> > > > > It fixes a bootstrap comparison issue on aarch64-*-freebsd*.
> > > > > The aarch64-*-freebsd* port is not yet merged to 6.x and 5.4.x due to
> > > > > the bootstrap comparison failure I faced.
> > > > 
> > > > Did you analyze the bootstrap miscompare?  I suspect the patch merely
> > > > papers
> > > > over the problem.
> > > 
> > > gcc/contrib/compare-debug -p prev-gcc/ipa-icf.o gcc/ipa-icf.o
> > > prev-gcc/ipa-icf.o.stripped. gcc/ipa-icf.o.stripped. differ: char 52841,
> > > line
> > > 253
> > > 
> > > 
> > > The objdump -dSx diff on the non stripped object looked always more or
> > > less
> > > the same, a rodata offset which was different.
> > > 
> > > -                       1448: R_AARCH64_ADD_ABS_LO12_NC .rodata+0x1d8
> > > +                       1448: R_AARCH64_ADD_ABS_LO12_NC .rodata+0x410
> > 
> > Hmm, sounds like a constant pool entry was created by -g at a different
> > time (and thus offset) from regular compilation.  So yes, the patch
> > in question should have the affect to "fix" this.
> > 
> > Note that I later changed the fix with
> > 
> > 2016-10-20  Richard Biener  <rguenther@suse.de>
> > 
> >         * cgraphunit.c (analyze_functions): Set node->definition to
> >         false to signal symbol removal to debug_hooks->late_global_decl.
> >         * ipa.c (symbol_table::remove_unreachable_nodes): When not in
> >         WPA signal symbol removal to the debuginfo machinery.
> >         * dwarf2out.c (dwarf2out_late_global_decl): Instead of
> >         using early_finised to guard the we're called for symbol
> >         removal case look at the symtabs definition flag.
> >         (gen_variable_die): Remove redundant check.
> > 
> > (the dwarf2out_late_global_decl and analyze_functions part are
> > relevant).
> > 
> > That should be the fix to backport (avoiding the early_dwarf_finished
> > part).
> 
> Ok, I bootstrapped with the attached snippet (had to tweak a bit to apply
> cleanly). w/o the previous mentioned fix (r240228). Is this what you had in
> mind or do you think some parts of the other fix (r240228) is also needed?

Not sure if you need the dwarf2out.c part you included but you clearly
missed the dwarf2out_late_global_decl part doing

          /* We get called via the symtab code invoking late_global_decl
             for symbols that are optimized out.  Do not add locations
             for those.  */
          varpool_node *node = varpool_node::get (decl);
          if (! node || ! node->definition)
            tree_add_const_value_attribute_for_decl (die, decl);
          else
            add_location_or_const_value_attribute (die, decl, false);

I wouldn't include the ipa.c part unless required (it adds additional
debug for optimized out decls).

Richard.

> Thanks for the help, really appreciated.
> 
> Andreas
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)