Page MenuHomePhabricator

[DWARF] Line 0 should not have a discriminator
ClosedPublic

Authored by probinson on Aug 31 2017, 5:45 PM.

Details

Summary

We have line 0 inherit the scope of the previous non-zero line, to limit the size cost of the line 0 records; but that scope might have a discriminator attached, and a discriminator on line 0 is pointless.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.Aug 31 2017, 5:45 PM
dblaikie edited edge metadata.

+Dehao to check on the semantics of this

test/CodeGen/Generic/dbg-line-0-no-discriminator.ll
7–15 ↗(On Diff #113493)

Looks like test/DebugInfo/X86/discriminator2.ll might provide a simpler example code that creates discriminators using distinct call sites on the same source line? (maybe not - but was hoping for something a bit simpler/more direct than the code here)

danielcdh edited edge metadata.Sep 3 2017, 5:33 PM

The change looks good to me. But the test is quite large and hard to understand. I don't think it's necessary to have the test generated from a source file, just forge a test with discriminator attached to line 0, and then checks if discriminators is omitted in the generated code. I guess it should suffice with < 20 LOC

The change looks good to me. But the test is quite large and hard to understand. I don't think it's necessary to have the test generated from a source file, just forge a test with discriminator attached to line 0, and then checks if discriminators is omitted in the generated code. I guess it should suffice with < 20 LOC

If the IR explicitly has a line 0 location with a nonzero discriminator, I think DwarfDebug should obediently emit that (or assert).
The point of this patch is that when DwarfDebug *implicitly* emits a line 0 record, a preceding discriminator should not "pollute" that.

An MIR test that set up the situation more directly might be better, although I am not super confident in my ability to hand-reduce an MIR input.

probinson updated this revision to Diff 114255.Sep 7 2017, 2:28 PM

Seriously simplified test (derived from DebugInfo/X86/discriminator2.ll, as suggested by dblaikie).

dblaikie accepted this revision.Sep 7 2017, 2:33 PM

Seems OK to me

For a minor perk - would it be possible to test the line table length? Since the benefit (smaller line table) of this change isn't readily apparent in the high level line table printing that dwarfdump does. I know it'd make the test a bit more brittle.

This revision is now accepted and ready to land.Sep 7 2017, 2:33 PM

Seems OK to me

For a minor perk - would it be possible to test the line table length? Since the benefit (smaller line table) of this change isn't readily apparent in the high level line table printing that dwarfdump does. I know it'd make the test a bit more brittle.

Yeah... I can add checks for both total_length and prologue_length, with a comment that says what we really care about is that the difference doesn't change unexpectedly. The metadata is specifying DWARF 4 so it shouldn't be *that* brittle.

This revision was automatically updated to reflect the committed changes.