From 13035b230fd51422f6c3223fcffab4f44bd00956 Mon Sep 17 00:00:00 2001 From: NAKAMURA Takumi Date: Fri, 19 Apr 2024 15:26:34 +0900 Subject: [PATCH 1/4] [MC/DC][Coverage] Add assertions into emitSourceRegions() `emitSourceRegions()` has bugs to emit malformed MC/DC coverage mappings. They were detected in `llvm-cov` as the crash. Detect inconsistencies earlier in `clang` with assertions. * mcdc-system-headers.cpp covers #78920. * mcdc-scratch-space.c covers #87000. --- clang/lib/CodeGen/CoverageMappingGen.cpp | 29 ++++++++++-- .../test/CoverageMapping/mcdc-scratch-space.c | 27 +++++++++++ .../CoverageMapping/mcdc-system-headers.cpp | 47 +++++++++++++++++++ 3 files changed, 98 insertions(+), 5 deletions(-) create mode 100644 clang/test/CoverageMapping/mcdc-scratch-space.c create mode 100644 clang/test/CoverageMapping/mcdc-system-headers.cpp diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index 71215da362d3d0..95ad1967c8b673 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -190,11 +190,21 @@ class SourceMappingRegion { bool isBranch() const { return FalseCount.has_value(); } + bool isMCDCBranch() const { + const auto *BranchParams = std::get_if(&MCDCParams); + assert(BranchParams == nullptr || BranchParams->ID >= 0); + return (BranchParams != nullptr); + } + + const auto &getMCDCBranchParams() const { + return mcdc::getParams(MCDCParams); + } + bool isMCDCDecision() const { const auto *DecisionParams = std::get_if(&MCDCParams); - assert(!DecisionParams || DecisionParams->NumConditions > 0); - return DecisionParams; + assert(DecisionParams == nullptr || DecisionParams->NumConditions > 0); + return (DecisionParams != nullptr); } const auto &getMCDCDecisionParams() const { @@ -464,13 +474,19 @@ class CoverageMappingBuilder { // Ignore regions from system headers unless collecting coverage from // system headers is explicitly enabled. if (!SystemHeadersCoverage && - SM.isInSystemHeader(SM.getSpellingLoc(LocStart))) + SM.isInSystemHeader(SM.getSpellingLoc(LocStart))) { + assert(!Region.isMCDCBranch() && !Region.isMCDCDecision() && + "Don't suppress the condition in system headers"); continue; + } auto CovFileID = getCoverageFileID(LocStart); // Ignore regions that don't have a file, such as builtin macros. - if (!CovFileID) + if (!CovFileID) { + assert(!Region.isMCDCBranch() && !Region.isMCDCDecision() && + "Don't suppress the condition in non-file regions"); continue; + } SourceLocation LocEnd = Region.getEndLoc(); assert(SM.isWrittenInSameFile(LocStart, LocEnd) && @@ -480,8 +496,11 @@ class CoverageMappingBuilder { // This not only suppresses redundant regions, but sometimes prevents // creating regions with wrong counters if, for example, a statement's // body ends at the end of a nested macro. - if (Filter.count(std::make_pair(LocStart, LocEnd))) + if (Filter.count(std::make_pair(LocStart, LocEnd))) { + assert(!Region.isMCDCBranch() && !Region.isMCDCDecision() && + "Don't suppress the condition"); continue; + } // Find the spelling locations for the mapping region. SpellingRegion SR{SM, LocStart, LocEnd}; diff --git a/clang/test/CoverageMapping/mcdc-scratch-space.c b/clang/test/CoverageMapping/mcdc-scratch-space.c new file mode 100644 index 00000000000000..962d10653a028b --- /dev/null +++ b/clang/test/CoverageMapping/mcdc-scratch-space.c @@ -0,0 +1,27 @@ +// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c99 -fcoverage-mcdc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s +// XFAIL: * +// REQUIRES: asserts + +int builtin_macro0(int a) { + return (__LINE__ + && a); +} + +int builtin_macro1(int a) { + return (a + || __LINE__); +} + +#define PRE(x) pre_##x + +int pre0(int pre_a, int b_post) { + return (PRE(a) + && b_post); +} + +#define POST(x) x##_post + +int post0(int pre_a, int b_post) { + return (pre_a + || POST(b)); +} diff --git a/clang/test/CoverageMapping/mcdc-system-headers.cpp b/clang/test/CoverageMapping/mcdc-system-headers.cpp new file mode 100644 index 00000000000000..329bb37822a9ea --- /dev/null +++ b/clang/test/CoverageMapping/mcdc-system-headers.cpp @@ -0,0 +1,47 @@ +// RUN: %clang_cc1 -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -fcoverage-mcdc -mllvm -system-headers-coverage -emit-llvm-only -o - %s | FileCheck %s + +// Will crash w/o -system-headers-coverage +// RUN: not --crash %clang_cc1 -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -fcoverage-mcdc -emit-llvm-only -o - %s +// REQUIRES: asserts + +#ifdef IS_SYSHEADER + +#pragma clang system_header +#define CONST 42 +#define EXPR1(x) (x) +#define EXPR2(x) ((x) * (x)) + +#else + +#define IS_SYSHEADER +#include __FILE__ + +// CHECK: _Z5func0i: +int func0(int a) { + // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:21 = M:0, C:2 + // CHECK: Expansion,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:16 = #0 (Expanded file = 1) + return (CONST && a); + // CHECK: Branch,File 0, [[@LINE-1]]:20 -> [[@LINE-1]]:21 = #2, (#1 - #2) [2,0,0] + // CHECK: Branch,File 1, [[@LINE-15]]:15 -> [[@LINE-15]]:17 = 0, 0 [1,2,0] +} + +// CHECK: _Z5func1ii: +int func1(int a, int b) { + // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:21 = M:0, C:2 + // CHECK: Branch,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:12 = (#0 - #1), #1 [1,0,2] + return (a || EXPR1(b)); + // CHECK: Expansion,File 0, [[@LINE-1]]:16 -> [[@LINE-1]]:21 = #1 (Expanded file = 1) + // CHECK: Branch,File 1, [[@LINE-23]]:18 -> [[@LINE-23]]:21 = (#1 - #2), #2 [2,0,0] +} + +// CHECK: _Z5func2ii: +int func2(int a, int b) { + // Decision,File 0, [[@LINE+3]]:11 -> [[@LINE+3]]:28 = M:0, C:2 + // Expansion,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:16 = #0 (Expanded file = 1) + // Expansion,File 0, [[@LINE+1]]:23 -> [[@LINE+1]]:28 = #1 (Expanded file = 2) + return (EXPR2(a) && EXPR1(a)); + // CHECK: Branch,File 1, [[@LINE-31]]:18 -> [[@LINE-31]]:29 = #1, (#0 - #1) [1,2,0] + // CHECK: Branch,File 2, [[@LINE-33]]:18 -> [[@LINE-33]]:21 = #2, (#1 - #2) [2,0,0] +} + +#endif From 27404e7caef3e5504964e47a65fd983d32c04f69 Mon Sep 17 00:00:00 2001 From: NAKAMURA Takumi Date: Mon, 22 Apr 2024 15:38:14 +0900 Subject: [PATCH 2/4] Rewind isMCDCDecision() for now --- clang/lib/CodeGen/CoverageMappingGen.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index 95ad1967c8b673..bc9923af731999 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -203,8 +203,8 @@ class SourceMappingRegion { bool isMCDCDecision() const { const auto *DecisionParams = std::get_if(&MCDCParams); - assert(DecisionParams == nullptr || DecisionParams->NumConditions > 0); - return (DecisionParams != nullptr); + assert(!DecisionParams || DecisionParams->NumConditions > 0); + return DecisionParams; } const auto &getMCDCDecisionParams() const { From 9c6966accecddad28e4900680e28c6675da3f3fb Mon Sep 17 00:00:00 2001 From: NAKAMURA Takumi Date: Mon, 20 May 2024 19:31:23 +0900 Subject: [PATCH 3/4] Prune getMCDCBranchParams() for now --- clang/lib/CodeGen/CoverageMappingGen.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index a2570ad356b14f..0d16d00617c280 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -197,10 +197,6 @@ class SourceMappingRegion { return (BranchParams != nullptr); } - const auto &getMCDCBranchParams() const { - return mcdc::getParams(MCDCParams); - } - bool isMCDCDecision() const { return std::holds_alternative(MCDCParams); } From 0edc053c4cb0e016e643d6763701ebc52018496c Mon Sep 17 00:00:00 2001 From: NAKAMURA Takumi Date: Mon, 20 May 2024 19:33:52 +0900 Subject: [PATCH 4/4] Update isMCDCBranch() --- clang/lib/CodeGen/CoverageMappingGen.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index 0d16d00617c280..993d7cc7e21fab 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -192,9 +192,7 @@ class SourceMappingRegion { bool isBranch() const { return FalseCount.has_value(); } bool isMCDCBranch() const { - const auto *BranchParams = std::get_if(&MCDCParams); - assert(BranchParams == nullptr || BranchParams->ID >= 0); - return (BranchParams != nullptr); + return std::holds_alternative(MCDCParams); } bool isMCDCDecision() const {