[SantizerCoverage] handle missing DBG MD when inserting libcalls

[nickdesaulniers]

Summary

Instruction::getDebugLoc can return an invalid DebugLoc. For such cases
where metadata was accidentally removed from the libcall insertion
point, simply insert a DILocation with line 0 scoped to the caller. When
we can inline the libcall, such as during LTO, then we won't fail a
Verifier check that all calls to functions with debug metadata
themselves must have debug metadata.

[nickdesaulniers]

WIP since I need to think more about the best way to fix this, but here's an example that trips the assertion. Very relevant is the statement in llvm::SplitCriticalEdge:

NewBI->setDebugLoc(TI->getDebugLoc());

where the synthesized block gets its debug info from the predecessor's branch (which can be none by accident).

Also, this is forked from https://reviews.llvm.org/D100137.

[dblaikie]

My best guess would be that any call synthesized from an instruction without a location should probably just get a zero line scoped directly to the current function. It's not ideal (not only the lack of location, but it also punches holes in scopes which can bloat debug info/make stepping more difficult as you hit discontiguous hunks of lines potentially, etc)

[nickdesaulniers]

In D100158#2678482, @dblaikie wrote:

My best guess would be that any call synthesized from an instruction without a location should probably just get a zero line scoped directly to the current function.

Sure. Should we do that unconditionally for the inserted CallInst, or predicate it on debug info being requested for the compilation, or something else?

It's not ideal (not only the lack of location, but it also punches holes in scopes which can bloat debug info/make stepping more difficult as you hit discontiguous hunks of lines potentially, etc)

For the purpose of ModuleSanitizerCoveragePass, I think that's ok. Developers accept a large increase in binary size anyways using the most aggressive level of tracing every edge, and stepping in a debugger probably isn't the use case folks are intending when using ModuleSanitizerCoveragePass. I would think the major use case for debug info with coverage is to symbolicate traces.

[dblaikie]

In D100158#2680116, @nickdesaulniers wrote:

In D100158#2678482, @dblaikie wrote:

My best guess would be that any call synthesized from an instruction without a location should probably just get a zero line scoped directly to the current function.

Sure. Should we do that unconditionally for the inserted CallInst, or predicate it on debug info being requested for the compilation, or something else?

This constraint (that calls have a !dbg location) only exists when the calling llvm::Function has a !dbg attached DISubprogram. So basically retrieve the caller's DISubprogram, if it exists, create a line 0 DILocation with the DISubprogram as the scope.

(we might already have some code for this in the merge debug location code? some fallback for call instructions when merged locations disagree or one is empty, etc)

[nickdesaulniers]

• handle case where existing DebugLoc was invalid

[nickdesaulniers]

So basically retrieve the caller's DISubprogram, if it exists, create a line 0 DILocation with the DISubprogram as the scope.

Cool, yeah, that's what this pass already did; it just did not forsee that EntryLoc = IP->getDebugLoc(); could return an invalid DebugLoc (ie. because !dbg metadata was missing from the insertion point). Let me rephrase the description of this bug, then I think it's ready for review.

[nickdesaulniers]

• fix lint about auto*

[dblaikie]

llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll

24

DBG7 isn't checked against anything - should it be? (I guess it (the EntryLoc = IP->getDebugLoc(); line, I guess) is hopefully already tested by existing tests?)

25

Probably good to check this is actually line 0, since that's what the code does intentionally - it'd be weird/problematic if we invented/used some other location.

[nickdesaulniers]

• add DBG7 check, specific lines

[nickdesaulniers]

llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll

24

I can add it.

I guess it (the EntryLoc = IP->getDebugLoc(); line, I guess) is hopefully already tested by existing tests?

Yes, llvm/test/Instrumentation/SanitizerCoverage/coverage2-dbg.ll does.

25

I think it will be scopeLine of parent plus 0.

[dblaikie]

llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll

25

