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

[David Blaikie]

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 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.

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?

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.
    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?
  DIE.h:
    checkCompileUnit could probably be called "getCompileUnitOrNull", the
name "check*" seems to imply that it does some checking, which isn't true.
  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.
  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?
  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?



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/03bf5631/attachment.html>