<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">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:1px solid rgb(204,204,204);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 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:1px solid 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><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><br>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>
<br></div><div>I didn't get a chance to implement another assertion we mentioned earlier (verify that inside addDIEEntry if a DIE does not have a parent,<br>it should belong to "this" CU). If we want to check if the assumption holds without my patch, I can implement the assertion and check on<br>
</div><div>Xalan.<br><br>If the assumption does not hold even without my patch, are we okay with using a worklist?<br>If the assumption does hold without my patch, but does not hold with my patch, are we okay with using a worklist? If not, we will need to fix the 3000+ violations (which may belong to <10 categories).<br>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>As I stated earlier, this assumption, in my opinion, is not really general, and if we only make this assumption to remove a worklist, it may not worth it.</div>
</div></blockquote><div><br></div></div><div>There is still no discussion about "removing the worklist" - the only discussion is about whether we need to /add/ the worklist. It's up to the commiter to justify the addition of code, not for the community to justify its removal. (I'd say this is /almost/ even true of committed code - if something isn't tested, I'm likely to remove it and see what breaks - if someone cared about it, they should've provided tests for it).</div>
</div></div></div></blockquote><div>Agree. I was under the impression we are trying to remove the worklist.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">
<div>How can developers make sure this assumption is not later violated?</div></div></blockquote><div><br></div></div><div>We can add the kind of assertions we've discussed in this thread, keeping a mapping every time we rely on the assumption and checking the mapping whenever we resolve the parent chain. I don't think this is necessary, but I wouldn't block such a change either.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>If we are going to make assumptions, </div></div></blockquote><div>
<br></div></div><div>We are always going to make assumptions - well, we're always going to have invariants in our design where any violation of those invariants is a bug.</div><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>I will suggest making general assumptions such as<br></div><div>1> always use a CU to construct a DIE</div>
<div>2> Before constructing a DIE, figure out the owner CU first by chasing the context chain, then call construction on the owner CU.</div><div>3> Before calling addDIEEntry in DwarfDebug, find the owner CU first, then call addDIEEntry on the owner CU.</div>
</div></blockquote><div><br></div></div><div>I'm not sure how valuable this would be - perhaps highly, perhaps not. It'd require more work for your patch, though.</div></div></div></div></blockquote><div><br>Agree. These 3 will help emit-one-CU-at-a-time. I was thinking if we are going to enforce the single assumption, it may be easier to enforce 3 general assumptions which can make emit-one-CU-at-a-time easier.<br>
<br></div><div>Thanks,<br>Manman <br></div></div><br></div></div>