<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 5, 2015, at 9:45 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Jun 5, 2015 at 9:34 AM, Adrian Prantl <span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><div class=""><div class="h5"><blockquote type="cite" class=""><div class="">On Jun 4, 2015, at 5:33 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Jun 4, 2015 at 5:23 PM, Duncan P. N. Exon Smith <span dir="ltr" class=""><<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><div class=""><br class="">
> On 2015 Jun 4, at 16:50, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:<br class="">
><br class="">
><br class="">
><br class="">
> On Thu, Jun 4, 2015 at 4:46 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>> wrote:<br class="">
><br class="">
> > On 2015 Jun 4, at 16:36, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:<br class="">
> ><br class="">
> ><br class="">
> ><br class="">
> > On Thu, Jun 4, 2015 at 4:28 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>> wrote:<br class="">
> ><br class="">
> > > On 2015 Jun 4, at 16:09, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:<br class="">
> > ><br class="">
> > > 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)<br class="">
> ><br class="">
> > So the code here already has an early return if there are no ranges:<br class="">
> ><br class="">
> > for (const auto &I : DbgValues) {<br class="">
> > InlinedVariable IV = I.first;<br class="">
> > if (Processed.count(IV))<br class="">
> > continue;<br class="">
> ><br class="">
> > // Instruction ranges, specifying where IV is accessible.<br class="">
> > const auto &Ranges = I.second;<br class="">
> > if (Ranges.empty())<br class="">
> > continue;<br class="">
> ><br class="">
> > My patch effectively hits this `continue` more often. The difference<br class="">
> > is cases where we *thought* we knew how to emit ranges, but in fact<br class="">
> > didn't know how.<br class="">
> ><br class="">
> > I suppose I assumed the intended behaviour was "don't emit the<br class="">
> > variable if we don't have any ranges for it" since that's what was<br class="">
> > going on, but are you saying this `continue` is a bug? (Or am I<br class="">
> > totally missing your point?)<br class="">
> ><br class="">
> > 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).<br class="">
><br class="">
> No idea, I just assume it happens because there's code.<br class="">
><br class="">
> Not sure what you mean.<br class="">
><br class="">
> 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.<br class="">
<br class="">
</div></div>One thing that Adrian walked me through is that even if we have<br class="">
ranges, and we have entries after the call to `buildLocationList()`,<br class="">
we can still fail to emit any bytes during `DebugLocEntry::finalize()`<br class="">
since we don't know how to emit every DWARF expression.<br class=""></blockquote><div class=""><br class="">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)<br class=""></div></div></div></div></div></blockquote><div class=""><br class=""></div></div></div><div class="">There are two cases that I’m aware of: Constant float expressions and MachineRegisters that cannot be synthesized in DWARF. Constant floats cannot be expressed in DWARF. DwarfExpression is trying very hard to synthesize registers that that don’t have a DWARF register number out of combinations/pieces of sub- and super-registers but may fail. Basically we have to try to build the expression before we know whether it can be built or not.</div></div></div></blockquote><div class=""><br class="">Sure - but at the end, if we decide we couldn't build the expression, wouldn't we just throw out the whole entry? (& if we end up with no entries, we'd not add the range list, etc)<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Because of that, I changed my approach from "trying to get `Ranges` to<br class="">
match up with `Entries`" to "popping empty/useless entries and ranges<br class="">
off the stack", and figured Ranges/Entries inconsistencies were a<br class="">
compile-time optimization that weren't as urgent.<br class=""></blockquote><div class=""><br class="">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.<br class=""><br class="">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.<br class=""></div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">I’m pretty confident that empty ranges only happen because we use a (too) simple heuristic to decide wether to emit a location in-line or as a .debug_loc entry. (That heuristic being whether there is only a single DBG_VALUE per bit_piece of the variable).</div></div></div></blockquote><div class=""><br class="">I'm a bit confused - I figured we'd build up the range list - then, if the length of that list was one we'd use an in-line location otherwise we'd use a debug_loc entry. (& if the length of the list was zero, we wouldn't add it in the first place)<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>That would off course be much saner. In fact Duncan’s patch gets us there.</div><div><br class=""></div><div>It doesn’t handle the single entry special case, but it also shouldn’t: I just realized that it is not necessarily correct to emit a single-entry debug_loc entry inline, because in a case like:</div><div><br class=""></div><div> l1: DBG_VALUE, var, reg0</div><div> ...</div><div> l2: DBG_VALUE, var, noreg</div><div> ...</div><div><br class=""></div><div>we’d still need to emit a single-entry debug_loc entry to describe that the reg0 location is only valid in the range [l1..l2).</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class=""> If building one or more expressions fails then we may end up with one (should have been inline instead) or zero (shouldn’t have a DW_AT_location at all) .debug_loc entries.</div></div></div></blockquote><div class=""><br class="">Can we practically/easily remove those at the source?<br class=""></div></div></div></div></div></blockquote><div><br class=""></div>Yes: DwarfDebug::collectVariableInfo() should invoke RegVar->setDebugLocListIndex() _after_ buildLocationList() and only if the location list is nonempty.</div><div><br class=""></div><div>-- adrian<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><span class="HOEnZb"><font color="#888888" class=""><div class=""><br class=""></div><div class="">-- adrian</div></font></span><div class=""><div class="h5"><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Maybe we should also be adding the commented out code I had in:<br class="">
<a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150525/278797.html" target="_blank" class="">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150525/278797.html</a><br class="">
<br class="">
It also seems like the `continue` in the current code might be the wrong<br class="">
thing (and we should create the variable with no DW_AT_location?).<br class=""></blockquote><div class=""><br class="">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).<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(All of this stuff is a tangent for me. I wandered in here to remove<br class="">
the empty .debug_loc entries/lists in the output.)<br class=""></blockquote><div class=""><br class="">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.<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> What's the right thing to do?<br class="">
><br class="">
> 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.<br class="">
<br class="">
</span>Not trying to rush you, sorry if it seems that way. This isn't really<br class="">
blocking me (I have other things to work on (although obviously I'd<br class="">
like to resolve this some time)).<br class="">
<span class=""><br class="">
> Should we create a variable the same<br class="">
> was as the for loop below for function arguments (that doesn't<br class="">
> attach ranges)? Should we not create a variable?<br class="">
><br class="">
> If we get the same output between creating it here or in the later loop, it doesn't matter in the broad sense.<br class="">
><br class="">
> Is it actually<br class="">
> *useful* that we create a .debug_loc list with no entries and point<br class="">
> to it?<br class="">
><br class="">
> No, there's no benefit to pointing to an empty loc list compared to just having no DW_AT_location at all.<br class="">
<br class="">
</span>Thanks for confirming.<br class="">
<br class="">
I'd be happy to adjust my patch to just address the empty .debug_loc<br class="">
list/entries and continue to create the same variables that were being<br class="">
created before (just missing whatever magic makes them create a<br class="">
`DW_AT_location` -- I'm guessing I just use the constructor logic<br class="">
that's used for optimized-out function parameters in the loop that<br class="">
follows?).<br class=""></blockquote><div class=""><br class="">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.<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(It sounds like it was a poor assumption on my part that we were trying<br class="">
to avoid emitting variables if they have no ranges.)</blockquote></div><br class=""></div></div>
</div></blockquote></div></div></div><br class=""></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>