<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Oct 16, 2013 at 12:38 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 class="im">On Wed, Oct 16, 2013 at 11:54 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: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 11:10 AM, 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 10:54 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: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 Tue, Oct 15, 2013 at 5:59 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"><div class="gmail_quote">
<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><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><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>In beginModule, we construct the CUs, but not all the DIEs that belong to the CU.</div><div>In endFunction, we started to construct the scope DIEs. So in some sense, we are adding things to prior CUs.</div>
<div><br></div><div>Looking at void CompileUnit::addDIEEntry(DIE *Die, uint16_t Attribute, DIE *Entry), we can possibly have 3 CUs, this CU,</div><div>the CU Die belongs to (i.e. the CU the parent chain points to), the CU Entry belongs to. </div>
</div></div></div></blockquote><div><br></div></div></div><div>When does "this CU" differ from both the Die and the Entry?</div></div></div></div></blockquote><div><br></div></div></div><div>The signature of addDIEEntry didn't say that "this CU" should be the same as either Die's CU or Entry's CU.</div>
</div></div></div></blockquote><div><br></div></div><div>No, but the code prior to your patch that's the only really sane thing for it to do, right? Why would one CompileUnit be dealing with some other CU's DIEs?<br>
<br>
This is why we should carefully think about the new semantics we're introducing by doing cross-CU DIE references.</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>If you think it should not happen, we should put a comment in so other developers will not violate it.</div><div>But that is irrelevant if we are going to make a single assumption:</div><div> "in addDIEEntry, if a DIE is without a parent, it belongs to the CU we call addDIEEntry on".<br>
</div></div></div></div></blockquote><div><br></div></div><div>I'm not entirely sure that will be true. In the case of implicit special members, such as the copy ctor, what could happen is that a later CU might want to add a member function to an earlier CU. That member function DIE will be being constructed from within the latter CompileUnit, but its parent will be the former CU. Walking the parent chain will find the correct (older CU) because we build the context for a member function and attach the member function to that context before we build any of its parameters, template arguments, etc.</div>
<div><br></div></div></div></div></blockquote></div><div>Exactly, so it is possible that a later CU can construct a DIE that belongs to another CU.</div><div> </div><div>I was rephrasing the statement in your earlier email and trying to be sure what assumption we are going to make.</div>
<div>
<div> <span style="color:rgb(80,0,80)">The claim Eric and I are making/discussed is that we should assume any DIE without a parent is in the current CU and/or that it possibly should never come up (I haven't thought through the codepaths carefully here - if we add CUs to their parent before creating their members, I think it should never come up).</span></div>
<div><br></div></div><div>I know that the assumption may not be true with the current code base, but we need to either</div><div>1> keep the worklist or</div><div>2> remove the worklist and modify the code base to make the assumption true, right?</div>
</div></div></div></blockquote><div><br></div></div><div>I'm OK assuming it to be true until we show it to not be true and treating those cases as bugs and fixing them on an as-found basis.<br></div></div></div></div>
</blockquote><div><br></div></div><div>Okay, I attached a patch removing the worklist. Please review,</div></div></div></div></blockquote><div><br></div></div><div>Will do.</div><div class="im"><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><div><br></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><br>I'm not OK adding the worklist without knowing and testing the cases it addresses.</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><br></div><div>If we are going to make the assumption true, any suggestion on how to fix the above cases? </div></div></div></div></blockquote><div><br></div></div><div>I don't know if the above cases are problems. Have you written and verified any of the test cases I've described?</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>One possibility is to find the correct CU in DwarfDebug, and call getOrCreate on the correct CU.</div>
<div>In your above example, if a later CU wants to add a member function to an earlier CU, we find the earlier CU in DwarfDebug by going through the context chain of the member function, and then call getOrCreate on the earlier CU.</div>
<div><br></div><div>So we can try to make the statement "a CU only constructs a DIE that belongs to the CU", </div></div></div></div></blockquote><div><br></div></div><div>I don't believe so - not based on your current implementation. We'd need to think more carefully about mismatches between type definitions (implicit special members, member template specializations, nested classes - these three things can create differences between definitions) and resolving the metadata so those mismatches don't exist.<br>
<br>That means when we merge the tables of type metadata string identifier to MDNode from each CU is that we may need to modify the type metadata nodes to merge the child lists from all those CUs.</div></div></div></div>
</blockquote><div><br></div></div><div>We don't do that. </div></div></div></div></blockquote><div><br></div></div><div>Yes - see the next paragraph - /if/ we did that, then the "we only produce DIEs from the CU that contains them" could be true. It won't be for now, with your change, unless we do that or something else.</div>
<div class="im">
<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><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>If we did that, perhaps we wouldn't have to retroactively add any DIEs to previous CUs, in which case your statement might become true.<br></div></div></div></div></blockquote><div><br></div></div><div>Adding DIE to a previous CU happens with the current code base, but we can still try to implement "a CU only constructs a DIE that belongs to the CU", with that, we can release all DIEs constructed by the CU when we are done emitting the CU.</div>
<div>This is actually related to emit-one-CU-at-a-time, not so much on this patch.</div></div></div></div></blockquote><div><br></div></div><div>That's correct - you were asking whether we could rely on the assumption that "a CU only constructs a DIE that belongs to that CU" and I said we can't at the moment because of this situation.<br>
<br>When we move to emit-one-CU-at-a-time, we'll need to figure out how to address this issue.</div><div class="im"><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><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>
<br>For now, it isn't, so far as I can tell. (again, I'm giving you some pointers about how I believe some of these things work, but my intention is that these should give you ideas about what you need to test, ways you might be able to find worklist cases, etc - this testing needs to be done to understand how your patch will interact with these systems)</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>that will help emit-one-CU-at-a-time? </div>
</div></div></div></blockquote><div><br></div><div>If we do one-CU-at-a-time emission we're not going to be able to make debug info as compact as we can with your current change, not without more work, at least.<br><br>
Taking the implicit special member example, CU1 would have the definition of some type 'foo' and its normal members. CU2 has a declaration of 'foo' (or perhaps another definition, depending on whether it's a polymorphic type, etc) and the implicit special member definition (such as the copy ctor). When we do the right type referencing and uniquing at the metadata level what's going to happen (I think) is we'll have one definition of 'foo', but its list of members will be that of the definition (what happens if we have more than one definition and they differ because they have different lists of members? I guess we'll produce two definitions of 'foo', one in each CU that has the definitions because they'll be different metadata nodes).</div>
</div></div></div></blockquote><div><br></div></div><div>We will have two DIEs corresponding to the two definitions.</div></div></div></div></blockquote><div><br></div></div><div>Unless one is a declaration and the other'</div>
</div></div></div></blockquote><div><br>and the other's a definition, in which case we'll resolve in favor of the definition.<br><br>> <span style="color:rgb(80,0,80);font-family:monospace">But when we actually refer to the definition via DIRef, we resolve to one of the two definitions.</span><div style="color:rgb(80,0,80);font-family:monospace">
The member DIEs will be added to the single class that we resolve the DIRef to.<br><br>Sort of. We'll have a weird situation where the type definition's list of members is incomplete - and, especially, one node's context, resolved via DIRef, will refer to a type that doesn't list that node as a child. That might be... interesting.</div>
</div></div></div></div>