Add column info for inline sites

[wenlei]

Summary

The column field is missing for all inline sites, currently it's always zero. This changes populates DW_AT_call_column field for inline sites. Test case modified to cover this change.

[probinson]

I suspect there are more tests that must or should be updated; grepping for "DW_AT_call_" turns up for example:
llvm/test/DebugInfo/NVPTX/debug-info.ll
llvm/test/DebugInfo/X86/dbg-value-inlined-parameter.ll
llvm/test/tools/dsymutil/odr-member-functions.cpp

and there are a variety of .s files with hand-generated DWARF that probably should be updated as well, under llvm/test/tools/llvm-dwarfdump.

[wenlei]

Update more tests.

[wenlei]

In D64033#1566445, @probinson wrote:

I suspect there are more tests that must or should be updated; grepping for "DW_AT_call_" turns up for example:
llvm/test/DebugInfo/NVPTX/debug-info.ll
llvm/test/DebugInfo/X86/dbg-value-inlined-parameter.ll
llvm/test/tools/dsymutil/odr-member-functions.cpp

and there are a variety of .s files with hand-generated DWARF that probably should be updated as well, under llvm/test/tools/llvm-dwarfdump.

Thanks for taking a look, Paul. You're right that more tests should be updated. I only fixed tests that failed, and I didn't include NVPTX target, so that one was missed. Now I updated all related tests except those hand generated .s under llvm-dwarfdump. I noticed comments saying some of these .s needs manual patching to introduce bad dwarf content for testing error handling, and it wasn't clear from the test how the manual patch should be done, so it's probably safer to leave it as is (and these tests involving .s input don't really test things related to inline/call sites). Let me know if that's ok..

[JDevlieghere]

In D64033#1576981, @wenlei wrote:

In D64033#1566445, @probinson wrote:

I suspect there are more tests that must or should be updated; grepping for "DW_AT_call_" turns up for example:
llvm/test/DebugInfo/NVPTX/debug-info.ll
llvm/test/DebugInfo/X86/dbg-value-inlined-parameter.ll
llvm/test/tools/dsymutil/odr-member-functions.cpp

and there are a variety of .s files with hand-generated DWARF that probably should be updated as well, under llvm/test/tools/llvm-dwarfdump.

Thanks for taking a look, Paul. You're right that more tests should be updated. I only fixed tests that failed, and I didn't include NVPTX target, so that one was missed. Now I updated all related tests except those hand generated .s under llvm-dwarfdump. I noticed comments saying some of these .s needs manual patching to introduce bad dwarf content for testing error handling, and it wasn't clear from the test how the manual patch should be done, so it's probably safer to leave it as is (and these tests involving .s input don't really test things related to inline/call sites). Let me know if that's ok..

I think it's fair to leave those alone. Paul, do you agree? Otherwise this patch LGTM.

[JDevlieghere]

In D64033#1577243, @JDevlieghere wrote:

I think it's fair to leave those alone. Paul, do you agree? Otherwise this patch LGTM.

Actually, tools/dsymutil/X86/odr-member-functions.cpp is failing for me with this patch applied.

[probinson]

I guess we can leave the .s file tests alone. Most of them look like they were derived from real -S output and it would be pedantically consistent to update them, but they don't have to be.
@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

[JDevlieghere]

In D64033#1577390, @probinson wrote:

I guess we can leave the .s file tests alone. Most of them look like they were derived from real -S output and it would be pedantically consistent to update them, but they don't have to be.
@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

I applied the patch and that obviously doesn't include the binaries, my bad. Do we need to regenerate these binaries at all? I'm tempted to just leave this test as-is.

[wenlei]

@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

The original .o files were for MIPS, my updated ones are for x86_64 as I didn't build MIPS target, and I thought that's not important for what's being tested. I think that's the reason for the size difference.

[wenlei]

In D64033#1577463, @JDevlieghere wrote:

In D64033#1577390, @probinson wrote:

I guess we can leave the .s file tests alone. Most of them look like they were derived from real -S output and it would be pedantically consistent to update them, but they don't have to be.
@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

I applied the patch and that obviously doesn't include the binaries, my bad. Do we need to regenerate these binaries at all? I'm tempted to just leave this test as-is.

These binaries are input for this test, so if we want to verify DW_AT_call_column, we'll need to update the input. But I agree that we don't have to, and I'm fine either way.

[JDevlieghere]

In D64033#1577696, @wenlei wrote:

@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

The original .o files were for MIPS, my updated ones are for x86_64 as I didn't build MIPS target, and I thought that's not important for what's being tested. I think that's the reason for the size difference.

I would have been very surprised if these object files were MIPS, and according to otool and lipo they're definitely x86_64. Anyway, like I said, if Paul agrees I don't think we need to update these.

$ lipo  -info test/tools/dsymutil/Inputs/odr-member-functions/1.o
Non-fat file: test/tools/dsymutil/Inputs/odr-member-functions/1.o is architecture: x86_64
$ lipo  -info test/tools/dsymutil/Inputs/odr-member-functions/2.o
Non-fat file: test/tools/dsymutil/Inputs/odr-member-functions/2.o is architecture: x86_64
$ lipo  -info test/tools/dsymutil/Inputs/odr-member-functions/3.o
Non-fat file: test/tools/dsymutil/Inputs/odr-member-functions/3.o is architecture: x86_64

[probinson]

If the change from DW_AT_MIPS_linkage_name to DW_AT_linkage_name made you think the older .o files were MIPS, more likely it is a change in the default DWARF version; older versions of DWARF did not have DW_AT_linkage_name.

I think the .ll tests are sufficient to show we emit the call_column so no need to update the odr-member-functions tests IMO.

[wenlei]

Undo changes to odr-member-functions test.

[JDevlieghere]

LGTM. Thank you!

[wenlei]

In D64033#1578894, @probinson wrote:

If the change from DW_AT_MIPS_linkage_name to DW_AT_linkage_name made you think the older .o files were MIPS, more likely it is a change in the default DWARF version; older versions of DWARF did not have DW_AT_linkage_name.

I think the .ll tests are sufficient to show we emit the call_column so no need to update the odr-member-functions tests IMO.

Yes, that's what made me think they were MIPS.. thanks for the context. DWARF/ELF is all new to me coming from PDB/COFF/PE world. :) I've reverted the update to that test case, ninja check passed locally.

[probinson]

LGTM2

[wenlei]

In D64033#1580985, @JDevlieghere wrote:

LGTM. Thank you!

Thanks for reviewing! Could you help land this change? I don't have commit permission yet.

[dblaikie]

What's the motivation for this?
Should it be conditional on "-gcolumn-info"? (or skipping it in general if the column is zero? that'd make "-gno-column-info" fall out naturally)
(we don't put decl_column on subprogram DIEs, for instance - admittedly, it's more likely the same function could have multiple inline call sites on the same line than you'd have multiple functions defined on the same line)

[wenlei]

In D64033#1586625, @dblaikie wrote:

What's the motivation for this?

We need to differentiate inline sites on the same line for an internal (basically the tool needs to annotate each instruction with stack of inline site locations). And the missing column info for inline sites gets in the way. I thought the change will be helpful to others as well..

Should it be conditional on "-gcolumn-info"? (or skipping it in general if the column is zero? that'd make "-gno-column-info" fall out naturally)
(we don't put decl_column on subprogram DIEs, for instance - admittedly, it's more likely the same function could have multiple inline call sites on the same line than you'd have multiple functions defined on the same line)

Good point. Thanks David. I will send a separate patch to skip zero column so we won't have column when "-gno-column-info" is used.

[dblaikie]

In D64033#1586885, @wenlei wrote:

In D64033#1586625, @dblaikie wrote:

What's the motivation for this?

We need to differentiate inline sites on the same line for an internal (basically the tool needs to annotate each instruction with stack of inline site locations). And the missing column info for inline sites gets in the way. I thought the change will be helpful to others as well..

For what it's worth, this still won't disambiguate all inline calls (LLVM used to try to use this technique to disambiguate instructions from two different inlined calls & it would fail sometimes even after we forced column info on to help), in this sort of situation:

#define FOO x(); x()
void x();
int main() {
  FOO;
}

Both calls to 'x' will be attributed to the location of the FOO usage, line 4 column 3.

Should it be conditional on "-gcolumn-info"? (or skipping it in general if the column is zero? that'd make "-gno-column-info" fall out naturally)
(we don't put decl_column on subprogram DIEs, for instance - admittedly, it's more likely the same function could have multiple inline call sites on the same line than you'd have multiple functions defined on the same line)

Good point. Thanks David. I will send a separate patch to skip zero column so we won't have column when "-gno-column-info" is used.

Cool cool - thanks!

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

DIE *DwarfCompileUnit::constructInlinedScopeDIE(LexicalScope *Scope) {
 
   attachRangesOrLowHighPC(*ScopeDIE, Scope->getRanges());
 
   // Add the call site information to the DIE.
   const DILocation *IA = Scope->getInlinedAt();
   addUInt(*ScopeDIE, dwarf::DW_AT_call_file, None,
           getOrCreateSourceID(IA->getFile()));
   addUInt(*ScopeDIE, dwarf::DW_AT_call_line, None, IA->getLine());
+  addUInt(*ScopeDIE, dwarf::DW_AT_call_column, None, IA->getColumn());
   if (IA->getDiscriminator() && DD->getDwarfVersion() >= 4)
     addUInt(*ScopeDIE, dwarf::DW_AT_GNU_discriminator, None,
             IA->getDiscriminator());
 
   // Add name to the name table, we do this here because we're guaranteed
   // to have concrete versions of our DW_TAG_inlined_subprogram nodes.
   DD->addSubprogramNames(*CUNode, InlinedSP, *ScopeDIE);
 

llvm/test/DebugInfo/Inputs/gmlt.ll

 ; As mentioned above - replace DW_AT_abstract_origin with DW_AT_name to save
 ; space once we have support for string indexing in non-dwo sections
 
 ; CHECK-NEXT:       DW_AT_abstract_origin {{.*}} "f3"
 ; CHECK-NEXT:       DW_AT_low_pc
 ; CHECK-NEXT:       DW_AT_high_pc
 ; CHECK-NEXT:       DW_AT_call_file
 ; CHECK-NEXT:       DW_AT_call_line
+; CHECK-NEXT:       DW_AT_call_column
 
 ; Make sure we don't have any other subprograms here (subprograms with no
 ; inlined subroutines are omitted by design to save space)
 
 ; CHECK-NOT: {{DW_TAG|DW_AT}}
 ; CHECK: NULL
 ; CHECK-NOT: {{DW_TAG|DW_AT}}
 ; CHECK: NULL

llvm/test/DebugInfo/X86/fission-inline.ll

 ; CHECK-NEXT:   DW_AT_name {{.*}} "f3"
 ; CHECK-NOT: {{DW_|NULL}}
 ; CHECK:        DW_TAG_inlined_subroutine
 ; CHECK-NEXT:     DW_AT_abstract_origin {{.*}} "f2<int>"
 ; CHECK-NEXT:     DW_AT_ranges
 ; CHECK-NOT: {{DW_AT|DW_TAG|NULL}}
 ; CHECK:     DW_AT_call_file
 ; CHECK-NEXT:     DW_AT_call_line {{.*}} (18)
+; CHECK-NEXT:     DW_AT_call_column {{.*}} (0x05)
 ; CHECK-NOT: DW_
 ; CHECK: .debug_info.dwo contents:
 
 ; RELOCS-NOT: RELOCATION RECORDS FOR [.rela.debug_ranges]
 
 ; Function Attrs: uwtable
 define void @_ZN3foo2f3Ez(...) #0 align 2 !dbg !10 {
 entry: