[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication

[David Blaikie]

The first patch seems fine, though the comment on the modified addDIEEntry
function is a bit confusing:

-/// addDIEEntry - Add a DIE attribute data and value.
+/// addDIEEntry - Add a DIE attribute data and value. The form should be
+/// a reference form: ref1, ref2, ref4, ref8, ref_udata, ref_addr,
+/// or ref_sig8. A form can be chosen inside addDIEEntry.

When the comment says "The form should be" - it sounds like it /could/ be
something else, etc. As though the caller would specify it and must meet
some requirement. But the caller doesn't specify it at all. I'd probably
leave the comment out entirely - as the form isn't specified at the caller,
it must be chosen by the callee.

As a comparison, "addFlag" doesn't take a form, but it doesn't document
that the form may be _flag or _flag_present - that's just an implementation
detail.

So in short, I would suggest you not modify the comment at all (leave it as
it is today) - with that, please commit this patch.

Did you find a test case for the worklist code yet?


On Fri, Oct 11, 2013 at 4:35 PM, Manman Ren <manman.ren at gmail.com> wrote:

>
> Ping :)
>
>
>
> 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.
>>
>> Thanks,
>> Manman
>>
>>
>>
>>
>>> Manman
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Oct 9, 2013 at 10:45 AM, Manman Ren <manman.ren at gmail.com>wrote:
>>>>
>>>>>
>>>>> Ping
>>>>>
>>>>> -Manman
>>>>>
>>>>>
>>>>> On Fri, Oct 4, 2013 at 7:00 PM, Manman Ren <manman.ren at gmail.com>wrote:
>>>>>
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> The first patch adds support for ref_addr.
>>>>>> Most of it is from r176882, but instead of always using an integer
>>>>>> for ref_addr, we use label + offset for relocation on non-darwin platforms.
>>>>>>
>>>>>> The second patch is a modified version of r191792.
>>>>>> The main change is to use a single map instead of 3 maps in
>>>>>> DwarfDebug and instead of calling DwarfDebug::getDIE|insertDIE directly, we
>>>>>> delegate the function calls to DwarfDebug from CompileUnit.
>>>>>>
>>>>>> No testing case is added in the 1st patch, since the compiler does
>>>>>> not use ref_addr yet.
>>>>>>
>>>>>> For the 2nd patch, testing cases are updated to make sure we remove
>>>>>> duplicated DIEs and use ref_addr to refer to the type DIE. A testing case
>>>>>> is also added to make sure we generate a relocation entry for ref_addr on
>>>>>> linux platform.
>>>>>>
>>>>>> Thanks,
>>>>>> Manman
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131011/b7f38312/attachment.html>