[Coverage] Map regions from system headers

[ManuelvOK]

In 2155195, the "system-headers-coverage" option has been added but not used in all necessary places.

Potential reviewers: @gulfemsavrun @petrhosek

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (ManuelvOK)

Changes

In 2155195, the "system-headers-coverage" option has been added but not used in all necessary places.

---

Full diff: https://github.com/llvm/llvm-project/pull/76950.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenPGO.cpp (+3-1)
• (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+1-1)

diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 81bf8ea696b164..84fe6e705a409b 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -28,6 +28,8 @@ static llvm::cl::opt<bool>
                          llvm::cl::desc("Enable value profiling"),
                          llvm::cl::Hidden, llvm::cl::init(false));
 
+extern llvm::cl::opt<bool> SystemHeadersCoverage;
+
 using namespace clang;
 using namespace CodeGen;
 
@@ -885,7 +887,7 @@ bool CodeGenPGO::skipRegionMappingForDecl(const Decl *D) {
   // Don't map the functions in system headers.
   const auto &SM = CGM.getContext().getSourceManager();
   auto Loc = D->getBody()->getBeginLoc();
-  return SM.isInSystemHeader(Loc);
+  return !SystemHeadersCoverage && SM.isInSystemHeader(Loc);
 }
 
 void CodeGenPGO::emitCounterRegionMapping(const Decl *D) {
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 56411e2240e505..435cd9df0295bf 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -37,7 +37,7 @@ static llvm::cl::opt<bool> EmptyLineCommentCoverage(
                    "disable it on test)"),
     llvm::cl::init(true), llvm::cl::Hidden);
 
-static llvm::cl::opt<bool> SystemHeadersCoverage(
+llvm::cl::opt<bool> SystemHeadersCoverage(
     "system-headers-coverage",
     llvm::cl::desc("Enable collecting coverage from system headers"),
     llvm::cl::init(false), llvm::cl::Hidden);

[mhaehnel]

LGTM

Disclaimer: I am a co-worker of the change author and discussed the change also offline.

[ManuelvOK]

Thanks for the approval. I unfortunately don't have the credentials to merge this change. Can you do this, @petrhosek ?

[chapuni]

Seems this causes the crash with -fcoverage-mcdc -mllvm -system-headers-coverage. Investigating.
See also #78920.

[chapuni]

I haven't had a reduced testcase for it, though.

I expect it reproducible with -fcoverage-mcdc -mllvm -system-headers-coverage to build LLVMSupport. clangCodeGen will crash. (llvm-cov crashes w/o system-headers-coverage)

[chapuni]

Excuse me, I've reverted this.

I posted a reduced testcase in #78920. Reproducible w/o -fcoverage-mcdc.

[mhaehnel]

Your revert just masks the underlying issue. What you do now is that you can specify -system-headers-coverage but the compiler will not actually provide coverage data for system headers because the flag is ignored. So specifying the switch becomes meaningless.

[chapuni]

This works for me if #78033 is fixed or reverted.

[chapuni]

I have restored this since our issue has been resolved in #80292.