[Coverage] Ensure that the hash for a used function is non-zero.

[ikudrin]

Summary

A valid function might not have any statement which affects the hash value, so the hash for that function was zero. The hash value for an unused function is also zero, so the loader could not distinguish the definitions and might load the incorrect one. This patch addresses the issue by assigning a special reserved value as a hash for such a functions.

[ikudrin]

The motivation sample (using llvm-cov with D20286 applied):

$ cat > sample.h << EOF
inline int sample_func(int A) {
  return A;
}
EOF
$ cat > dummy.cpp << EOF
#include "sample.h"
EOF
$ cat > sample.cpp << EOF
#include "sample.h"

int main() {
  return sample_func(5);
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping dummy.cpp sample.cpp
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
sample.h:
      0|    1|inline int sample_func(int A) {
      0|    2|  return A;
      0|    3|}

sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}

And if this patch is applied:

$ clang++ -fprofile-instr-generate -fcoverage-mapping dummy.cpp sample.cpp
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
sample.h:
      1|    1|inline int sample_func(int A) {
      1|    2|  return A;
      1|    3|}

sample.cpp:
       |    1|#include "sample.h"
       |    2|
      1|    3|int main() {
      1|    4|  return sample_func(5);
      1|    5|}

[ikudrin]

Does anyone known, why we need dummy coverage mapping records for unused functions? How are they used? Isn't it better to remove these dummy records to prevent confusion with the real ones?

[davidxl]

Strictly speaking, this patch requires a version bump of the indexed format. The profile reader also needs to adjust the FunctionHash computation (either using 0 or simple function hash) based on the version of the profile data.

Check with Justin/vsk to see if it is important to keep backward compatability for these simple functions in older profile data.

On the other hand, I wonder what is the real root cause of the problem. The dummy function record does not have its 'own' profile counts, so

