[PATCH] D136385: Add support for MC/DC in LLVM Source-Based Code Coverage

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've got about as far as the clang changes. As I mentioned in Discourse I'm not familiar enough in this area to give good feedback on the implementation, most if not all my comments fall under the bike shedding category. Will need to leave the high-level feedback to people that are more experienced in this area.

Thanks very much for uploading the design doc pdf, that was very useful. Although likely not for this patch as it is large enough already, will be worth updating https://llvm.org/docs/CoverageMappingFormat.html and https://clang.llvm.org/docs/SourceBasedCodeCoverage.html

I suspect that this patch will need to get split up into smaller parts. While it is very useful to see everything at once, it may be better to use this as a kind of support for the RFC, and then split off the implementation into smaller independently reviewable parts, even if the full functionality isn't useable until the last patch lands. For example the LLVM changes could be looked at and accepted prior to clang.

Will try and look and some more parts next week, my apologies I can't go through it all at once.



================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1522
+  /// Used by MC/DC to track the execution state of a boolean expression.
+  Address MCDCTempAddr = Address::invalid();
+
----------------
Would MCDCTrackingStateAddr be more appropriate? It could save people some time tracking back up to the comment to work out what Temp is being used for.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1542
 
+  bool isMCDCCoverageEnabled() {
+    return (CGM.getCodeGenOpts().hasProfileClangInstr() &&
----------------
Can this be `bool isMCDCCoverageEnabled() const`


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1554
+      MCDCTempAddr = CreateIRTemp(getContext().UnsignedIntTy, "mcdc.addr");
+      return;
+    }
----------------
Is the return necessary here?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1558
+
+  bool isBinaryLogicalOp(const Expr *S) {
+    const BinaryOperator *BOp = dyn_cast<BinaryOperator>(S->IgnoreParens());
----------------
This looks like it doesn't need to be a member function? I can appreciate that here may be the most convenient place to  put it though.


================
Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:164
+  /// The next bitmask byte index to assign.
+  unsigned NextBitmask;
+  /// The map of statements to bitmask coverage objects.
----------------
perhaps worth including index in the name, for example `NextBitmaskIdx` . Will help prevent confusion as to whether this is an index or an actual bitmask (comment does say, but harder to see from reading the code).


================
Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:979
+  // set it to zero.
+  unsigned MCDCMaxConditions = 0;
+  if (CGM.getCodeGenOpts().MCDCCoverage)
----------------
`unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? CGM.getCodeGenOpts().MCDCMaxConditionCount : 0;`

Purely subjective opinion on my part though.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:102
 
+  unsigned Mask = 0;
+  unsigned Conditions = 0;
----------------
Worth a comment that these are associated with MCDC?


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:662
+  /// Is this a logical-AND operation?
+  bool isLAnd(const BinaryOperator *E) { return E->getOpcode() == BO_LAnd; }
+
----------------
Could this be a free function? If not then could it be `const`? 


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:718
+  void pushAndAssignIDs(const BinaryOperator *E) {
+    if (CGM.getCodeGenOpts().MCDCCoverage) {
+      // If binary expression is disqualified, don't do mapping.
----------------
Would it be better to invert the condition and early exit? This would save a level of indentation for the rest of the function.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:759
+    unsigned TotalConds = 0;
+    if (CGM.getCodeGenOpts().MCDCCoverage) {
+      // Pop Stmt from 'NestLevel' stack.
----------------
Would it be better to invert the condition and early exit? This would save a level of indentation for the rest of the function.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:924
+                   false)) {
+    if (!CoverageMapping)
+      D.Diag(clang::diag::err_drv_argument_only_allowed_with)
----------------
Maybe able to simplify this to `if (Args.hasFlag(options::OPT_fcoverage_mapping)` and remove `CoverageMapping`. I think that in the case of `!ProfileGenerateArg` there are several options so it needed to be a variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136385