[PATCH] Avoid empty .debug_loc entries and lists

[David Blaikie]

On Thu, Jun 4, 2015 at 5:23 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 Jun 4, at 16:50, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Thu, Jun 4, 2015 at 4:46 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >
> > > On 2015 Jun 4, at 16:36, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > >
> > >
> > > On Thu, Jun 4, 2015 at 4:28 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >
> > > > On 2015 Jun 4, at 16:09, David Blaikie <dblaikie at gmail.com> wrote:
> > > >
> > > > Judging by the code, does this cause us not to create a variable if
> it has no locations? That's probably not quite right - even if we have no
> locations, it's probably a good idea/correct to describe the variable so
> shadowing, etc, works correctly (if someone tries to print the variable the
> compiler still finds the right variable but just reports that it doesn't
> know the location)
> > >
> > > So the code here already has an early return if there are no ranges:
> > >
> > >     for (const auto &I : DbgValues) {
> > >       InlinedVariable IV = I.first;
> > >       if (Processed.count(IV))
> > >         continue;
> > >
> > >       // Instruction ranges, specifying where IV is accessible.
> > >       const auto &Ranges = I.second;
> > >       if (Ranges.empty())
> > >         continue;
> > >
> > > My patch effectively hits this `continue` more often.  The difference
> > > is cases where we *thought* we knew how to emit ranges, but in fact
> > > didn't know how.
> > >
> > > I suppose I assumed the intended behaviour was "don't emit the
> > > variable if we don't have any ranges for it" since that's what was
> > > going on, but are you saying this `continue` is a bug?  (Or am I
> > > totally missing your point?)
> > >
> > > How do we end up with an empty range list here? That seems strange - I
> assume we're just lazily creating the rang lists in
> calculateDbgValueHistory... (I could sort of understand empty entries in a
> range list - when it turns out the dbg_value intrinsic describes no
> instructions due to things being optimized away, hoisted here or there,
> etc).
> >
> > No idea, I just assume it happens because there's code.
> >
> > Not sure what you mean.
> >
> > What I'm trying to understand is whether or not we should be fixing this
> bug closer to the source (in calculateDbgValueHistory) that's producing
> empty ranges or empty entries.
>
> One thing that Adrian walked me through is that even if we have
> ranges, and we have entries after the call to `buildLocationList()`,
> we can still fail to emit any bytes during `DebugLocEntry::finalize()`
> since we don't know how to emit every DWARF expression.
>

How are we building DWARF expressions we can't emit? Shouldn't we just not
build those? (I guess this is more of a question for Adrian)


> Because of that, I changed my approach from "trying to get `Ranges` to
> match up with `Entries`" to "popping empty/useless entries and ranges
> off the stack", and figured Ranges/Entries inconsistencies were a
> compile-time optimization that weren't as urgent.
>

My perspective is just to understand why they're there in the first place -
I dislike solutions/fixes/patches that are predicated on "I don't know why
this state occurs, but I can handle it here" - they feel unsatisfying & can
end up layering workarounds on workarounds when there was some underlying
issues that could've been addressed & lead to simpler code with stronger
invariants.

But if there's a reason for them (Empty range lists or empty ranges in the
range lists) that's cool - I just don't know it & wanted to understand it
before I'd be comfortable with the patch.


> Maybe we should also be adding the commented out code I had in:
>
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150525/278797.html
>
> It also seems like the `continue` in the current code might be the wrong
> thing (and we should create the variable with no DW_AT_location?).
>

As you mentioned, the loop that catches optimized-out variables is probably
fine for all these cases (because there are cases where all the dbg_values
could be optimized away & we'd find no evidence of the variable at all, I
should imagine - so that logic has to be up to the task of coping with
these cases).


> (All of this stuff is a tangent for me.  I wandered in here to remove
> the empty .debug_loc entries/lists in the output.)
>

Right - just trying to make sure I understand why this is the right place
to fix that issue & not working around some other issue/missing invariant.


> > What's the right thing to do?
> >
> > I don't have all the context here to know that off-hand. Trying to
> discuss it to try to wrap my head around enough of it to offer a useful
> opinion - sorry it takes a while.
>
> Not trying to rush you, sorry if it seems that way.  This isn't really
> blocking me (I have other things to work on (although obviously I'd
> like to resolve this some time)).
>
> > Should we create a variable the same
> > was as the for loop below for function arguments (that doesn't
> > attach ranges)?  Should we not create a variable?
> >
> > If we get the same output between creating it here or in the later loop,
> it doesn't matter in the broad sense.
> >
> > Is it actually
> > *useful* that we create a .debug_loc list with no entries and point
> > to it?
> >
> > No, there's no benefit to pointing to an empty loc list compared to just
> having no DW_AT_location at all.
>
> Thanks for confirming.
>
> I'd be happy to adjust my patch to just address the empty .debug_loc
> list/entries and continue to create the same variables that were being
> created before (just missing whatever magic makes them create a
> `DW_AT_location` -- I'm guessing I just use the constructor logic
> that's used for optimized-out function parameters in the loop that
> follows?).
>

No, I think that's fine as is - as you pointed out, we have that other loop
for emitting optimized-out variables and we have other logic that's
skipping variables here already.


> (It sounds like it was a poor assumption on my part that we were trying
> to avoid emitting variables if they have no ranges.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150604/1ac7ad3b/attachment.html>