<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">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:0 0 0 .8ex;border-left:1px #ccc solid;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>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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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>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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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>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> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>If we are going to make assumptions, </div></div></blockquote><div><br></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> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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>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>