[PATCH] D90717: [llvm] Add a test for debug info generated with split functions.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/DebugInfo/X86/machine-function-splitter.ll:48
+;; llvm-profdata merge -o default.profdata default_*.profraw
+;; clang -fprofile-use -O2 -g -S -emit-llvm split_functions.c
+;; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 
----------------
snehasish wrote:
> dblaikie wrote:
> > snehasish wrote:
> > > dblaikie wrote:
> > > > tmsriram wrote:
> > > > > dblaikie wrote:
> > > > > > MaskRay wrote:
> > > > > > > tmsriram wrote:
> > > > > > > > Do we need a -gsplit-dwarf fission test too?
> > > > > > > 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.
> > > > > > 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).
> > > > > 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.
> > > > 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)
> > > 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. 
> > 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?
> 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?
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90717/new/

https://reviews.llvm.org/D90717