<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><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 class="h5"><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>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><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 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>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> </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 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>When Die or CU does not have an owner, what assumption should we make</div>
<div>to decide between ref_addr and ref4 inside addDIEEntry?</div></div></div></div></blockquote></div><div><br>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).</div>
</div></div></div></blockquote><div><br></div></div><div>Does "current CU" mean the CU we are calling addDIEEntry on?</div></div></div></div></blockquote><div><br></div><div>Yes, I think that's a reasonable interpretation.</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 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>If we assume that if Die or Entry does not have an owner, it should belong to this CU, it means:<br>
</div><div>1> Die or Entry should be constructed by this CU.</div>
2> DIEs that are constructed by CU A should belong to CU A.</div><div class="gmail_quote">3> About DIEs that are constructed directly in DwarfDebug, they should be added to a CU before calling addDIEEntry.</div><div class="gmail_quote">
<div> Another option is to wrap all DIE constructions in a CU, which sounds reasonable for emit-a-CU-at-a-time.</div></div></div></div></blockquote><div><br></div></div><div>I'm not really sure what you mean by "wrap all DIE constructions in a CU". Even with your change the DIEs are constructed by CUs and just added to the appropriate cache (either CU-local, or cross-CU), they're never constructed outside of a CU, are they?</div>
</div></div></div></blockquote></div><div>I am referring to the "new DIE" in DwarfDebug.cpp, and saying that we should instead call getOrCreate function on a CU.</div></div></div></div></blockquote><div><br></div>
<div>Perhaps I'll need to go back and check your patch, but generally, I tend to think we should be calling into a CU to build anything ratehr than doing it 'raw' in DwarfDebug.cpp.</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 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>How much effort is required to make sure these assumptions are true? We also should have the corresponding assertions to make sure the assumptions are not violated.</div>
</div></div></div></blockquote><div><br></div></div><div>Personally, I'm OK saying "this is the intended design, anything else is a bug" and leaving it at that. If we want to add in the necessary infrastructure to assert this is the case, I'm OK with that too.<br>
</div></div></div></div></blockquote><div><br></div></div><div>Okay, let's go with the assumption that "in addDIEEntry, if a DIE is without a parent, it belongs to the CU we can addDIEEntry on".</div></div>
</div></div></blockquote><div><br></div><div>Above counter-example I believe to be true. You should verify that and other such examples, please.</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 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><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>To implement the assertions, we need to keep a list of all DIEs that are constructed by a CU.</div>
</div></div></div></blockquote><div><br></div></div><div>It shouldn't be hard - we'd add a map of assumptions "we found this DIE without a parent and assumed it was in this CU" then whenever we add a DIE to a parent we would check that map and remove the entry (assert-fail if there was a mismatch between assumption and reality).</div>
</div></div></div></blockquote><div><br></div></div><div>Yes that will verify the single assumption. So in addDIEEntry, if a DIE (either Die or Entry) has no parent, add it to a map, and in DIE::addChild, verify the assumption is true and remove it from the map.</div>
<div class="im">
<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>
<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>With the ConstructedDIEs, we can verify 1> in addDIEEntry, 2> in finalize 3> by wrapping all DIE constructions in a CU.</div>
<div><br></div><div>I am wondering whether we should make the type uniquing patches depend on the invariants/assumptions.</div></div></div></div></blockquote><div><br></div></div><div>If the alternative is introducing defensive and unjustified/untested complexity, yes - we must. We cannot tolerate introducing unjustified complexity into the codebase - it's the only way we can strive to move the code in a maintainable direction (I contend that it's not maintainable at the moment and it isn't because of changes like this - adding more is untenable, removing them is admirable, but hard and something we'll be working on for a long time to come).</div>
</div></div></div></blockquote><div><br></div></div><div>I agree that the backend is not that clean and we should try to clean it up. But that single assumption seems not really generic. Instead I think "DIEs that are constructed by CU A should belong to CU A" is more generic and it may help us in emit-one-CU-at-a-time.</div>
<span class="HOEnZb"><font color="#888888">
<div><br></div><div>Manman</div></font></span><div><div class="h5"><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"><span><font color="#888888">
<div><br></div><div>- David</div></font></span><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"><span><font color="#888888"><div>
<br></div><div>Manman</div></font></span><div><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>
<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>The CU constructing a DIE will add the DIE to the context owner, which may not belong to the CU itself.</div>
</div></div></div></blockquote><div><br></div></div><div>That's the question, isn't it? When is it ever /not/ the current CU under construction?</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>In the case that a DIE is added to an owner in DwarfDebug, I don't think we should try to enforce that DwarfDebug will add the DIE to the CU that constructed the DIE earlier.</div></div></div></div></blockquote>
<div><br></div></div><div>My claim is that this is already an invariant. That the code you are adding is never needed and thus is additional, unjustified/unused complexity that comes at a cost to all future developers/development. This codebase needs /much/ less of this than it already has, let alone adding more of it. Though I wouldn't mind an assertion, I suspect it'll be more code than is really worth it to keep track of this information/state.<br>
<br>I think if we more to CU-at-a-time DWARF emission it'll be more obvious that this invariant is true and cannot be violated.</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><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 we can demonstrate a case where this isn't true, then we should work to address that problem - until we demonstrate that, we should not (though we might want to search for such cases - but without type units I can't imagine how they could occur - we build the DIE tree for one CU at a time, at no point do we have DIEs under construction for multiple CUs).<br>
<br>So if we want to build a reference to a DIE, if the DIE is not in another CU we should use ref4. (then the only other case is that it's either in this CU or it's in no known CU at all - in which case it's under construction and, without evidence to the contrary, will be added to the current CU when it's done).<br>
<br>About the only assertion we could add would involve keeping a side-table of "assumptions" ("we expect this DIE will be added to this CU") and check that those assumptions are fulfilled at some point.</div>
</div></div></div></blockquote></div><div>In my opinion, if we can't verify the assumption with a reasonable amount of effort, then we should not make the assumption.</div></div></div></div></blockquote><div><br></div>
</div><div>This leads to unbounded, unjustified complexity that makes the codebase impossible to maintain. It just cannot be an acceptable method of development.<span><font color="#888888"><br><br>- David</font></span></div>
<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"><span><font color="#888888"><div> </div><div>Manman</div></font></span><div><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><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>For that case, we can't assume ref4 should be used. I don't think we can enforce that all DIEs must be added to a parent before calling addDIEEntry.</div>
<div><br></div><div>For the specific testing case, when constructing children of a scope DIE, we call addDIEEntry on the children, after that, we add the children to the scope DIE.</div><div><div>cat foo.cpp </div><div><br>
</div><div>#include "a.hpp"</div><div>void f(int a) {</div><div> Base t;</div><div>}</div><div>cat bar.cpp </div><div><br></div><div>#include "a.hpp"</div><div>void f(int);</div><div>void g(int a) {</div>
<div> Base t;</div><div>}</div><div>int main() {</div><div> f(0);</div><div> g(1);</div><div> return 0;</div><div>}</div><div>cat a.hpp </div><div>struct Base {</div><div> int a;</div><div>};</div></div><div><br></div>
<div>Let me know if I should investigate further.<br></div><div><br></div><div>Thanks,</div><div>Manman</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>
<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>Let me know which one you prefer.</div><div><br></div><div>Do you have any comments on the ref_addr patch (the 1st patch of the two)?</div></div></div></div></blockquote><div><br></div></div><div>Nothing much - I wouldn't mind Eric taking a look (& would rather you not commit this until you're committing the second patch, since it's otherwise untested/unjustified code) on the label/offset related stuff since I'm less familiar with that.<br>
<br>There's one or two cases of {} on single-statement blocks you could fix up.<br><br>"DebugInfoOffset" (both the member and the functions to set/get it) might be more meaningfully named "SectionOffset" - but I'm not sure. Eric? Other names (DebugInfoSectionOffset?)?<span><font color="#888888"><br>
<br>- David</font></span></div></div></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>