<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 15, 2013 at 10:51 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><div class="h5">On Tue, Oct 15, 2013 at 10: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: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><div>On Tue, Oct 15, 2013 at 10:05 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"><div><br></div><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div>On Wed, Oct 9, 2013 at 5:22 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 class="gmail_extra"><br><br><div class="gmail_quote"><div><div>On Wed, Oct 9, 2013 at 1:32 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">David,</div><div class="gmail_extra">
<br></div><div class="gmail_extra">Thanks for reviewing!<br><br><div class="gmail_quote"><div>On Wed, Oct 9, 2013 at 11:36 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">Might be easier if these were on Phabricator, but here are some thoughts:<br>
<br>0001:<div> This patch generally, while separated for legibility, is untested & difficult to discuss in isolation.</div></div></blockquote></div><div>I agree, this patch adds the functionality but does not use it, the 2nd patch uses ref_addr.</div>
<div>If you think I should merge the two and commit as a single patch, let me know.</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> I may need to refer to the second patch in reviewing this first one.<br>
DwarfDebug.cpp:<div> computeSizeAndOffsets:<br> I believe this produces the wrong offset for the 3rd CU and onwards. computeSizeAndOffset returns the EndOffset which is absolute, not relative to the Offset passed in, so it should be assigned to SecOffset, not added to it. (eg: if you have CUs at 0, 42, and 57 - the first pass through SecOffset will be zero, then it'll be 0 + 42, then on the 3rd it'll be (0 + 42) + 57 which isn't right, it should just be 57). Please add more test coverage while fixing this issue.<br>
</div></div></div></blockquote><div><br></div></div><div>computeSizeAndOffset takes an offset that is relative to the start of the CU and returns the offset relative to the CU after laying out the DIE.</div><div>The initial offset before laying out the CU DIE is the header size, EndOffset will be the offset relative to the CU after laying out the whole CU DIE.</div>
<div>We can think of EndOffset as the size of the whole CU DIE. SecOffset is the offset from the Debug Info section, so we can update it by adding the CU size.</div><div><br></div><div> // Offset from the beginning of debug info section.</div>
<div> unsigned SecOffset = 0;</div><div> for (SmallVectorImpl<CompileUnit *>::iterator I = CUs.begin(),</div><div> E = CUs.end(); I != E; ++I) {</div><div> (*I)->setDebugInfoOffset(SecOffset);</div>
<div>
unsigned Offset =</div><div> sizeof(int32_t) + // Length of Compilation Unit Info</div><div> sizeof(int16_t) + // DWARF version number</div><div> sizeof(int32_t) + // Offset Into Abbrev. Section</div><div>
sizeof(int8_t); // Pointer Size (in bytes)</div><div><br></div><div> unsigned EndOffset = computeSizeAndOffset((*I)->getCUDie(), Offset);</div><div> SecOffset += EndOffset;</div><div><div> } </div>
</div></div></div></div></blockquote><div><br></div></div></div><div>Added more comments in attached patch.</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><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><div>
<br>Eric/Manman: rough design question: compute the absolute offset of each CU within the debug_info section and describe them all relative to a single symbol at the start of the debug_info section, or should we put a label at the start of each CU?<br>
</div></div></div></blockquote><div><br></div></div><div>Either way should work. But since we already have the section offset for each CU, describing relative to the start of debug_info section saves us a few symbols :)</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><div>
<br>0002:</div><div> ref_addr_relocation.ll:<br> seems a bit vague in terms of how you test for the relocation. I think it'd make more sense to test the assembly, than the reafobj output, that way you can test that the correct bytes have the relocation rather than just that there's "some" .debug_info relocation in the file. For an example, see test/DebugInfo/X86/tls.ll I wrote - it has some "unannotated" bytes because we still don't have a nice way to annotate location bytes that are DWARF expressions, but it's close - I guess those should be CHECK-NEXTs, though. In any case, you should be able to check a few lines of assembly with the # DW_AT/DW_TAG annotation comments.<br>
</div></div></div></blockquote><div><br></div></div><div>I can check for ".quad .Lsection_info+38 #DW_AT_type".</div></div></div></div></blockquote></div><div>Done in attached patch. </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><div>
You'd need to add the tu3.cpp from my example if you want to demonstrate that the relocation is actually working as intended and avoiding the bogus result I showed.<br> type-unique-simple-a.ll<br> While I agree that having common test cases helps reduce the number of separate invocations we have, this test case seems like it might be becoming a little hard to follow what's under test. Just going from the diff I'm not sure what's what. Could you give a brief description of the state of type-unique-simple2 files? What's involved? What's it meant to test? What's it actually testing?<br>
</div></div></div></blockquote></div><div>I can add more comments. The source files are included in the testing case. I am checking that llvm-link only generates a single copy of the struct and the backend generates a single DIE and uses ref_addr.</div>
<div>The changes are to check "the backend generates a single DIE and uses ref_addr".</div></div></div></div></blockquote></div><div>Done in attached patch. </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><div>
DIE.h:<br> checkCompileUnit could probably be called "getCompileUnitOrNull", the name "check*" seems to imply that it does some checking, which isn't true.<br></div></div></div></blockquote></div>
<div>
Will do.</div></div></div></div></blockquote></div><div>Done in attached patch. </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><div> DwarfCompileUnit.cpp:<br>
the "istype || issubprogram" check should probably be pulled out into a separate function, something like "isShareableAcrossCUs" or something like that (please, that's not the best name, let's discuss it further before we settle on a name) so that getDIE and insertDIE are sure to use the same test at all times.<br>
</div></div></div></blockquote></div><div>Yes, the condition is shared between getDIE and insertDIE. I like isSharableAcrossCUs, because that is why the map is in DwarfDebug instead of CompileUnit.</div></div></div></div>
</blockquote></div><div>Done in attached patch. </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><div>
Why does addDIEEntry take a form? When does the caller get to choose the form rather than the callee deciding between ref4 and ref_addr based on context?<br></div></div></div></blockquote><div> </div></div><div>addDIEEntry took a form before my change and I didn't check why it did.</div>
<div>I will check if all callers always use ref4, if it it true, I will submit a cleanup patch first.</div></div></div></div></blockquote></div><div>Done in attached patch. </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><div> I'm still unsure about this worklist thing - do your current tests cover the need for the worklist? ie: if we removed that logic, would tests fail? Can you describe a specific sequence where the worklist is necessary?<br>
</div></div></div></blockquote><div><br></div></div><div>If we are sure that DIEs are always added to an owner before calling addDIEEntry, we don't need the worklist.</div><div>But I saw cases where that was not true, I will get a reduced testing case.</div>
</div></div></div></blockquote><div><br></div></div><div>If we try to assert both DIEs have owner in addDIEEntry, the following testing cases will fail:</div><div><div> LLVM :: DebugInfo/X86/multiple-aranges.ll</div><div>
LLVM :: DebugInfo/X86/ref_addr_relocation.ll</div>
<div> LLVM :: DebugInfo/X86/stmt-list-multiple-compile-units.ll</div><div> LLVM :: DebugInfo/two-cus-from-same-file.ll</div><div> LLVM :: Linker/type-unique-simple-a.ll</div><div> LLVM :: Linker/type-unique-simple2.ll</div>
</div><div><br></div><div>For ref_addr_relocation, it failed in</div><div><div>#5 0x0000000100b169ba in llvm::DwarfDebug::addDIEEntry (this=0x103023600, Die=0x102e14090, Attribute=73, Entry=0x10302a9d0) at /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3071</div>
<div>#6 0x0000000100b040e0 in llvm::CompileUnit::addType (this=0x102e13ec0, Entity=0x102e14090, Ty={<llvm::DIScope> = {<llvm::DIDescriptor> = {DbgNode = 0x102e05f30}, <No data fields>}, <No data fields>}, Attribute=73) at /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:910</div>
<div>#7 0x0000000100b05bff in llvm::CompileUnit::createGlobalVariableDIE (this=0x102e13ec0, N=0x102e068c0) at /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1505</div></div><div><br></div><div>
If we look at DwarfCompileUnit.cpp:</div><div><div> VariableDIE = new DIE(GV.getTag());</div><div> // Add to map.</div><div> insertDIE(N, VariableDIE);</div><div><br></div><div> // Add name and type.</div><div>
addString(VariableDIE, dwarf::DW_AT_name, GV.getDisplayName());</div><div> addType(VariableDIE, GTy);</div></div><div><br></div><div>The VariableDIE is not added to an owner yet when calling addType.</div></div></div>
</div></blockquote><div><br></div></div></div><div>I believe I have addressed all review comments, the patches are re-attached here for convenience.</div></div></div></div></blockquote><div><br></div></div></div><div>So I'm still thinking about the work list work.<br>
<br>If we don't know which CU a DIE is in - isn't it, necessarily, going to be in the current CU (& thus referenced by ref_data4 (using a CU-local offset), not ref_addr)?<br></div></div></div></div></blockquote>
<div><br></div></div></div><div>That may be true. But can we prove that?</div></div></div></div></blockquote><div><br></div><div>We really shouldn't add extra complexity to the codebase just because we don't know how the codebase works - that's what leads to the kind of complexity we see in the debug info handling today.</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> There are two ways of handling this:</div>
<div>1> use a worklist to be conservative</div><div>2> do not use a worklist, but add an assertion when emitting a DIE A, make sure the DIE referenced with ref4 is indeed in the same CU as DIE A.</div></div></div></div>
</blockquote><div><br></div><div>Please just add this assertion. If it fires we'll have a test case and a concrete justification for this complexity such that should anyone remove it later because it looked unnecessary, they'll at least have a failing test to explain why it was there in the first place.</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>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>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?)?<br>
<br>- David</div></div></div></div>