Page MenuHomePhabricator

Add column info for inline sites
ClosedPublic

Authored by wenlei on Jul 1 2019, 2:00 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

wenlei created this revision.Jul 1 2019, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 2:00 PM

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 updated this revision to Diff 208796.Jul 9 2019, 1:26 PM

Update more tests.

wenlei added a comment.Jul 9 2019, 1:43 PM

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

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.

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

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

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 updated this revision to Diff 209261.Jul 11 2019, 10:52 AM

Undo changes to odr-member-functions test.

JDevlieghere accepted this revision.Jul 11 2019, 10:59 AM

LGTM. Thank you!

This revision is now accepted and ready to land.Jul 11 2019, 10:59 AM

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.

LGTM. Thank you!

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

This revision was automatically updated to reflect the committed changes.

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)

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.

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!