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

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

0001-Debug-Info-support-for-DW_FORM_ref_addr.patch (7.32 KB)

0002-Debug-Info-remove-duplication-of-DIEs-when-a-DIE-is-.patch (16.2 KB)

Ping

-Manman

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?

David,

Thanks for reviewing!

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;
  }

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 :slight_smile:

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

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

  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.

  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.

  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.

  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.

Manman

David,

Thanks for reviewing!

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 :slight_smile:

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

0001-Debug-Info-remove-from-from-addDIEEntry.patch (8.44 KB)

0002-Debug-Info-support-for-DW_FORM_ref_addr.patch (7.32 KB)

0003-Debug-Info-remove-duplication-of-DIEs-when-a-DIE-is-.patch (18.9 KB)

Ping :slight_smile:

0001-Debug-Info-remove-form-from-addDIEEntry.patch (8.44 KB)

0002-Debug-Info-support-for-DW_FORM_ref_addr.patch (7.96 KB)

0003-Debug-Info-remove-duplication-of-DIEs-when-a-DIE-is-.patch (17.7 KB)

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?

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.

Yes, I will leave the comments as it was, and commit the "remove-form"
patch.

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

It was in my email before the ping :slight_smile:
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

BTW, the worklist code could be used to decide among ref1, ref2, ref4 etc if we want to reduce dwarf size.
There are some issues we need to fix before actually deciding among ref1, ref2, and ref4:
Since the Abbreviation depends on the reference form, we can assign Abbreviation for a DIE after the reference form is decided,
but we can decide on the reference form only after we know the offsets of each DIE (the offset depends on the encoded size of AbbrevNumber, which is encoded with ULEB128).
So it may not work out, just a thought,

Manman

I believe I have addressed all review comments, the patches are re-attached
here for convenience.

Manman

0002-Debug-Info-support-for-DW_FORM_ref_addr.patch (7.96 KB)

0003-Debug-Info-remove-duplication-of-DIEs-when-a-DIE-is-.patch (17.7 KB)

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

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that? There are two ways of handling
this:
1> use a worklist to be conservative
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.

Let me know which one you prefer.

Do you have any comments on the ref_addr patch (the 1st patch of the two)?

Thanks,
Manman

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that?

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.

There are two ways of handling this:
1> use a worklist to be conservative
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.

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.

Let me know which one you prefer.

Do you have any comments on the ref_addr patch (the 1st patch of the two)?

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.

There's one or two cases of {} on single-statement blocks you could fix up.

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

- David

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that?

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.

There are two ways of handling this:
1> use a worklist to be conservative
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.

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

The assertion fails even with a simple testing case when the referenced DIE
has an owner and the DIE itself does not have an owner.
For that case, we can't assume ref4 should be used. I don't think we can
enforce that all DIEs must be added to a parent before calling addDIEEntry.

For the specific testing case, when constructing children of a scope DIE,
we call addDIEEntry on the children, after that, we add the children to the
scope DIE.
cat foo.cpp

#include "a.hpp"
void f(int a) {
  Base t;
}
cat bar.cpp

#include "a.hpp"
void f(int);
void g(int a) {
  Base t;
}
int main() {
  f(0);
  g(1);
  return 0;
}
cat a.hpp
struct Base {
  int a;
};

Let me know if I should investigate further.

Thanks,
Manman

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that?

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.

There are two ways of handling this:
1> use a worklist to be conservative
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.

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.

Let me know which one you prefer.

Do you have any comments on the ref_addr patch (the 1st patch of the two)?

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.

Eric,

Do you want to take a look at the 1st patch?

There's one or two cases of {} on single-statement blocks you could fix up.

I couldn't find that.
Are you referring to:

+ } else {

+ Asm->EmitInt32(Addr);
+ }

It has a multi-statement if, I thought the rule is to have a parenthesis
for the else as well.

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

Eric, any opinion here?

Thanks,
Manman

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that?

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.

There are two ways of handling this:
1> use a worklist to be conservative
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.

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.

Let me know which one you prefer.

Do you have any comments on the ref_addr patch (the 1st patch of the
two)?

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.

Eric,

Do you want to take a look at the 1st patch?

I do, I've been following the thread, but give me a day or so.

Thanks.

-eric

>
>
>
>>
>>
>>
>>
>>>
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> David,
>>>>>>>
>>>>>>> Thanks for reviewing!
>>>>>>>
>>>>>>>>
>>>>>>>> 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 :slight_smile:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>> >}, 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)?
>>>
>>>
>>> That may be true. But can we prove that?
>>
>>
>> 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.
>>
>>>
>>> There are two ways of handling this:
>>> 1> use a worklist to be conservative
>>> 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.
>>
>>
>> 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.
>>
>>>
>>>
>>> Let me know which one you prefer.
>>>
>>> Do you have any comments on the ref_addr patch (the 1st patch of the
>>> two)?
>>
>>
>> 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.
>
>
> Eric,
>
> Do you want to take a look at the 1st patch?
>

I do, I've been following the thread, but give me a day or so.

Sure :slight_smile:

Thanks,
Manman

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that?

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.

There are two ways of handling this:
1> use a worklist to be conservative
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.

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

The assertion fails even with a simple testing case when the referenced
DIE has an owner and the DIE itself does not have an owner.

OK, sorry - I should've read your description of the assertion more
carefully. I believe the assertion you added wasn't the right thing to test
for.

I'm not sure there is a correct assertion to add here to detect the case
you're describing - perhaps a complex verification after-the-fact could be
done, but essentially if we have a DIE that's partially constructed/has no
parent we should assume it's in the current unit. If we can demonstrate a
case where this isn't true, then we should work to address that problem -
until we demonstrate that, we should not (though we might want to search
for such cases - but without type units I can't imagine how they could
occur - we build the DIE tree for one CU at a time, at no point do we have
DIEs under construction for multiple CUs).

So if we want to build a reference to a DIE, if the DIE is not in another
CU we should use ref4. (then the only other case is that it's either in
this CU or it's in no known CU at all - in which case it's under
construction and, without evidence to the contrary, will be added to the
current CU when it's done).

About the only assertion we could add would involve keeping a side-table of
"assumptions" ("we expect this DIE will be added to this CU") and check
that those assumptions are fulfilled at some point.

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that?

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.

There are two ways of handling this:
1> use a worklist to be conservative
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.

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

The assertion fails even with a simple testing case when the referenced
DIE has an owner and the DIE itself does not have an owner.

OK, sorry - I should've read your description of the assertion more
carefully. I believe the assertion you added wasn't the right thing to test
for.

I'm not sure there is a correct assertion to add here to detect the case
you're describing - perhaps a complex verification after-the-fact could be
done, but essentially if we have a DIE that's partially constructed/has no
parent we should assume it's in the current unit.

Why should we make the assumption that a DIE without a parent at some point
should belong to the CU that is constructing the DIE?
The CU constructing a DIE will add the DIE to the context owner, which may
not belong to the CU itself.
In the case that a DIE is added to an owner in DwarfDebug, I don't think we
should try to enforce that DwarfDebug will add the DIE to the CU that
constructed the DIE earlier.

If we can demonstrate a case where this isn't true, then we should work to
address that problem - until we demonstrate that, we should not (though we
might want to search for such cases - but without type units I can't
imagine how they could occur - we build the DIE tree for one CU at a time,
at no point do we have DIEs under construction for multiple CUs).

So if we want to build a reference to a DIE, if the DIE is not in another
CU we should use ref4. (then the only other case is that it's either in
this CU or it's in no known CU at all - in which case it's under
construction and, without evidence to the contrary, will be added to the
current CU when it's done).

About the only assertion we could add would involve keeping a side-table
of "assumptions" ("we expect this DIE will be added to this CU") and check
that those assumptions are fulfilled at some point.

In my opinion, if we can't verify the assumption with a reasonable amount
of effort, then we should not make the assumption.

Manman

David,

Thanks for reviewing!

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 :slight_smile:

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

That may be true. But can we prove that?

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.

There are two ways of handling this:
1> use a worklist to be conservative
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.

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

The assertion fails even with a simple testing case when the referenced
DIE has an owner and the DIE itself does not have an owner.

OK, sorry - I should've read your description of the assertion more
carefully. I believe the assertion you added wasn't the right thing to test
for.

I'm not sure there is a correct assertion to add here to detect the case
you're describing - perhaps a complex verification after-the-fact could be
done, but essentially if we have a DIE that's partially constructed/has no
parent we should assume it's in the current unit.

Why should we make the assumption that a DIE without a parent at some
point should belong to the CU that is constructing the DIE?

Because the code we have today only constructs one CU at a time. I know of
no code that adds anything to prior CUs. Indeed this may become an
invariant one day when we do CU-at-a-time DWARF emission to reduce memory
overhead. There's nothing in the design today that I know of that would
make CU-at-a-time DWARF emission anything other than trivial. We build all
the DIEs for a CU in one go, then move on to the next CU - the CU_Nodes
loop in beginModule does this.

The CU constructing a DIE will add the DIE to the context owner, which may
not belong to the CU itself.

That's the question, isn't it? When is it ever /not/ the current CU under
construction?

In the case that a DIE is added to an owner in DwarfDebug, I don't think
we should try to enforce that DwarfDebug will add the DIE to the CU that
constructed the DIE earlier.

My claim is that this is already an invariant. That the code you are adding
is never needed and thus is additional, unjustified/unused complexity that
comes at a cost to all future developers/development. This codebase needs
/much/ less of this than it already has, let alone adding more of it.
Though I wouldn't mind an assertion, I suspect it'll be more code than is
really worth it to keep track of this information/state.

I think if we more to CU-at-a-time DWARF emission it'll be more obvious
that this invariant is true and cannot be violated.

If we can demonstrate a case where this isn't true, then we should work
to address that problem - until we demonstrate that, we should not (though
we might want to search for such cases - but without type units I can't
imagine how they could occur - we build the DIE tree for one CU at a time,
at no point do we have DIEs under construction for multiple CUs).

So if we want to build a reference to a DIE, if the DIE is not in another
CU we should use ref4. (then the only other case is that it's either in
this CU or it's in no known CU at all - in which case it's under
construction and, without evidence to the contrary, will be added to the
current CU when it's done).

About the only assertion we could add would involve keeping a side-table
of "assumptions" ("we expect this DIE will be added to this CU") and check
that those assumptions are fulfilled at some point.

In my opinion, if we can't verify the assumption with a reasonable amount
of effort, then we should not make the assumption.

This leads to unbounded, unjustified complexity that makes the codebase
impossible to maintain. It just cannot be an acceptable method of
development.

- David