Page MenuHomePhabricator

Add support for MC/DC in LLVM Source-Based Code Coverage
Needs ReviewPublic

Authored by alanphipps on Oct 20 2022, 3:15 PM.

Details

Reviewers
vsk
phosek
Summary

I previously upstreamed work to enable Branch Coverage (https://reviews.llvm.org/D84467), which was pushed in early 2021. MC/DC (Modified Condition/Decision Coverage) is a planned enhancement to Source-based Code Coverage. Implementation was completed in May for our downstream Arm compiler, and in general use it has not yielded any issues. Our downstream implementation does not use most of the compiler-rt profile functionality, and so that is relatively new to this patch.

See attached file for Background, Motivation, Design, and Implementation Concepts:

I am also available at Discord @alanphipps

Recommended order for the review. Start with the front-end changes:

  • New Option and common parameters
    • CodeGenOptions.def
    • Options.td
    • Clang.cpp
  • Bitmask Coverage Object Allocation:
    • CodeGenPGO.h, CodeGenPGO.cpp
  • Source Region Mapping:
    • CoverageMappingGen.h, CoverageMappingGen.cpp
  • Instrumentation (during IR lowering):
    • CGExprScalar.cpp
    • CGStmt.cpp
    • CodeGenFunction.h, CodeGenFunction.cpp

LLVM back-end processing and profile reading/writing:

  • Instrumentation intrinsic lowering and section allocation:
    • IntrinsicInst.h
    • Intrinsics.td
    • IntrinsicInst.cpp
    • InstrProfiling.h, InstrProfiling.cpp
  • Profiling format read/write:
    • CoverageMappingWriter.cpp
    • CoverageMappingReader.cpp
  • Profile read/write:
    • InstrProf.h, InstrProf.cpp
    • InstrProfWriter.h, InstrProfWriter.cpp
    • InstrProfReader.h, InstrProfReader.cpp
  • Runtime profiling aids:
    • InstrProfiling.h
    • InstrProfilingFilebaremetal.c
    • InstrProfilingPlatformBaremetal.c
    • compiler-rt

Visualization and Evaluation:

  • Bitmask evaluation and MC/DC Region Analysis:
    • CoverageMapping.h, CoverageMapping.cpp
  • Creation of MC/DC Views with Records:
    • CodeCoverage.cpp
    • CoverageViewOptions.h
  • Text, HTML Visualization:
    • SourceCoverageView.h, SourceCoverageView.cpp
    • SourceCoverageViewHTML.h, SourceCoverageViewHTML.cpp
    • SourceCoverageViewText.h, SourceCoverageViewText.cpp
  • Summary Report Generation:
    • CoverageSummaryInfo.h, CoverageSummaryInfo.cpp
    • CoverageReport.cpp
  • JSON exporting:
    • CoverageExporterJson.cpp

This patch has been tested on windows (stage1) and linux/mac (stage1 and stage2)

Unsupported at the moment (please consider enabling in a future patch!):

  • Fuchsia
  • Debug Correlation

Diff Detail

Event Timeline

alanphipps created this revision.Oct 20 2022, 3:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 3:15 PM
alanphipps requested review of this revision.Oct 20 2022, 3:15 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptOct 20 2022, 3:15 PM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits and 2 others. · View Herald Transcript
alanphipps edited the summary of this revision. (Show Details)Oct 20 2022, 3:23 PM
alanphipps edited the summary of this revision. (Show Details)Oct 20 2022, 3:27 PM
alanphipps edited the summary of this revision. (Show Details)Oct 21 2022, 1:13 PM
alanphipps added a subscriber: peter.smith.
ellis added a subscriber: ellis.Oct 24 2022, 4:48 PM
ellis added inline comments.
llvm/lib/ProfileData/InstrProfCorrelator.cpp
208

Since BitmaskOffset and NumBitmaskBytes are always zero, I would rather set them as zero here rather than passing in more parameters. See ValuesPtr.

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.

clang/lib/CodeGen/CodeGenFunction.h
1522

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.

1542

Can this be bool isMCDCCoverageEnabled() const

1554

Is the return necessary here?

1558

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.

clang/lib/CodeGen/CodeGenPGO.cpp
164

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).

979

unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? CGM.getCodeGenOpts().MCDCMaxConditionCount : 0;

Purely subjective opinion on my part though.

clang/lib/CodeGen/CoverageMappingGen.cpp
102

Worth a comment that these are associated with MCDC?

662

Could this be a free function? If not then could it be const?

718

Would it be better to invert the condition and early exit? This would save a level of indentation for the rest of the function.

759

Would it be better to invert the condition and early exit? This would save a level of indentation for the rest of the function.

clang/lib/Driver/ToolChains/Clang.cpp
924

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.

Thank you (both) for looking at it! I appreciate your time.

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.

I agree -- I will explore splitting the review up, probably next week. And I'll review the suggested adjustments.

alanphipps edited the summary of this revision. (Show Details)Oct 28 2022, 9:25 AM

Thank you for the comments thus-far. I have split this patch into three stacked patches and updated them according to comments found on this review:

Clang Front-end: https://reviews.llvm.org/D138849
LLVM back-end/compiler-rt: https://reviews.llvm.org/D138846
llvm-cov visualization/evaluation: https://reviews.llvm.org/D138847