llvm dev - Oct 2013 - [LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication

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/20131009/b37505fe/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Debug-Info-remove-from-from-addDIEEntry.patch
Type: application/octet-stream
Size: 8647 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131009/b37505fe/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Debug-Info-support-for-DW_FORM_ref_addr.patch
Type: application/octet-stream
Size: 7495 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131009/b37505fe/attachment-0001.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: 19379 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131009/b37505fe/attachment-0002.obj>

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/62f86caa/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Debug-Info-remove-form-from-addDIEEntry.patch
Type: application/octet-stream
Size: 8647 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131011/62f86caa/attachment.obj>
-------------- 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/20131011/62f86caa/attachment-0001.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/20131011/62f86caa/attachment-0002.obj>

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>

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>

On Tue, Oct 15, 2013 at 10:05 AM, Manman Ren <manman.ren at gmail.com>
wrote:
>
>
>
> 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.
>
So I'm still thinking about the work list work.

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)?

Even in the not-too-distant type unit future, we'd essentially have a stack
of units (this CU lead us to produce this TU which lead us to produce this
other TU, etc) but the creation of one (compile or type) unit wouldn't ever
cause us to insert new DIEs into previous/earlier units) so far as I can
think of...
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131015/f39965bd/attachment.html>