Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Replace boolean options with a CoverageLevel enum #124507

Merged
merged 1 commit into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ use rustc_data_structures::profiling::TimePassesFormat;
use rustc_errors::{emitter::HumanReadableErrorType, registry, ColorConfig};
use rustc_session::config::{
build_configuration, build_session_options, rustc_optgroups, BranchProtection, CFGuard, Cfg,
CollapseMacroDebuginfo, CoverageOptions, DebugInfo, DumpMonoStatsFormat, ErrorOutputType,
ExternEntry, ExternLocation, Externs, FunctionReturn, InliningThreshold, Input,
InstrumentCoverage, InstrumentXRay, LinkSelfContained, LinkerPluginLto, LocationDetail, LtoCli,
NextSolverConfig, OomStrategy, Options, OutFileName, OutputType, OutputTypes, PAuthKey, PacRet,
Passes, Polonius, ProcMacroExecutionStrategy, Strip, SwitchWithOptPath, SymbolManglingVersion,
WasiExecModel,
CollapseMacroDebuginfo, CoverageLevel, CoverageOptions, DebugInfo, DumpMonoStatsFormat,
ErrorOutputType, ExternEntry, ExternLocation, Externs, FunctionReturn, InliningThreshold,
Input, InstrumentCoverage, InstrumentXRay, LinkSelfContained, LinkerPluginLto, LocationDetail,
LtoCli, NextSolverConfig, OomStrategy, Options, OutFileName, OutputType, OutputTypes, PAuthKey,
PacRet, Passes, Polonius, ProcMacroExecutionStrategy, Strip, SwitchWithOptPath,
SymbolManglingVersion, WasiExecModel,
};
use rustc_session::lint::Level;
use rustc_session::search_paths::SearchPath;
Expand Down Expand Up @@ -761,7 +761,7 @@ fn test_unstable_options_tracking_hash() {
})
);
tracked!(codegen_backend, Some("abc".to_string()));
tracked!(coverage_options, CoverageOptions { branch: true, mcdc: true });
tracked!(coverage_options, CoverageOptions { level: CoverageLevel::Mcdc });
tracked!(crate_attr, vec!["abc".to_string()]);
tracked!(cross_crate_inline_threshold, InliningThreshold::Always);
tracked!(debug_info_for_profiling, true);
Expand Down
24 changes: 20 additions & 4 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,26 @@ pub enum InstrumentCoverage {
/// Individual flag values controlled by `-Z coverage-options`.
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Default)]
pub struct CoverageOptions {
/// Add branch coverage instrumentation.
pub branch: bool,
/// Add mcdc coverage instrumentation.
pub mcdc: bool,
pub level: CoverageLevel,
// Other boolean or enum-valued options might be added here.
}

/// Controls whether branch coverage or MC/DC coverage is enabled.
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
pub enum CoverageLevel {
/// Instrument for coverage at the MIR block level.
Block,
/// Also instrument branch points (includes block coverage).
Branch,
/// Instrument for MC/DC. Mostly a superset of branch coverage, but might
/// differ in some corner cases.
Mcdc,
}

impl Default for CoverageLevel {
fn default() -> Self {
Self::Block
}
}

