<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 27, 2015 at 10:39 PM, Amjad Aboud via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aaboud added a comment.<br>
<span class=""><br>
> Right, what I'm suggesting is that part could be done separately from the scope handling stuff.<br>
<br>
<br>
</span>I understand the benefit of splitting code into as much patches as possible.<br>
<span class=""><br>
> This change would ensure that the DIEs go in the right function (the abstract one or the concrete one) - which would fix bugs that occur when there is no concrete DIE, if I understand correctly?<br>
<br>
<br>
</span>Actually, what we agreed on that the DIEs would go to abstract one if it exists, otherwise to the concrete. So if we have abstract and concrete the DIE will be only in the abstract.<br>
And this is what I implemented above.<br>
<span class=""><br>
> For example, currently if you have a static local variable in a function that is inlined we produce an abstract definition, an inlined subroutine, but then we also produce this weird stub-concrete definition (a subprogram tag with /only/ an abstract_origin attribute, and the child DW_TAG_variable for the static variable). If we delay the addition of the local types and static variables and just put them in the abstract definition if there is one, we shouldn't end up producing the weird stub concrete definition (that's not really a concrete definition anyway).<br>
<br>
<br>
</span>But the “currently” you are mentioning is not true anymore, once we changed the design to create the DIE without attaching it to a scope, the weird stub-concrete definition is not created anymore.<br>
So, unless we handle these DIEs at end of module processing and put them in the correct scope (abstract or concrete), we will end up having flying (unattached) DIEs.<br>
<span class=""><br>
> So that would be a good, separable, incremental step. /then/ we can have a separate (3rd) patch or patches to try to put it in the right scopes?<br>
<br>
<br>
</span>We improved the design, there is no reason to go one step back, just to commit two patches that none of them make sense as itself.<br></blockquote><div><br>I think they do make sense by themselves - one of them fixes a smaller existing bug, the next one improves things after we have a good foundation for those improvements. Much like the series of patches I made last year to improve inline debug information - a lot of it was figuring out where I needed to go, then working backwards to figure out the steps that would best get me there.<br><br>This isn't uncommon review feedback either - we often try to break up larger changes to ease review and keep the commit history (& also the revert history) simple  and easy to follow.<br><br>I'd be happy to work with you to help with this - I can work up some/all of these patches & we can review them together if you prefer.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="im HOEnZb"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D11180" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11180</a><br>
<br>
<br>
<br>
</span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>