Comments (10)
Does that sound OK?
Yes that sounds good. No need to keep condition AND branch coverage available to the user in the long term, as it will likely create confusion.
from rust.
@rustbot label +A-code-coverage
from rust.
There's a similar concern for some other boolean expressions that don't directly participate in a branch. For example:
// Non-lazy logical operators:
a | b
a & b
// Branch hidden in a standard-library function:
b.then_some(x)
So any mode that tries to instrument boolean expressions probably needs to consider whether/how to handle cases like these, too.
from rust.
In terms of implementation, I think the easiest would be to make
condition
implybranch
coverage, and treat the special case only whencondition
is enabled.
When I originally designed -Zcoverage-options
, I imagined that it would mostly consist of individual boolean flags.
But with branch coverage, I think what we're seeing is closer to an enum-valued setting with multiple levels, e.g.:
branch=off
branch=basic
(alias:branch
,branch=on
)branch=condition
(alias:condition
)branch=mcdc
(alias:mcdc
)
That might relieve the awkwardness of having so many “independent” flags that actually do depend on each other.
from rust.
Based on #124217 we could accept it in mcdc first as they are required on the same occasion mostly.
If we do that we probably get different results of llvm-cov --show-branches=count
between --coverage-options=branch
and --coverage-options=mcdc
.
What's more, since LLVM also produces branch coverage reports with MappingKind::MCDCBranch
, we can view --coverage-options=mcdc
as another "branch coverage" and make them mutually exclusive temporarily.
from rust.
I am unsure about how this should be implemented.
Shall we
- insert an actual "useless" branch and instrument it no matter what;
- or maybe be we could read the assigned result and update the corresponding counter/condition bitmap for MCDC.
Solution 1. is probably the simplest to implement, at the cost of small runtime bloat.
Solution 2. may involve harder implementations of the next steps.
I would rather go with solution 1, because again, slowdown is expected when instrumenting for code coverage
from rust.
In terms of implementation, I think the easiest would be to make
condition
implybranch
coverage, and treat the special case only whencondition
is enabled.When I originally designed
-Zcoverage-options
, I imagined that it would mostly consist of individual boolean flags.But with branch coverage, I think what we're seeing is closer to an enum-valued setting with multiple levels, e.g.:
branch=off
branch=basic
(alias:branch
,branch=on
)branch=condition
(alias:condition
)branch=mcdc
(alias:mcdc
)That might relieve the awkwardness of having so many “independent” flags that actually do depend on each other.
Yes, we definitely need a better way to organize all the options.
However, I am not convinced by the name branch=*
, as MC/DC and Branch coverage are clearly different things.
Maybe we could use -Z coverage-level=(statement (default) | branch | condition | mcdc)
, and -Z coverage-options=
could be used for things like no-pattern-matching
from rust.
I still don't understand the motivation for wanting to instrument the RHS of lazy boolean operators specifically, but then not also instrument other boolean-valued expressions.
Because if instrumenting all booleans is the goal, then I don't think focusing on lazy booleans specifically is a good intermediate step.
from rust.
I still don't understand the motivation for wanting to instrument the RHS of lazy boolean operators specifically, but then not also instrument other boolean-valued expressions.
Because if instrumenting all booleans is the goal, then I don't think focusing on lazy booleans specifically is a good intermediate step.
Condition coverage would really only be meant as a building block for MC/DC.
The MC/DC criterion only makes sense for boolean expressions with at least two operands, so tracking branch coverage for simple boolean expressions isn't really relevant.
We do need to track the valuation of all operands, however, as otherwise it would be too easy to cheese the criteria (even for branch coverage). Instead of requiring 4 tests to show full MC/DC coverage (and same for branch coverage, due to the short circuit operators) for
if (a && b && c) { ... }
We could rewrite this as `
let tmp1 = a && b;
let tmp2 = tmp1 && c;
if (tmp2) { ... }
Then, by only testing with a == True
and a == False
, all other variables being True
we'd get 100% branch coverage.
The reason why we don't want to instrument all booleans (including simple assignements like let x: bool = a
) is because it produces an overwhelming quantity of coverage information which is not very relevant for certification contexts
For example:
fn foo(a: bool) {
let x = a; // 2. check
if x { // 3. check
...
}
}
fn main() {
let res = foo(true); // 1. check
}
In this example. if we were to instrument all boolean values, we would end up with 3 tests for the exact same value, when the 3rd one would be enough.
Regarding the instrumentation of short-circuit operators only, there are seemingly 2 reasons for that:
llvm-cov
builds the expressions BDD (binary decision diagram) from the mappings present in the binary. From what I understood, it assumes that operators are short-circuits, and will consider short-circuited conditions asDONT_CARE
when performing analysis, no matter what the actual operators are. I am not sure how this can interfere with the actual correctness of the analysis.- The MC/DC criterion is easier to fulfill for short-circuited expressions, for the
DONT_CARE
reason. Also, legacy MCDC analysis solutions (including gnatcov) were based on tracing the branches in assembly at runtime, thus it was only possible to track the execution of short-circuit operands. Also, these solutions needed a-fpreserve-control-flow
flag to be passed to the compiler to instrument the last conditions as well.
from rust.
Condition coverage would really only be meant as a building block for MC/DC.
I've been thinking some more about whether we should have a separate condition
mode at all (instead of just having it be a part of mcdc
), and I think my conclusion for now is that I'm reluctantly OK with it.
My main concern is that I want to avoid a proliferation of modes that complicate and weaken testing, especially if they'll be contentious to remove later on.
On the other hand, I do recognise the implementation benefits of having a separate tier on a temporary basis, so that it can be developed and tested separately from the full complexity of MC/DC, without affecting the main branch
coverage mode.
So I'd like to suggest that we primarily think of the condition
mode as a temporary implementation detail that will eventually go away once the MC/DC mode is more mature, unless there turns out to be real end-user demand for it.
Does that sound OK?
from rust.
Related Issues (20)
- [const_closures, const_mut_refs, effects] thread 'rustc' panicked at rustc_middle/src/ty/util.rs:945:22: ConstContext::Maybe must have host effect param HOT 1
- Invalid Program with `tokio::fs::File` creates Type Dependency Cycle (E0391)
- Allow moving out of `&'static mut T` HOT 17
- `alloc::System` docs contradict "the default global allocator is unspecified"
- ICE: `cannot convert ReLateParam to a region vid` HOT 4
- ICE: `Encountered unexpected type in ConstructorSet::for_ty` in `rustc_pattern_analysis/src/rustc.rs` HOT 1
- `std::env::remove_var()` becomes `unsafe` even in Rust 2021 HOT 13
- ICE: `Result::unwrap()` on an `Err` value: `NoMatch(…)` HOT 3
- ICE: effects: expected region for `'x/#1` (`'x/#1/1`) but found `Type(())` when instantiating args=`[false, ()]` HOT 1
- Inaccurate recognition of lifetime HOT 2
- ICE: `unimplemented: inhabitedness checking for inherent projections` in `rustc_middle/src/ty/inhabitedness/mod.rs` HOT 1
- ICE: `upvar_tys called before capture types are inferred` in `rustc_type_ir/src/ty_kind/closure.rs` HOT 7
- Tracking Issue for `integer_sign_cast` HOT 6
- ICE: Should not be resolving bound region HOT 3
- ICE: icnr comp: type variable should not be hashed
- ICE: Unexpected type for constructor `Variant(1)` HOT 3
- Generic `(&A -> T) -> (&'a A -> Box<dyn Any + 'a>)` crashes HOT 2
- Support for 'split-debuginfo' on wasm targets HOT 1
- compiletest: more consistent handling of bool-like env vars
- ICE: panic during codegen/LLVM phase HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rust.