[GCOV] Skip artificial functions from being emitted

[aganea]

Summary

I had to revert D66328 because it was breaking asan, see here or here. Before I could commit it again, it would need this patch.

[rnk]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

731–732 ↗

(On Diff #219119)

Artificial lines here are skipped. If gcov is running, it means that we have an artificial function which contains non-artificial debug locations, otherwise functionHasLines would return false. Do you think we should just add this guard to the other addLine call maybe?

+@nlewycky @dblaikie, since they show up in the blame at rL210239, which added this check.

[dblaikie]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

731–732 ↗

(On Diff #219119)

It's... errr... been a while?

I'm not sure I'll be much more use than a blind/fresh reading of the code/commit history here. But if it's useful to have a second set of eyes (mine) - I might need more words (just rephrasing your question, @rnk with some more words to help me understand what's happening here/being proposed)

[aganea]

As suggested by @rnk

[aganea]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

731–732 ↗

(On Diff #219119)

@dblaikie : Code such as:

int foo() { return 1; }
int XXX = foo();

Generates a `dynamic initializer for 'XXX'. The change in D66328 now marks the generated function as artificial, and only the call to foo() has a line location:

define internal void @"??__EXXX@@YAXXZ"() #1 !dbg !12 {
entry:
  %call = call i32 @"?foo@@YAHXZ"(), !dbg !13
  store i32 %call, i32* @"?XXX@@3HA", align 4, !dbg !15
  ret void, !dbg !13
}

and

!12 = distinct !DISubprogram(name: "`dynamic initializer for 'XXX'", scope: !1, file: !1, type: !10, flags: DIFlagArtificial, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)
!13 = !DILocation(line: 8, column: 11, scope: !14)
!14 = !DILexicalBlockFile(scope: !12, file: !9, discriminator: 0)
!15 = !DILocation(line: 0, scope: !12)

Which, before this patch, caused instrumentation to fail.

[aganea]

NOTE: The test that was breaking before: compiler-rt/test/asan/TestCases/asan_and_llvm_coverage_test.cpp (and inherently, it now runs on Windows if removing the "UNSUPPORTED:" line)

[aganea]

Adding more reviewers.

[aprantl]

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

718

The proper way of detecting artificial functions is look for SPFlagArtificial / DISubprogram::isArtificial(), not the line number.

[aganea]

As requested. I also took the liberty to enable the (previously) failing asan test on Windows, it works perfectly well. Tested on Linux as well.

[dblaikie]

Trying to piece together the history here

https://bugs.llvm.org/show_bug.cgi?id=43012 - an assertion failure when using debug info (the classic "inlinable function call in a function with debug info must have a !dbg location").

https://reviews.llvm.org/D66328 to fix PR43012, has two associated commits
r369458: [DebugInfo] Add debug location to dynamic atexit destructor
-> reverted in r69633
r371080: [DebugInfo] Add debug location to stubs generated by CGDeclCXX and mark them as artificial

I can't reproduce the original crash right now based on the repro steps in PR43012, but perhaps I'm doing something incorrect?
If not, then perhaps r371080 fixed the original bug? So there's some other bugs that are being fixed by r369458/this patch being reviewed that unblocks that change?

Does this hit the same GCOV issue due to line zeros? But the existing fix to line zeros should hold here - but GCOV trips over a function defined at line zero? Or a function without any non-zero lines?

[aganea]

@dblaikie : I had to revert *all* the revisions you're mentioning, because of this issue with GCOV instrumentation. This current patch is required before re-landing D66328.

For a repro with latest git upstream, you need to compile with clang and LLVM_ENABLE_ASSERTIONS=ON, and run the test cfe/trunk/test/CodeGenCXX/debug-info-atexit-stub.cpp from D66328. The issue is still there.

GCOV trips with D66328 applied because the entry block for `dynamic initializer for 'XXX' is now marked as artificial and doesn't have a line #. Please see my comment above. Let me know if it's unclear and needs more explanation.

[rnk]

lgtm

Sorry for the delay, things happened, I got distracted.

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

741–742

@aprantl I suspect this preexisting code was the model for the check above. Maybe it should be standardized to isArtificial to. This is not blocking to this patch, of course.

[aprantl]

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

741–742

You can easily have line 0 locations inside a non-artificial function, but the comment doesn't make it sound like that's what they were after here...

compiler-rt/test/asan/TestCases/asan_and_llvm_coverage_test.cpp

 // RUN: %clangxx_asan -coverage -O0 %s -o %t
 // RUN: %env_asan_opts=check_initialization_order=1 %run %t 2>&1 | FileCheck %s
 
-// We don't really support running tests using profile runtime on Windows.
-// UNSUPPORTED: windows-msvc
-
 #include <stdio.h>
 int foo() { return 1; }
 int XXX = foo();
 int main() {
   printf("PASS\n");
 // CHECK: PASS
 }

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

for (auto &F : M->functions()) {
                                                 Options.UseCfgChecksum,
                                                 Options.ExitBlockBeforeBody));
       GCOVFunction &Func = *Funcs.back();
 
       // Add the function line number to the lines of the entry block
       // to have a counter for the function definition.
       uint32_t Line = SP->getLine();
       auto Filename = getFilename(SP);
+
+      // Artificial functions such as global initializers
+      if (!SP->isArtificial())
         Func.getBlock(&EntryBlock).getFile(Filename).addLine(Line);
 
       for (auto &BB : F) {
         GCOVBlock &Block = Func.getBlock(&BB);
         Instruction *TI = BB.getTerminator();
         if (int successors = TI->getNumSuccessors()) {
           for (int i = 0; i != successors; ++i) {
             Block.addEdge(Func.getBlock(TI->getSuccessor(i)));
           }
         } else if (isa<ReturnInst>(TI)) {
           Block.addEdge(Func.getReturnBlock());
         }
 
         for (auto &I : BB) {
           // Debug intrinsic locations correspond to the location of the
           // declaration, not necessarily any statements or expressions.
           if (isa<DbgInfoIntrinsic>(&I)) continue;
 
           const DebugLoc &Loc = I.getDebugLoc();
           if (!Loc)
             continue;
 
           // Artificial lines such as calls to the global constructors.
           if (Loc.getLine() == 0 || Loc.isImplicitCode())
             continue;
 
           if (Line == Loc.getLine()) continue;
           Line = Loc.getLine();
           if (SP != getDISubprogram(Loc.getScope()))
             continue;
 
           GCOVLines &Lines = Block.getFile(Filename);