<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 17, 2013 at 11:11 AM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div class="im">On Wed, Oct 16, 2013 at 10:32 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Wed, Oct 16, 2013 at 9:10 PM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Wed, Oct 16, 2013 at 1:58 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Wed, Oct 16, 2013 at 1:21 PM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div>There are a few places where we break the assumption:</div>
<div>1> formal_parameter constructed in DwarfDebug when adding attribute type</div>
<div> we call SPCU->addType(Arg, ATy), where Arg does not belong to SPCU.</div>
<div>2> inlined_subroutine constructed in DwarfDebug when adding attribute abstract_origin</div><div> The inlined_subroutine does not belong to the CU we call addDIEEntry on.</div><div> We create the children of inlined_subroutine and call addDIEEntry before we add it to an owner.</div>
<div>3> ...</div><div><br></div><div>When building xalan with "lto -g", I saw 3158 violations of the assumption. Should we try to fix all these violations to make the assumption true?</div></div></blockquote>
<div><br></div></div><div>As you've said, this assumption (that DIEs are always constructed by the CU that will own them) isn't necessary for this patch - it will be necessary for cu-at-a-time emission. Perhaps we should discuss it in a separate thread rather than trying to make a stronger assumption than is needed for this patch.<br>
<br>Of course without your patch the assumption is trivially correct (well, given your point (1) above, perhaps it wasn't - maybe we do do things out of order and from outside the CU) and we might want to say that it's so important that your patch shouldn't violate it - but I'm not sure that's necessary (Eric?). I haven't given it a great deal of thought.</div>
</div></div></div></blockquote></div><div><br><br>Patches are reattached for convenience: remove DIE
duplication with a worklist (name it patch_with), remove DIE duplication without a worklist
but an assertion when we emit a ref4, we make sure the DIE and the
referenced DIE belong to the same CU (name it patch_without).<br></div></div></div></div></blockquote><div><br></div></div><div>OK, we're going round in circles here and I'm not sure there are many other ways I can communicate things.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>Without my patch, the assumption may be true, but it does not matter since we should always use ref4.<br>
</div><div>I have provided some cases that the assertion fails with patch_without.<br></div></div></div></div></blockquote><div><br></div></div><div>Yes, because it's a narrow assertion based on an assumption I've explained is not true (due to adding DIEs to types after-the-fact, due to things like implicit special members, member templates, and nested types).<br>
<br>The assertion you would need to check that a worklist is not required would be a mapping listing the "assumptions" (ie: we found this parentless DIE and assumed it was in the same DIE as this other DIE we're building) and then, whenever a DIE is added to a parent that chains up to a CU we would have to check that mapping to see if the assumption turned out to be true.<br>
<br>I don't mind if you implement this or not.<br><br>What I do mind and will not accept is committing the worklist version of this patch without justification, and by that I mean a demonstrated example where it is needed, not just where it causes an overly-narrow assertion to fail, but where it causes us to produce the wrong DWARF. If you have such an example, let's look at that and figure out how to fix that - it might (probably isn't) not be the right answer to use a worklist, but simply to rework the order of construction/registration/parenting.</div>
</div></div></div></blockquote><div><br></div></div><div>Let's agree on what assumptions you are trying to make:</div><div>I have stated a few times: inside addDIEEntry, if a DIE does not have a parent, it should belong to "this" CU</div>
<div>--> that is the assumption I made in the patch and I thought that you wanted to verify against</div><div>Now you are talking about a different assumption: "we found this parentless DIE and assumed it was in the same DIE as this other DIE we're building", please be specific on where we should update the mapping list </div>
</div></div></div></blockquote><div><br></div><div>I'm suggesting that we'd update the assumption mapping, if we wanted to add assertions for this, at the time we rely on the assumption - ie: when we choose the form to use.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>and what you mean by "this other DIE".</div>
</div></div></div></blockquote><div><br></div><div>"this other DIE we're building" would be "Die" in the addDIEEntry function - the one we're building, the one we're adding the attribute to.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>With my understanding of the assumption, I implemented "patch_without", where </div><div> +void CompileUnit::addDIEEntry(DIE *Die, uint16_t Attribute, DIEEntry *Entry) {</div><div>+ DIE *DieCU = Die->getCompileUnitOrNull();</div>
<div>+ DIE *EntryCU = Entry->getEntry()->getCompileUnitOrNull();</div><div>+ if (!DieCU)</div><div>+ // We assume that Die belongs to this CU, if it is not linked to any CU yet.</div><div>+ DieCU = getCUDie(); </div>
</div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>+ if (!EntryCU)</div><div>+ EntryCU = getCUDie();</div><div>+ Die->addValue(Attribute, EntryCU == DieCU ? dwarf::DW_FORM_ref4 : </div><div>+ dwarf::DW_FORM_ref_addr, Entry);</div>
<div> }</div><div><br></div><div>If we made the correct assumptions inside addDIEEntry, we would make the correct decision between ref4 and ref_addr, so when we emitting a ref4, the two DIEs must belong to the same CU.</div>
<div>If when emitting a ref4, the two DIEs do not belong to the same CU, it means we made the wrong assumptions inside addDIEEntry.</div><div>If when emitting a ref4, the two DIEs do not belong to the same CU, that means we are generating wrong DWARF. Why is it an overly-narrow assertion? <br>
</div></div></div></div></blockquote><div><br></div><div>Perhaps I'm not understanding the programmatic assertion you actually wrote. Did you write a programmatic assertion (if so, what assertion did you write?), or are you saying you examined the DWARF output and observed that it was wrong? </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
</div><div>Since you are the one to ask for testing cases against some assumptions, please be specific on how you want addDIEEntry to look like without a worklist.<br></div></div></div></div></blockquote><div><br>Perhaps we could approach this in a simpler way.<br>
<br>Start with the code you wrote, remove the worklist, choose an assumption (such as the one in your code snippet here /\) then show me a test case that produces incorrect DWARF. Then we'll talk about that concrete situation (not an assertion failure, but actual incorrect DWARF) and see if we should use a different assumption/model - try that, see if we have bogus DWARF, look at all the test cases together and decide the right design as we go.<br>
<br>It'll be much easier to discuss with concrete examples.<br><br>- David</div></div></div></div>