[PATCH] D138847: MC/DC in LLVM Source-Based Code Coverage: llvm-cov visualization

[Alan Phipps via Phabricator via llvm-commits]

alanphipps added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:37
 #include <memory>
+#include <sstream>
 #include <string>
----------------
MaskRay wrote:
> This is an uncommon include in LLVM. We usually use format, formatv, etc.
I get that it's uncommon, though there appears to be some precedent, plus it's very useful. The Coding Standards [[ https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden | seem to suggest  ]] `<sstream>` is OK (just not `<iostream>`).


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:450
+    unsigned Covered = 0;
+    for (unsigned C = 0; C < getNumConditions(); C++) {
+      if (isCondFolded(C))
----------------
paquette wrote:
> Would it be valid for `getNumConditions` to ever be 0? Would it make sense to add an assert somewhere if it doesn't make sense?
Yep, added an assertion inside getNumConditions() itself.


================
Comment at: llvm/test/tools/llvm-cov/mcdc-const.test:38
+// RUN: llvm-profdata merge %S/Inputs/mcdc-const-folding.proftext -o %t.profdata
+// RUN: llvm-cov show --show-mcdc %S/Inputs/mcdc-const-folding.o -instr-profile %t.profdata -path-equivalence=/tmp,%S/Inputs %S/Inputs/mcdc-const-folding.cpp | FileCheck %s -check-prefix=CHECKFULLCASE
+// RUN: llvm-cov report --show-mcdc-summary %S/Inputs/mcdc-const-folding.o -instr-profile %t.profdata -show-functions -path-equivalence=/tmp,%S/Inputs %S/Inputs/mcdc-const-folding.cpp | FileCheck %s -check-prefix=REPORT
----------------
MaskRay wrote:
> I am also concerned of prebuilt relocatable object files checked into the repository.
> They are target-specific, opaque, and difficult to update.
> 
> I am well aware of https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer
> but in this case perhaps we should use compiler-rt/test/profile
Ah I see -- you're suggesting using compiler-rt/test/profile so that object files can be generated directly as input to these tools.  I think that's a good idea.  I'd like to address that in a subsequent PR since this particular patch doesn't include the changes that enable MC/DC in clang.

I understand the concern about adding prebuilt object files to the repo. However, I really like the testing granularity it provides. I've added the steps to reproduce them.  But if there is a strong desire, perhaps I can remove them in the subsequent PR and keep them in our downstream repo only (where it's easier to test our baremetal toolchain).


================
Comment at: llvm/tools/llvm-cov/CoverageSummaryInfo.h:148
+  /// The number of Independence Pairs that were covered.
+  size_t CoveredPairs;
+
----------------
paquette wrote:
> init to 0?
The pattern across all of the *Info classes in this file appears to be to set the member vars on construction. I'm OK with changing it if it's important.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138847/new/

https://reviews.llvm.org/D138847