[DWARF] Line 0 should not have a discriminator

[probinson]

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.

[dblaikie]

+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]

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

[probinson]

In D37364#860065, @danielcdh wrote:

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.

[probinson]

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]

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

[dblaikie]

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.

[probinson]

In D37364#863979, @dblaikie wrote:

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.

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::recordSourceLine(unsigned Line, unsigned Col, const MDNode *S,
                                   unsigned Flags) {
   StringRef Fn;
   StringRef Dir;
   unsigned Src = 1;
   unsigned Discriminator = 0;
   if (auto *Scope = cast_or_null<DIScope>(S)) {
     Fn = Scope->getFilename();
     Dir = Scope->getDirectory();
+    if (Line != 0 && getDwarfVersion() >= 4)
       if (auto *LBF = dyn_cast<DILexicalBlockFile>(Scope))
-      if (getDwarfVersion() >= 4)
         Discriminator = LBF->getDiscriminator();
 
     unsigned CUID = Asm->OutStreamer->getContext().getDwarfCompileUnitID();
     Src = static_cast<DwarfCompileUnit &>(*InfoHolder.getUnits()[CUID])
               .getOrCreateSourceID(Fn, Dir);
   }
   Asm->OutStreamer->EmitDwarfLocDirective(Src, Line, Col, Flags, 0,
                                           Discriminator, Fn);

llvm/trunk/test/CodeGen/X86/dbg-line-0-no-discriminator.ll

+; RUN: llc -filetype=obj -use-unknown-locations=Enable -mtriple=x86_64-unknown-linux %s -o %t
+; RUN: llvm-dwarfdump -debug-dump=line %t | FileCheck %s
+
+define void @_Z3bazv() !dbg !6 {
+  call void @_Z3foov(), !dbg !9
+  call void @_Z3foov() ; no !dbg, so will be marked as line 0
+  ret void, !dbg !11
+}
+
+declare void @_Z3foov()
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.9.0 (trunk 267219)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test.cc", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang version 3.9.0 (trunk 267219)"}
+!6 = distinct !DISubprogram(name: "baz", linkageName: "_Z3bazv", scope: !1, file: !1, line: 3, type: !7, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!7 = !DISubroutineType(types: !8)
+!8 = !{null}
+!9 = !DILocation(line: 4, column: 3, scope: !10)
+!10 = !DILexicalBlockFile(scope: !6, file: !1, discriminator: 1)
+!11 = !DILocation(line: 6, column: 1, scope: !6)
+
+; Look at the lengths. We can't verify the line-number-program size
+; directly, but the difference in the two lengths should not change
+; unexpectedly.
+; CHECK:    total_length: 0x00000043
+; CHECK: prologue_length: 0x0000001e
+;
+; Verify that we see a line entry with a discriminator, and the next entry
+; has line 0 and no discriminator.
+;             line column file ISA discriminator
+; CHECK:      4    3      1    0   1
+; CHECK-NEXT: 0    3      1    0   0