[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
Manman Ren
manman.ren at gmail.com
Tue Oct 15 10:05:34 PDT 2013
On Wed, Oct 9, 2013 at 5:22 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> On Wed, Oct 9, 2013 at 1:32 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>>
>> David,
>>
>> Thanks for reviewing!
>>
>> On Wed, Oct 9, 2013 at 11:36 AM, David Blaikie <dblaikie at gmail.com>wrote:
>>
>>> Might be easier if these were on Phabricator, but here are some thoughts:
>>>
>>> 0001:
>>> This patch generally, while separated for legibility, is untested &
>>> difficult to discuss in isolation.
>>>
>> I agree, this patch adds the functionality but does not use it, the 2nd
>> patch uses ref_addr.
>> If you think I should merge the two and commit as a single patch, let me
>> know.
>>
>>
>>> I may need to refer to the second patch in reviewing this first one.
>>> DwarfDebug.cpp:
>>> computeSizeAndOffsets:
>>> 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.
>>>
>>
>> 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.
>> 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.
>> 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.
>>
>> // Offset from the beginning of debug info section.
>> unsigned SecOffset = 0;
>> for (SmallVectorImpl<CompileUnit *>::iterator I = CUs.begin(),
>> E = CUs.end(); I != E; ++I) {
>> (*I)->setDebugInfoOffset(SecOffset);
>> unsigned Offset =
>> sizeof(int32_t) + // Length of Compilation Unit Info
>> sizeof(int16_t) + // DWARF version number
>> sizeof(int32_t) + // Offset Into Abbrev. Section
>> sizeof(int8_t); // Pointer Size (in bytes)
>>
>> unsigned EndOffset = computeSizeAndOffset((*I)->getCUDie(), Offset);
>> SecOffset += EndOffset;
>> }
>>
>
> Added more comments in attached patch.
>
>
>>
>>
>>> 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?
>>>
>>
>> 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 :)
>>
>>
>>>
>>> 0002:
>>> ref_addr_relocation.ll:
>>> 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.
>>>
>>
>> I can check for ".quad .Lsection_info+38 #DW_AT_type".
>>
> Done in attached patch.
>
>>
>>
>>> 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.
>>> type-unique-simple-a.ll
>>> 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?
>>>
>> 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.
>> The changes are to check "the backend generates a single DIE and uses
>> ref_addr".
>>
> Done in attached patch.
>
>>
>> DIE.h:
>>> checkCompileUnit could probably be called "getCompileUnitOrNull",
>>> the name "check*" seems to imply that it does some checking, which isn't
>>> true.
>>>
>> Will do.
>>
> Done in attached patch.
>
>>
>>
>>> DwarfCompileUnit.cpp:
>>> 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.
>>>
>> Yes, the condition is shared between getDIE and insertDIE. I like
>> isSharableAcrossCUs, because that is why the map is in DwarfDebug instead
>> of CompileUnit.
>>
> Done in attached patch.
>
>>
>>
>>> 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?
>>>
>>
>> addDIEEntry took a form before my change and I didn't check why it did.
>> I will check if all callers always use ref4, if it it true, I will submit
>> a cleanup patch first.
>>
> Done in attached patch.
>
>>
>> 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?
>>>
>>
>> If we are sure that DIEs are always added to an owner before calling
>> addDIEEntry, we don't need the worklist.
>> But I saw cases where that was not true, I will get a reduced testing
>> case.
>>
>
> If we try to assert both DIEs have owner in addDIEEntry, the following
> testing cases will fail:
> LLVM :: DebugInfo/X86/multiple-aranges.ll
> LLVM :: DebugInfo/X86/ref_addr_relocation.ll
> LLVM :: DebugInfo/X86/stmt-list-multiple-compile-units.ll
> LLVM :: DebugInfo/two-cus-from-same-file.ll
> LLVM :: Linker/type-unique-simple-a.ll
> LLVM :: Linker/type-unique-simple2.ll
>
> For ref_addr_relocation, it failed in
> #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
> #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
> #7 0x0000000100b05bff in llvm::CompileUnit::createGlobalVariableDIE
> (this=0x102e13ec0, N=0x102e068c0) at
> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1505
>
> If we look at DwarfCompileUnit.cpp:
> VariableDIE = new DIE(GV.getTag());
> // Add to map.
> insertDIE(N, VariableDIE);
>
> // Add name and type.
> addString(VariableDIE, dwarf::DW_AT_name, GV.getDisplayName());
> addType(VariableDIE, GTy);
>
> The VariableDIE is not added to an owner yet when calling addType.
>
I believe I have addressed all review comments, the patches are re-attached
here for convenience.
Manman
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131015/9e50594a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Debug-Info-support-for-DW_FORM_ref_addr.patch
Type: application/octet-stream
Size: 8150 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131015/9e50594a/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Debug-Info-remove-duplication-of-DIEs-when-a-DIE-is-.patch
Type: application/octet-stream
Size: 18123 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131015/9e50594a/attachment-0001.obj>
More information about the llvm-dev
mailing list