This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Add a test for debug info generated with split functions.
AbandonedPublic

Authored by snehasish on Nov 3 2020, 2:28 PM.

Details

Summary

This commit adds a test for debug info generated when the machine
function splitter pass is enabled. We check that the line numbers for
the hot and cold parts of the split function are what we expect them to
be.

Diff Detail

Event Timeline

snehasish created this revision.Nov 3 2020, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2020, 2:29 PM
snehasish requested review of this revision.Nov 3 2020, 2:29 PM
saugustine accepted this revision.Nov 3 2020, 2:53 PM

I can't speak to its technical correctness, but this is exactly the test I requested and it looks good at a high level.

This revision is now accepted and ready to land.Nov 3 2020, 2:53 PM
snehasish updated this revision to Diff 302704.Nov 3 2020, 3:34 PM

Add UNSUPPORTED tag for windows and mac os.

Thanks for the quick review @saugustine , I'll wait for @tmsriram take a look.

tmsriram added inline comments.Nov 3 2020, 10:59 PM
llvm/test/DebugInfo/X86/machine-function-splitter.ll
23

What does this correspond to?

31

Isn't this the cold basic block? What is split out here?

49

Do we need a -gsplit-dwarf fission test too?

MaskRay added inline comments.
llvm/test/DebugInfo/X86/machine-function-splitter.ll
49

The current test only checks -g1 line table. If it targets -g2 (and I think it probably should), I expect that more stuff should be tested.
Adding @dblaikie who knows better how to construct an interesting test.

MaskRay added a comment.EditedNov 3 2020, 11:12 PM

In addition, I wonder where this is layering issue for the testing. This patch adds llvm-objdump -l which is not common in test/DebugInfo. In fact, there is only one test using -S/--source/-l/--line-numbers, so I wonder whether it is the most appropriate testing approach here.

Perhaps this patch should test .loc instead.

dblaikie added inline comments.Nov 4 2020, 3:44 PM
llvm/test/DebugInfo/X86/machine-function-splitter.ll
3

This doesn't sound right - we can run ELF tests on non-ELF platforms. (I assume that's what's trying to be avoided/addressed by this?) Note all the other tests in this directory that don't have such UNSUPPORTED tagging.

49

If this is a general discussion of "What debug info related things should we test for bb-sections in the LLVM project" it'd be good to get a quick summary of what testing already exists.

But otherwise, broad testing I'd think:

  • llvm-symbolizer testing (check a couple of addresses in different bb sections, covering an inlined function)
  • llvm-dwarfdump of a linked executable showing bb section line table, DW_AT_ranges on the DW_TAG_subprogram

But maybe both of these tests are already checked in/implemented?

I don't think there's much need for Split DWARF-specific testing, as the Split DWARF handling of ranges is fairly orthogonal to where they appear (shouldn't be interesting to Split DWARF that the ranges appear on a DW_TAG_subprogram, I don't think).

tmsriram added inline comments.Nov 4 2020, 3:55 PM
llvm/test/DebugInfo/X86/machine-function-splitter.ll
49

Yes the llvm-dwarfdump and the split dwarf testing of DW_AT_ranges is already there: test/DebugInfo/X86/basic-block-sections_1.ll
I didnt add a llvm-symbolizer test, only did lldb tests. I can add a symbolizer test.

dblaikie added inline comments.Nov 4 2020, 4:01 PM
llvm/test/DebugInfo/X86/machine-function-splitter.ll
49

Could the coverage this new test is adding be added to the existing test? Looks like this new test is intended to cover the line table - which could be added to expanded coverage in the basic-block-sections_1.ll by dumping debug_line as well as debug_info (adding "-debug-line" to those existing dwarfdump commands)

snehasish updated this revision to Diff 303278.Nov 5 2020, 3:32 PM

Updates based on review comments.

  • Use llvm-dwarfdump --debug-line instead of llvm-objdump -l
  • Updated the checks, added line numbers to the source.
  • Removed unsupported tag, should no longer be required since we don't use llvm-objdump anymore.

Thanks for the review all. I've updated the test to use llvm-dwarfdump to check that the line table is as expected for the hot and cold parts.

PTAL @dblaikie @MaskRay
Thanks!

llvm/test/DebugInfo/X86/machine-function-splitter.ll
3

Removed, I think this was failing on windows due to the use of llvm-objdump. I'll keep an eye on the windows bot and double check.

31

The cold block is the one below, the modulus yields the remainder which is interpreted as true for the if condition.

49

I think there is value in having a simple test for the line table which many downstream tools rely on. We should add a line table test for basic block sections too.

dblaikie added inline comments.Nov 5 2020, 5:23 PM
llvm/test/DebugInfo/X86/machine-function-splitter.ll
49

Generally we'd test all the things related to a feature/new/interesting output in the same place. Sometimes worth splitting into multiple files, but there's some benefits to keeping it all together (can page in the context for one feature area, less test process overhead, etc). Admittedly, the tests aren't especially rigorously laid out by any means.

And I see I got this jumbled up between split-machine-functions and bb-sections. What's the difference between those two features? I /guess/ that split-machine-functions is a specific application of/builds on top of bb-sections?

snehasish added inline comments.Nov 5 2020, 6:15 PM
llvm/test/DebugInfo/X86/machine-function-splitter.ll
49

Yes, split-machine-functions is a specific application of basic block sections. The key difference here is the type of profile consumed to drive the layout decisions (FDO/AFDO vs Propeller). From the perspective of the machine function splitting pass this test ensures that the underlying mechanism, i.e. basic block sections updates debug information correctly. Thus I added an independent test. There is some duplication here and if you feel strongly we can just test the line table in the existing basic block sections. WDYT?

dblaikie added inline comments.Nov 5 2020, 7:39 PM
llvm/test/DebugInfo/X86/machine-function-splitter.ll
49

Yeah, seems like it's probably better to test this functionality down at the bb-sections level, since that's the functionality it's really testing/relying on. If several features are built on top of bb-sections, we don't want the fundamental testing to only be done via one of those features (if we then remove that feature - might lose the testing), and makes it clearer what testing covers what functionality/implementation code.

Looks like the debug info functionality being tested isn't specific to split-machine-functions, so I'd generally favor not testing it here and instead testing it with bb-sections. (if there's specific DWARF code/functionality only reachable through split-machine-functions, that should be tested here)

snehasish abandoned this revision.Nov 6 2020, 4:31 PM
snehasish added inline comments.
llvm/test/DebugInfo/X86/machine-function-splitter.ll
49

Sent out https://reviews.llvm.org/D90989 to check the line table for basic block sections.