/// Settings for `-Z instrument-xray` flag.
Expand Down
14 changes: 4 additions & 10 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ mod desc {
pub const parse_optimization_fuel: &str = "crate=integer";
pub const parse_dump_mono_stats: &str = "`markdown` (default) or `json`";
pub const parse_instrument_coverage: &str = parse_bool;
pub const parse_coverage_options: &str = "either `no-branch`, `branch` or `mcdc`";
pub const parse_coverage_options: &str = "`block` | `branch` | `mcdc`";
pub const parse_instrument_xray: &str = "either a boolean (`yes`, `no`, `on`, `off`, etc), or a comma separated list of settings: `always` or `never` (mutually exclusive), `ignore-loops`, `instruction-threshold=N`, `skip-entry`, `skip-exit`";
pub const parse_unpretty: &str = "`string` or `string=string`";
pub const parse_treat_err_as_bug: &str = "either no value or a non-negative number";
Expand Down Expand Up @@ -946,15 +946,9 @@ mod parse {

for option in v.split(',') {
match option {
"no-branch" => {
slot.branch = false;
slot.mcdc = false;
}
"branch" => slot.branch = true,
"mcdc" => {
slot.branch = true;
slot.mcdc = true;
}
"block" => slot.level = CoverageLevel::Block,
"branch" => slot.level = CoverageLevel::Branch,
"mcdc" => slot.level = CoverageLevel::Mcdc,
_ => return false,
}
}
Comment on lines 947 to 954
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we keep the for loop for future changes ? Otherwise, I think we should get rid of it, as it wouldn't make sense to give -Z coverage-options=mcdc,block to the compiler

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the loop.

It's true that there are no other independent options at the moment, but I don't think there's much benefit in removing it now just to have to add it back later when adding another option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Some examples of possible future options: #124144 (comment), #120013, and perhaps an option to not expand/emit zero-width spans.)

Expand Down
10 changes: 6 additions & 4 deletions compiler/rustc_session/src/session.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::code_stats::CodeStats;
pub use crate::code_stats::{DataTypeKind, FieldInfo, FieldKind, SizeKind, VariantInfo};
use crate::config::{
self, CrateType, FunctionReturn, InstrumentCoverage, OptLevel, OutFileName, OutputType,
RemapPathScopeComponents, SwitchWithOptPath,
self, CoverageLevel, CrateType, FunctionReturn, InstrumentCoverage, OptLevel, OutFileName,
OutputType, RemapPathScopeComponents, SwitchWithOptPath,
};
use crate::config::{ErrorOutputType, Input};
use crate::errors;
Expand Down Expand Up @@ -349,11 +349,13 @@ impl Session {
}

pub fn instrument_coverage_branch(&self) -> bool {
self.instrument_coverage() && self.opts.unstable_opts.coverage_options.branch
self.instrument_coverage()
&& self.opts.unstable_opts.coverage_options.level >= CoverageLevel::Branch
}

pub fn instrument_coverage_mcdc(&self) -> bool {
self.instrument_coverage() && self.opts.unstable_opts.coverage_options.mcdc
self.instrument_coverage()
&& self.opts.unstable_opts.coverage_options.level >= CoverageLevel::Mcdc
}

pub fn is_sanitizer_cfi_enabled(&self) -> bool {
Expand Down
6 changes: 1 addition & 5 deletions src/doc/rustc/src/instrument-coverage.md
Original file line number Diff line number Diff line change
Expand Up @@ -348,11 +348,7 @@ $ llvm-cov report \

## `-Z coverage-options=<options>`

This unstable option provides finer control over some aspects of coverage
instrumentation. Pass one or more of the following values, separated by commas.

- Either `no-branch`, `branch` or `mcdc`
- `branch` enables branch coverage instrumentation and `mcdc` further enables modified condition/decision coverage instrumentation. `no-branch` disables branch coverage instrumentation, which is same as do not pass `branch` or `mcdc`.
[This unstable option is described in the Unstable Book.](../unstable-book/compiler-flags/coverage-options.html)

## Other references

Expand Down
11 changes: 10 additions & 1 deletion src/doc/unstable-book/src/compiler-flags/coverage-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,13 @@ This option controls details of the coverage instrumentation performed by

Multiple options can be passed, separated by commas. Valid options are:

- `no-branch`, `branch` or `mcdc`: `branch` enables branch coverage instrumentation and `mcdc` further enables modified condition/decision coverage instrumentation. `no-branch` disables branch coverage instrumentation as well as mcdc instrumentation, which is same as do not pass `branch` or `mcdc`.
- `block`, `branch`, `mcdc`:
Sets the level of coverage instrumentation.
Setting the level will override any previously-specified level.
- `block` (default):
Blocks in the control-flow graph will be instrumented for coverage.
- `branch`:
In addition to block coverage, also enables branch coverage instrumentation.
- `mcdc`:
In addition to block and branch coverage, also enables MC/DC instrumentation.
(Branch coverage instrumentation may differ in some cases.)
2 changes: 1 addition & 1 deletion tests/ui/instrument-coverage/coverage-options.bad.stderr
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
error: incorrect value `bad` for unstable option `coverage-options` - either `no-branch`, `branch` or `mcdc` was expected
error: incorrect value `bad` for unstable option `coverage-options` - `block` | `branch` | `mcdc` was expected

11 changes: 4 additions & 7 deletions tests/ui/instrument-coverage/coverage-options.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
//@ needs-profiler-support
//@ revisions: branch no-branch bad
//@ revisions: block branch bad
//@ compile-flags -Cinstrument-coverage

//@ [block] check-pass
//@ [block] compile-flags: -Zcoverage-options=block

//@ [branch] check-pass
//@ [branch] compile-flags: -Zcoverage-options=branch

//@ [no-branch] check-pass
//@ [no-branch] compile-flags: -Zcoverage-options=no-branch

//@ [mcdc] check-pass
//@ [mcdc] compile-flags: -Zcoverage-options=mcdc

//@ [bad] check-fail
//@ [bad] compile-flags: -Zcoverage-options=bad

//@ [conflict] check-fail
//@ [conflict] compile-flags: -Zcoverage-options=no-branch,mcdc

fn main() {}