[Coverage][Windows] Null pointer dereference in CodeGenPGO::skipRegionMappingForDecl (fixes PR32761)

[adamf]

Summary

In function CodeGenPGO::skipRegionMappingForDecl there is possible NULL pointer dereference on line:
auto Loc = D->getBody()->getLocStart();
Value returned by getBody may be NULL.
In corresponding test it happens during processing the virtual destructor ~A.

(minor)
The variable SkipCoverageMapping in the same function is always false. We can remove it.

[vsk]

Thanks for your patch! I have a few requests.

First, please split off the SkipCoverageMapping change from this patch. If you don't have commit access yet, let me know and I can commit it for you. You can also ping Chris L for commit access.

Second, the test can be minimized a bit further. Here's what I got:

// RUN: %clang_cc1 -cc1 -triple i686-pc-windows-msvc19.0.0 -emit-obj -fprofile-instrument=clang -fdelayed-template-parsing -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name empty-destructor.cpp -o - %s
                                                                                                                                                                                                                                                                                                                              
struct A {                                                                                                                                                                                                                                                                                                                    
  virtual ~A();                                                                                                                                                                                                                                                                                                               
};                                                                                                                                                                                                                                                                                                                            
                                                                                                                                                                                                                                                                                                                              
void PR32761() {                                                                                                                                                                                                                                                                                                              
  A a;                                                                                                                                                                                                                                                                                                                        
}

[rnk]

In D32406#735725, @vsk wrote:

// RUN: %clang_cc1 -cc1 -triple i686-pc-windows-msvc19.0.0 -emit-obj -fprofile-instrument=clang -fdelayed-template-parsing -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name empty-destructor.cpp -o - %s

This is probably closer to a minimal command line:
%clang_cc1 -triple i686-windows -emit-llvm-only -fcoverage-mapping -dump-coverage-mapping %s

Is there something we can FileCheck in the IR?

[rnk]

In D32406#735725, @vsk wrote:

First, please split off the SkipCoverageMapping change from this patch. If you don't have commit access yet, let me know and I can commit it for you. You can also ping Chris L for commit access.

Are you sure you want to do that? This is the only use of this boolean.

[adamf]

Thanks for your comments!

Updated test case according to @vsk comment about test case reduction.

@rnk, your compilation command probably exposes another issue (with your compilation command the test still failure). I don't have time to analyze it now.

I don't have write access to svn so if you think this change is ok please commit it.

[vsk]

Are you sure you want to do that? This is the only use of this boolean.

IMO removing 'SkipCoverageMapping' isn't related to fixing the original bug. Since it's simple enough to make it a separate change, I thought it'd be best.

In D32406#735847, @rnk wrote:

In D32406#735725, @vsk wrote:

// RUN: %clang_cc1 -cc1 -triple i686-pc-windows-msvc19.0.0 -emit-obj -fprofile-instrument=clang -fdelayed-template-parsing -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name empty-destructor.cpp -o - %s

This is probably closer to a minimal command line:
%clang_cc1 -triple i686-windows -emit-llvm-only -fcoverage-mapping -dump-coverage-mapping %s

I'll go with this. @adamf this is just missing '-fprofile-instrument=clang', otherwise it's fine.

Is there something we can FileCheck in the IR?

Absolutely, I'll add in the checks.

cfe/trunk/lib/CodeGen/CodeGenPGO.cpp

void CodeGenPGO::mapRegionCounters(const Decl *D) {
   NumRegionCounters = Walker.NextCounter;
   FunctionHash = Walker.Hash.finalize();
 }
 
 bool CodeGenPGO::skipRegionMappingForDecl(const Decl *D) {
   if (SkipCoverageMapping)
     return true;
 
+  if (!D->getBody())
+    return true;
+
   // Don't map the functions in system headers.
   const auto &SM = CGM.getContext().getSourceManager();
   auto Loc = D->getBody()->getLocStart();
   return SM.isInSystemHeader(Loc);
 }
 
 void CodeGenPGO::emitCounterRegionMapping(const Decl *D) {
   if (skipRegionMappingForDecl(D))

cfe/trunk/test/CoverageMapping/empty-destructor.cpp

+// RUN: %clang_cc1 -triple i686-windows -emit-llvm-only -fcoverage-mapping -dump-coverage-mapping -fprofile-instrument=clang %s | FileCheck %s
+
+struct A {
+  virtual ~A();
+};
+
+// CHECK: ?PR32761@@YAXXZ:
+// CHECK-NEXT: File 0, [[@LINE+1]]:16 -> [[@LINE+3]]:2 = #0
+void PR32761() {
+  A a;
+}