On Wed, Jun 5, 2019 at 4:30 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Richard Biener <richard.guent...@gmail.com> writes:
> > On Sat, Jun 1, 2019 at 5:49 PM Richard Sandiford
> > <richard.sandif...@arm.com> wrote:
> >>
> >> Taking the address of a variable stops us doing var-tracking on it,
> >> so that we just use the DECL_RTL instead. This can easily cause wrong
> >> debug info for regions of code that would have had correct debug info
> >> if the variable weren't addressable. E.g.:
> >>
> >> {
> >> int base;
> >> get_start (&base);
> >> x[i1] = base;
> >> base += 1; // No need to store this
> >> x[i2] = base; // ...so the debug info for "base" is wrong here
> >> }
> >>
> >> or (the motivating example):
> >>
> >> {
> >> int base;
> >> get_start (&base);
> >> for (int i = 0; i < n; ++i)
> >> {
> >> x[i] = base;
> >> base += y[i]; // Can apply LSM here, so the debug info for "base"
> >> // in the loop is wrong
> >> }
> >> consume (&base);
> >> }
> >>
> >> This patch series lets us use the DECL_RTL location for some parts of a
> >> variable's lifetime and debug-bind locations for other parts:
> >>
> >> 1) Gimple uses "VAR s=> VAR" to bind VAR to its DECL_RTL. The binding
> >> holds until overridden.
> >>
> >> 2) RTL does the same thing using:
> >>
> >> (var_location VAR (decl_rtl_ref VAR))
> >>
> >> where DECL_RTL_REF is a new rtx code that captures the DECL_RTL
> >> by reference rather than by value.
> >>
> >> We can't just use "(var_location VAR (mem X))" for this, because
> >> that would bind VAR to the value that (mem X) has at that exact point.
> >> VAR would therefore get reset by any possible change to (mem X),
> >> whereas here we want it to track (possibly indirect) updates instead.
> >>
> >> 3) The gimplifier decides which variables should get the new treatment
> >> and emits "VAR s=> VAR" to mark the start of VAR's lifetime.
> >> Clobbers continue to mark the end of VAR's lifetime.
> >>
> >> 4) Stores to VAR implicitly reestablish the link between VAR and its
> >> DECL_RTL. This is simpler (and IMO more robust) than inserting an
> >> explicit "VAR s=> VAR" at every write.
> >>
> >> 5) gsi_remove tries to insert "VAR => X" in place of a deleted "VAR = X",
> >> falling back to a "VAR => NULL" reset if that fails.
> >>
> >> Patch 1 handles the new rtl code, patch 2 adds the gimple framework,
> >> and patch 3 uses it for LSM.
> >
> > So I wonder how it handles
> >
> > void __attribute__((noinline)) foo(int *p) { *p = 42; }
> > int x;
> > int main()
> > {
> > int base = 1;
> > foo (&base);
> > base = 2;
> > *(x ? &x : &base) = 1; // (*)
> > return 0;
> > }
> >
> > here we DSE the base = 2 store leaving a
> >
> > # DEBUG base = 2
> >
> > stmt? But there's an indirect store that also stores
> > to base - what will the debug info say at/after (*)? Will
> > it claim that base is 2? At least I do not see that
> > the connection with bases DECL_RTL is re-established?
>
> Yeah, true.
>
> > There's a clobber of base before return 0 so you eventually
> > have to add some dummy stmt you can print base after
> > the indirect store.
> >
> > That said, doesn't "aliasing" create another source of wrong-debug
> > with your approach that might be even worse?
>
> Not sure about even worse, but maybe different. In the example above
> the patches fix the debug info after "base = 2" but break it after the
> following statement.
>
> But there's no real need for the compiler to store to base in (*) either.
Indeed partial dead code elim code sink the store into both arms and then
remove the store in one of them.
> We could end up with "if (...) x = 1;" instead. So AFAICT there's no
> guarantee that we'll get correct debug info at the return statement even
> as things stand.
>
> For memory variables, I think we're always at the mercy of dead stores
> being optimised away, and the patch isn't trying to fix that.
Hmm, but you _do_ insert the debug stmts when we remove stores...
> Since
> both writes to base are dead in the above, I don't think we can guarantee
> correct debug info without compromising optimisation for the sake of
> debuggability. (FWIW, I have a WIP patch to add an option for that,
> hope to post an RFC soon.)
>
> I can't think of a case in which the patches introduce wrong debug
> info for code that isn't dead.
All the recent slew of wrong-debug bugs were exactly for dead code...
I don't think that the difference of dead store vs dead SSA assignment
is different from the user perception. Now - I think that var-tracking
could fix things up here once it sees aliasing stores. The important
difference to before is that now it suddenly deals with variables that
are aliased while before that could not happen (besides spills/reloads).
Can you quickly see what it would take to take this into account?
For my testcase the debugger should say "optimized out" because
we can't tell whether the store will actuall touch 'base', so refering
to its memory location is wrong as well (since we elided the last store).
Richard.
> Thanks,
> Richard
>
> >
> > Otherwise the patches look reasonable.
> >
> > Richard.
> >
> >> Bootstrapped & regtested on aarch64-linux-gnu and x86_64-linux-gnu.
> >> OK to install?
> >>
> >> Richard