if (std::error_code EC = ProfileReader.getFunctionCounts(

Record.FunctionName, Record.FunctionHash, Counts)) {

call in CoverageMapping::load (..)

method should return the the counts of the used function even with zero functionhash. What did I miss?

[ikudrin]

In D20287#431055, @davidxl wrote:

On the other hand, I wonder what is the real root cause of the problem. The dummy function record does not have its 'own' profile counts, so

if (std::error_code EC = ProfileReader.getFunctionCounts(
            Record.FunctionName, Record.FunctionHash, Counts)) {

call in CoverageMapping::load (..)

method should return the the counts of the used function even with zero functionhash. What did I miss?

There are no problems in ProfileReader, because only real functions have their profile counts. The problem is in VersionedCovMapFuncRecordReader::readFunctionRecords(...), which has difficulties in distinguishing real and dummy coverage mapping records, so that it might load improper ones in some cases.

[ikudrin]

This change is not needed anymore because the whole issue was fixed in D20286.

lib/CodeGen/CodeGenPGO.cpp

enum HashType : unsigned char {
     DefaultStmt,
     IfStmt,
     CXXTryStmt,
     CXXCatchStmt,
     ConditionalOperator,
     BinaryOperatorLAnd,
     BinaryOperatorLOr,
     BinaryConditionalOperator,
+    SimpleFunctionHash, // Reserved for functions without aforesaid statements.
 
     // Keep this last.  It's for the static assert that follows.
     LastHashType
   };
   static_assert(LastHashType <= TooBig, "Too many types in HashType");
 
   // TODO: When this format changes, take in a version number here, and use the
   // old hash calculation for file formats that used the old hash.

 }
 
 uint64_t PGOHash::finalize() {
   // Use Working as the hash directly if we never used MD5.
   if (Count <= NumTypesPerWord)
     // No need to byte swap here, since none of the math was endian-dependent.
     // This number will be byte-swapped as required on endianness transitions,
     // so we will see the same value on the other side.
-    return Working;
+    // If we didn't meet any known statements, return a special hash value,
+    // so that any used function will have a non-zero hash and won't be
+    // messed up with non-used functions when reading coverage data.
+    return Working ? Working : static_cast<uint64_t>(SimpleFunctionHash);
 
   // Check for remaining work in Working.
   if (Working)
     MD5.update(Working);
 
   // Finalize the MD5 and return the hash.
   llvm::MD5::MD5Result Result;
   MD5.final(Result);

lib/CodeGen/CoverageMappingGen.cpp

static bool isMachO(const CodeGenModule &CGM) {
   return CGM.getTarget().getTriple().isOSBinFormatMachO();
 }
 
 static StringRef getCoverageSection(const CodeGenModule &CGM) {
   return llvm::getInstrProfCoverageSectionName(isMachO(CGM));
 }
 
 static void dump(llvm::raw_ostream &OS, StringRef FunctionName,
-                 ArrayRef<CounterExpression> Expressions,
+                 uint64_t FuncHash, ArrayRef<CounterExpression> Expressions,
                  ArrayRef<CounterMappingRegion> Regions) {
-  OS << FunctionName << ":\n";
+  OS << FunctionName << "(" << FuncHash << "):\n";
   CounterMappingContext Ctx(Expressions);
   for (const auto &R : Regions) {
     OS.indent(2);
     switch (R.Kind) {
     case CounterMappingRegion::CodeRegion:
       break;
     case CounterMappingRegion::ExpansionRegion:
       OS << "Expansion,";
if (CGM.getCodeGenOpts().DumpCoverageMapping) {
     llvm::SmallVector<StringRef, 16> FilenameRefs;
     FilenameRefs.resize(FileEntries.size());
     for (const auto &Entry : FileEntries)
       FilenameRefs[Entry.second] = Entry.first->getName();
     RawCoverageMappingReader Reader(CoverageMapping, FilenameRefs, Filenames,
                                     Expressions, Regions);
     if (Reader.read())
       return;
-    dump(llvm::outs(), NameValue, Expressions, Regions);
+    dump(llvm::outs(), NameValue, FuncHash, Expressions, Regions);
   }
 }
 
 void CoverageMappingModuleGen::emit() {
   if (FunctionRecords.empty())
     return;
   llvm::LLVMContext &Ctx = CGM.getLLVMContext();
   auto *Int32Ty = llvm::Type::getInt32Ty(Ctx);

test/CoverageMapping/block-storage-starts-region.m

 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -triple x86_64-apple-darwin -fobjc-runtime=macosx-10.10.0 -fblocks -fobjc-arc %s | FileCheck %s
 
 @interface Foo
 @end
 
-// CHECK-LABEL: doSomething:
+// CHECK-LABEL: doSomething
 void doSomething() { // CHECK: File 0, [[@LINE]]:20 -> {{[0-9:]+}} = #0
   return;
   __block Foo *f; // CHECK: File 0, [[@LINE]]:3 -> {{[0-9:]+}} = 0
 }
 
 int main() {}

test/CoverageMapping/decl.c

 // Ensure that declarations without definitions don't have maps emitted for them
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s > %t
 // FileCheck -input-file %t %s
 // RUN: FileCheck -check-prefix BAR -input-file %t %s
 
-// FOO: foo:
-// FOO-NOT: foo:
+// FOO: foo
+// FOO-NOT: foo
 inline int foo() { return 0; }
 extern inline int foo();
 
-// BAR: bar:
-// BAR-NOT: bar:
+// BAR: bar
+// BAR-NOT: bar
 int bar() { return 0; }
 extern int bar();

test/CoverageMapping/implicit-def-in-macro.m

 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -triple x86_64-apple-darwin -fobjc-runtime=macosx-10.10.0 -fblocks -fobjc-arc %s | FileCheck %s
 
 @interface Foo
 @end
 #define Bar Foo
 
 @implementation Blah
-// CHECK-LABEL: +[Blah load]:
+// CHECK-LABEL: +[Blah load]
 + load { // CHECK: File 0, [[@LINE]]:8 -> [[END:[0-9:]+]] = #0
   return 0;
   // CHECK: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:6 = 0
   Bar *attribute; // CHECK: File 0, [[@LINE]]:6 -> [[END]] = 0
 }
 @end
 
 int main() {}

test/CoverageMapping/lambda.cpp

 // RUN: %clang_cc1 -x c++ -std=c++11 -triple %itanium_abi_triple -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s -main-file-name lambda.cpp | FileCheck %s
 
-// CHECK-LABEL: _Z3fooi:
+// CHECK-LABEL: _Z3fooi
 void foo(int i) { // CHECK: File 0, [[@LINE]]:17 -> {{[0-9]+}}:2 = #0
   auto f = [](int x) {
     return x + 1;
   };
 
   f(i);
   // Make sure the zero region after the return doesn't escape the lambda.
   // CHECK-NOT: File 0, {{[0-9:]+}} -> [[@LINE+1]]:2 = 0
 }
 
 int main(int argc, const char *argv[]) {
   foo(1);
   return 0;
 }

test/CoverageMapping/loops.cpp

for(auto i : arr) {               // CHECK: File 0, [[@LINE]]:21 -> [[@LINE+6]]:4 = #1
     if (sum >= 7)
       break;                        // CHECK: File 0, [[@LINE]]:7 -> [[@LINE]]:12 = #3
   }
 
   // CHECK: File 0, [[@LINE+1]]:7 -> [[@LINE+1]]:10 = #0
   if (sum) {}
 }
 
-                                    // CHECK: main:
+                                    // CHECK: main
 int main() {                        // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+24]]:2 = #0
                                     // CHECK-NEXT: File 0, [[@LINE+1]]:18 -> [[@LINE+1]]:24 = (#0 + #1)
   for(int i = 0; i < 10; ++i)       // CHECK-NEXT: File 0, [[@LINE]]:26 -> [[@LINE]]:29 = #1
      ;                              // CHECK-NEXT: File 0, [[@LINE]]:6 -> [[@LINE]]:7 = #1
   for(int i = 0;
       i < 10;                       // CHECK-NEXT: File 0, [[@LINE]]:7 -> [[@LINE]]:13 = (#0 + #2)
       ++i)                          // CHECK-NEXT: File 0, [[@LINE]]:7 -> [[@LINE]]:10 = #2
   {                                 // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+2]]:4 = #2

test/CoverageMapping/macroception.c

 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name macroception.c %s | FileCheck %s
 
 #define M2 {
 #define M1 M2
 #define M22 }
 #define M11 M22
 
-// CHECK-LABEL: main:
+// CHECK-LABEL: main
 // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:12 -> [[@LINE+2]]:14 = #0
 // CHECK-NEXT: File 0, [[@LINE+1]]:14 -> [[@LINE+3]]:2 = #0
 int main() M1
   return 0;
 }
 // CHECK-NEXT: File 1, 4:12 -> 4:14 = #0
 // CHECK-NEXT: Expansion,File 1, 4:12 -> 4:14 = #0
 // CHECK-NEXT: File 2, 3:12 -> 3:13 = #0
 
-// CHECK-LABEL: func2:
+// CHECK-LABEL: func2
 // CHECK-NEXT: File 0, [[@LINE+2]]:14 -> [[@LINE+4]]:4 = #0
 // CHECK-NEXT: Expansion,File 0, [[@LINE+3]]:1 -> [[@LINE+3]]:4 = #0
 void func2() {
   int x = 0;
 M11
 // CHECK-NEXT: File 1, 6:13 -> 6:16 = #0
 // CHECK-NEXT: Expansion,File 1, 6:13 -> 6:16 = #0
 // CHECK-NEXT: File 2, 5:13 -> 5:14 = #0
 
-// CHECK-LABEL: func3:
+// CHECK-LABEL: func3
 // CHECK-NEXT: Expansion,File 0, [[@LINE+3]]:14 -> [[@LINE+3]]:16 = #0
 // CHECK-NEXT: File 0, [[@LINE+2]]:16 -> [[@LINE+4]]:4 = #0
 // CHECK-NEXT: Expansion,File 0, [[@LINE+3]]:1 -> [[@LINE+3]]:4 = #0
 void func3() M1
   int x = 0;
 M11
 // CHECK-NEXT: File 1, 4:12 -> 4:14 = #0
 // CHECK-NEXT: Expansion,File 1, 4:12 -> 4:14 = #0
 // CHECK-NEXT: File 2, 6:13 -> 6:16 = #0
 // CHECK-NEXT: Expansion,File 2, 6:13 -> 6:16 = #0
 // CHECK-NEXT: File 3, 3:12 -> 3:13 = #0
 // CHECK-NEXT: File 4, 5:13 -> 5:14 = #0
 
-// CHECK-LABEL: func4:
+// CHECK-LABEL: func4
 // CHECK-NEXT: Expansion,File 0, [[@LINE+3]]:14 -> [[@LINE+3]]:16 = #0
 // CHECK-NEXT: File 0, [[@LINE+2]]:16 -> [[@LINE+2]]:20 = #0
 // CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:17 -> [[@LINE+1]]:20 = #0
 void func4() M1 M11
 // CHECK-NEXT: File 1, 4:12 -> 4:14 = #0
 // CHECK-NEXT: Expansion,File 1, 4:12 -> 4:14 = #0
 // CHECK-NEXT: File 2, 6:13 -> 6:16 = #0
 // CHECK-NEXT: Expansion,File 2, 6:13 -> 6:16 = #0
 // CHECK-NEXT: File 3, 3:12 -> 3:13 = #0
 // CHECK-NEXT: File 4, 5:13 -> 5:14 = #0

test/CoverageMapping/moremacros.c

 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name macro-expansion.c %s | FileCheck %s
 
 #define LBRAC {
 #define RBRAC }
 
-// CHECK: main:
+// CHECK: main
 // CHECK-NEXT: File 0, [[@LINE+1]]:40 -> {{[0-9]+}}:2 = #0
 int main(int argc, const char *argv[]) {
   // CHECK-NEXT: File 0, [[@LINE+1]]:7 -> [[@LINE+1]]:12 = #0
   if (!argc) {} // CHECK-NEXT: File 0, [[@LINE]]:14 -> [[@LINE]]:16 = #1
 
   // CHECK-NEXT: File 0, [[@LINE+3]]:7 -> [[@LINE+3]]:12 = #0
   // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:14 -> [[@LINE+2]]:19 = #2
   // CHECK-NEXT: File 0, [[@LINE+1]]:19 -> [[@LINE+4]]:8 = #2

test/CoverageMapping/simple-function-hash.cpp

+// RUN: %clang_cc1 -main-file-name simple-function-hash.cpp %s -o %t.o -emit-llvm -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping | FileCheck %s
+
+// Hash value for unused_func() is 0 because it is an inline function which is not used in the translation unit.
+// CHECK-DAG: _Z11unused_funcv(0):
+inline int unused_func() { return 1; }
+
+// Hash value for used_func() is 17 even though it does not contain any statement which affects hash.
+// CHECK-DAG: _Z9used_funcv(17):
+inline int used_func() { return 1; }
+
+// Hash value for main() is 10 because it contains the "if" statement.
+// CHECK-DAG: main(10):
+int main() {
+  if (used_func())
+    return used_func();
+  return 0;
+}

test/CoverageMapping/system_macro.c

 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name system_macro.c -o - %s | FileCheck %s
 
 #ifdef IS_SYSHEADER
 
 #pragma clang system_header
 #define Func(x) if (x) {}
 #define SomeType int
 
 #else
 
 #define IS_SYSHEADER
 #include __FILE__
 
-// CHECK-LABEL: doSomething:
+// CHECK-LABEL: doSomething
 void doSomething(int x) { // CHECK: File 0, [[@LINE]]:25 -> {{[0-9:]+}} = #0
   Func(x); // CHECK: Expansion,File 0, [[@LINE]]:3 -> [[@LINE]]:7
   return;
   // CHECK: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:11
   SomeType *f; // CHECK: File 0, [[@LINE]]:11 -> {{[0-9:]+}} = 0
 }
 
 int main() {}
 
 #endif

test/CoverageMapping/trymacro.cpp

 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fexceptions -fcxx-exceptions -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name trymacro.cpp %s | FileCheck %s
 
-// CHECK: Z3fn1v:
+// CHECK: Z3fn1v
 void fn1() try { return; } // CHECK: [[@LINE]]:12 -> [[@LINE+1]]:14 = #1
 catch(...) {}              // CHECK: [[@LINE]]:12 -> [[@LINE]]:14 = #2
 
 #define RETURN_BLOCK { return; }
 
-// CHECK: Z3fn2v:
+// CHECK: Z3fn2v
 void fn2() try RETURN_BLOCK // CHECK: [[@LINE]]:12 -> [[@LINE+1]]:14 = #1
 catch(...) {}               // CHECK: [[@LINE]]:12 -> [[@LINE]]:14 = #2
 
 #define TRY try
 #define CATCH(x) catch (x)
 
-// CHECK: Z3fn3v:
+// CHECK: Z3fn3v
 void fn3() TRY { return; } // CHECK: [[@LINE]]:15 -> [[@LINE+1]]:14 = #1
 CATCH(...) {}              // CHECK: [[@LINE]]:12 -> [[@LINE]]:14 = #2
 
 int main() {
   fn1();
   fn2();
   fn3();
 }

test/Profile/Inputs/c-captured.proftext

 c-captured.c:__captured_stmt
 10
 2
 1
 1
 
 c-captured.c:__captured_stmt.1
 266
 3
 1
 10
 1
 
 main
-0
+17
 1
 1
 
 debug_captured
 650
 3
 1
 1
 1
 

test/Profile/Inputs/c-general.proftext

 16586
 4
 1
 10
 2
 8
 
 main
-0
+17
 1
 1
 
 c-general.c:static_func
 4
 2
 1
 10
 

test/Profile/Inputs/c-outdated-data.proftext

 no_usable_data
 650
 3
 1
 0
 0
 
 main
-0
+17
 1
 1
 

test/Profile/Inputs/c-unprofiled.proftext

 function_in_header
 10
 2
 1
 0
 
 main
-0
+17
 1
 1

test/Profile/Inputs/cxx-class.proftext

 _Z14simple_wrapperv
 4
 2
 1
 100
 
 main
-0
+17
 1
 1
 
 _ZN6SimpleD1Ev
 10
 2
 0
 0

test/Profile/Inputs/cxx-lambda.proftext

 cxx-lambda.cpp:_ZZ7lambdasvENK3$_0clEi
 654
 3
 10
 9
 9
 
 main
-0
+17
 1
 1
 
 _Z7lambdasv
 41226
 4
 1
 1
 10
 1
 

test/Profile/Inputs/cxx-rangefor.proftext

 _Z9range_forv
 0x000000000014a28a
 5
 1
 4
 1
 1
 1
 
 main
-0
+17
 1
 1

test/Profile/Inputs/cxx-templates.proftext

 main
-0
+17
 1
 1
 
 _Z4loopILj0EEvv
 4
 2
 1
 0
 
 _Z4loopILj100EEvv
 4
 2
 1
 100
 

test/Profile/Inputs/func-entry.proftext

 foo
-0
+17
 1
 1000
 
 main
 4
 2
 1
 10000

test/Profile/Inputs/max-function-count.proftext

 begin
 # Func Hash:
 10
 # Num Counters:
 2
 # Counter Values:
 1
 0
 
 main
 # Func Hash:
-0
+17
 # Num Counters:
 1
 # Counter Values:
 1
 
 end
 # Func Hash:
 10
 # Num Counters:
 2
 # Counter Values:
 2
 2
 

test/Profile/Inputs/objc-general.proftext

 objc-general.m:__13+[A foreach:]_block_invoke
 10
 2
 2
 1
 
 objc-general.m:+[A foreach:]
 6
 2
 1
 2
 
 main
-0
+17
 1
 1
 

test/Profile/Inputs/profile-summary.proftext

 begin
 # Func Hash:
 10
 # Num Counters:
 2
 # Counter Values:
 1
 0
 
 main
 # Func Hash:
-0
+17
 # Num Counters:
 1
 # Counter Values:
 1
 
 end
 # Func Hash:
 10
 # Num Counters:
 2
 # Counter Values:
 2
 2