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.
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. 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.
Thanks,
Richard
>
> Otherwise the patches look reasonable.
>
> Richard.
>
>> Bootstrapped & regtested on aarch64-linux-gnu and x86_64-linux-gnu.
>> OK to install?
>>
>> Richard