hmm, wait, that doesn't sound right - sorry, didn't realize that's what the code is doing. The code should be setting the line to 0, not the scopeLine. (line 0 is for lines that aren't related to any particular/identifiable line of source code)

[nickdesaulniers]

• fix line 0 DILocation

[dblaikie]

Looks good - thanks!

[nickdesaulniers]

Thanks as always for the review; appreciated!

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

if (IsEntryBB) {
     if (auto SP = F.getSubprogram())
       EntryLoc = DILocation::get(SP->getContext(), SP->getScopeLine(), 0, SP);
     // Keep static allocas and llvm.localescape calls in the entry block.  Even
     // if we aren't splitting the block, it's nice for allocas to be before
     // calls.
     IP = PrepareToSplitEntryBlock(BB, IP);
   } else {
     EntryLoc = IP->getDebugLoc();
+    if (!EntryLoc)
+      if (auto *SP = F.getSubprogram())
+        EntryLoc = DILocation::get(SP->getContext(), 0, 0, SP);
   }
 
   IRBuilder<> IRB(&*IP);
   IRB.SetCurrentDebugLocation(EntryLoc);
   if (Options.TracePC) {
     IRB.CreateCall(SanCovTracePC)
         ->setCannotMerge(); // gets the PC using GET_CALLER_PC.
   }

llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll

+; RUN: opt -passes='module(sancov-module)' -sanitizer-coverage-trace-pc \
+; RUN: -sanitizer-coverage-level=3 %s -S -o - | FileCheck %s
+
+; The edge between %entry and %for.inc.i is a critical edge.
+; ModuleSanitizerCoveragePass must split this critical edge in order to track
+; coverage of this edge. ModuleSanitizerCoveragePass will also insert calls to
+; @__sanitizer_cov_trace_pc using the debug location from the predecessor's
+; branch.  but, if the branch itself is missing debug info (say, by accident
+; due to a bug in an earlier transform), we would fail a verifier check that
+; verifies calls to functions with debug info themselves have debug info.
+; The definition of @__sanitizer_cov_trace_pc may be visible during LTO.
+
+; Of the below checks, we really only care that the calls to
+; @__sanitizer_cov_trace_pc retain !dbg metadata.
+
+define void @update_shadow() !dbg !3 {
+; CHECK-LABEL: @update_shadow(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__sanitizer_cov_trace_pc() #[[ATTR0:[0-9]+]], !dbg [[DBG6:![0-9]+]]
+; CHECK:       entry.for.inc.i_crit_edge:
+; CHECK-NEXT:    call void @__sanitizer_cov_trace_pc() #[[ATTR0]], !dbg [[DBG7:![0-9]+]]
+; CHECK:       if.end22.i:
+; CHECK-NEXT:    call void @__sanitizer_cov_trace_pc() #[[ATTR0]], !dbg [[DBG8:![0-9]+]]
+; CHECK:       [[DBG6]] = !DILocation(line: 192, scope: !3)
+; CHECK:       [[DBG7]] = !DILocation(line: 0, scope: !3)
+; CHECK:       [[DBG8]] = !DILocation(line: 129, column: 2, scope: !3)
+entry:
+  br i1 undef, label %for.inc.i, label %if.end22.i
+
+if.end22.i:                                       ; preds = %entry
+  br label %for.inc.i, !dbg !8
+
+for.inc.i:                                        ; preds = %if.end22.i, %entry
+  ret void, !dbg !6
+}
+
+define void @__sanitizer_cov_trace_pc() !dbg !7{
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C89, file: !1, producer: "", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "kernel/cfi.c", directory: "")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(name: "update_shadow", scope: !1, file: !1, line: 190, type: !4, scopeLine: 192, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!4 = !DISubroutineType(types: !5)
+!5 = !{}
+!6 = !DILocation(line: 223, column: 1, scope: !3)
+!7 = distinct !DISubprogram(name: "__sanitizer_cov_trace_pc", scope: !1, file: !1, line: 200, type: !4, scopeLine: 200, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!8 = !DILocation(line: 129, column: 2, scope: !3)