[PATCH] D57982: [SanitizierCoverage] Avoid splitting critical edges when destination is a basic block containing unreachable

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D57982#1391344 <https://reviews.llvm.org/D57982#1391344>, @craig.topper wrote:

> Should this code from SanitizerCoverage.cpp be checking that the first non-debug instruction is an unreachable instead of the terminator?
>
>   static bool shouldInstrumentBlock(const Function &F, const BasicBlock *BB,
>                                     const DominatorTree *DT,
>                                     const PostDominatorTree *PDT,
>                                     const SanitizerCoverageOptions &Options) {
>     // Don't insert coverage for unreachable blocks: we will never call
>     // __sanitizer_cov() for them, so counting them in
>     // NumberOfInstrumentedBlocks() might complicate calculation of code coverage
>     // percentage. Also, unreachable instructions frequently have no debug
>     // locations.
>     if (isa<UnreachableInst>(BB->getTerminator()))
>       return false;
>


Here's how I interpret the comment. I don't believe the "we will never call `__sanitizer_cov` for them". Maybe I just don't understand when `__sanitizer_cov` would be called, so that could be my lack of understanding. But, I think the second portion indicates that this is *intended* to avoid instrumenting fatal error patterns typically produced by macros like assert. Sanitizer coverage mostly exists to guide fuzzers, and I think, in the context of a codebase that does not use exceptions, i.e. most existing users (no value judgement), it's uninteresting to explore paths that lead to rejecting inputs with a fatal error. Of course, it's very interesting to explore longjmp and throw, which also create blocks that end in unreachable.

I think we'll need guidance from @kcc to know what needs to be done.


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

https://reviews.llvm.org/D57982