Skip to content

Commit

Permalink
Implement validation canary
Browse files Browse the repository at this point in the history
  • Loading branch information
cwfitzgerald authored and kvark committed Mar 1, 2022
1 parent 3841353 commit 4d7f6eb
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 19 deletions.
4 changes: 2 additions & 2 deletions wgpu-hal/src/dx12/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ unsafe extern "system" fn output_debug_string_handler(
});

if cfg!(debug_assertions) && level == log::Level::Error {
// Panicking behind FFI is UB, so we just exit.
std::process::exit(1);
// Set canary and continue
crate::VALIDATION_CANARY.set();
}

excpt::EXCEPTION_CONTINUE_EXECUTION
Expand Down
3 changes: 2 additions & 1 deletion wgpu-hal/src/gles/egl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,8 @@ fn gl_debug_message_callback(source: u32, gltype: u32, id: u32, severity: u32, m
});

if cfg!(debug_assertions) && log_severity == log::Level::Error {
std::process::exit(1);
// Set canary and continue
crate::VALIDATION_CANARY.set();
}
}

Expand Down
34 changes: 34 additions & 0 deletions wgpu-hal/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ use std::{
num::{NonZeroU32, NonZeroU8},
ops::{Range, RangeInclusive},
ptr::NonNull,
sync::atomic::AtomicBool,
};

use bitflags::bitflags;
Expand Down Expand Up @@ -1148,6 +1149,39 @@ pub struct ComputePassDescriptor<'a> {
pub label: Label<'a>,
}

/// Stores if any API validation error has occurred in this process
/// since it was last reset.
///
/// This is used for internal wgpu testing only and _must not_ be used
/// as a way to check for errors.
///
/// This works as a static because `cargo nextest` runs all of our
/// tests in separate processes, so each test gets its own canary.
///
/// This prevents the issue of one validation error terminating the
/// entire process.
pub static VALIDATION_CANARY: ValidationCanary = ValidationCanary {
inner: AtomicBool::new(false),
};

/// Flag for internal testing.
pub struct ValidationCanary {
inner: AtomicBool,
}

impl ValidationCanary {
#[allow(dead_code)] // in some configurations this function is dead
fn set(&self) {
self.inner.store(true, std::sync::atomic::Ordering::SeqCst);
}

/// Returns true if any API validation error has occurred in this process
/// since the last call to this function.
pub fn get_and_reset(&self) -> bool {
self.inner.swap(false, std::sync::atomic::Ordering::SeqCst)
}
}

#[test]
fn test_default_limits() {
let limits = wgt::Limits::default();
Expand Down
6 changes: 4 additions & 2 deletions wgpu-hal/src/vulkan/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,10 @@ unsafe extern "system" fn debug_utils_messenger_callback(
});
}

// uncommenting this is useful for tests
//assert_ne!(level, log::Level::Error);
if cfg!(debug_assertions) && level == log::Level::Error {
// Set canary and continue
crate::VALIDATION_CANARY.set();
}

vk::FALSE
}
Expand Down
6 changes: 3 additions & 3 deletions wgpu/examples/skybox/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ fn skybox_bc1() {
Some(wgpu::Backends::GL),
None,
Some("ANGLE"),
true,
false,
), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
tolerance: 5,
max_outliers: 10,
Expand All @@ -512,7 +512,7 @@ fn skybox_etc2() {
Some(wgpu::Backends::GL),
None,
Some("ANGLE"),
true,
false,
), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
tolerance: 5,
max_outliers: 105, // Bounded by llvmpipe
Expand All @@ -530,7 +530,7 @@ fn skybox_astc() {
Some(wgpu::Backends::GL),
None,
Some("ANGLE"),
true,
false,
), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
tolerance: 5,
max_outliers: 300, // Bounded by rp4 on vk
Expand Down
6 changes: 3 additions & 3 deletions wgpu/tests/clear_texture.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ fn clear_texture_2d_bc() {
initialize_test(
TestParameters::default()
.features(wgpu::Features::CLEAR_TEXTURE | wgpu::Features::TEXTURE_COMPRESSION_BC)
.specific_failure(Some(wgpu::Backends::GL), None, Some("ANGLE"), true), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
.specific_failure(Some(wgpu::Backends::GL), None, Some("ANGLE"), false), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
|ctx| {
clear_texture_tests(&ctx, TEXTURE_FORMATS_BC, false, true);
},
Expand All @@ -327,7 +327,7 @@ fn clear_texture_2d_astc() {
initialize_test(
TestParameters::default()
.features(wgpu::Features::CLEAR_TEXTURE | wgpu::Features::TEXTURE_COMPRESSION_ASTC_LDR)
.specific_failure(Some(wgpu::Backends::GL), None, Some("ANGLE"), true), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
.specific_failure(Some(wgpu::Backends::GL), None, Some("ANGLE"), false), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
|ctx| {
clear_texture_tests(&ctx, TEXTURE_FORMATS_ASTC, false, true);
},
Expand All @@ -339,7 +339,7 @@ fn clear_texture_2d_etc2() {
initialize_test(
TestParameters::default()
.features(wgpu::Features::CLEAR_TEXTURE | wgpu::Features::TEXTURE_COMPRESSION_ETC2)
.specific_failure(Some(wgpu::Backends::GL), None, Some("ANGLE"), true), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
.specific_failure(Some(wgpu::Backends::GL), None, Some("ANGLE"), false), // https://bugs.chromium.org/p/angleproject/issues/detail?id=7056
|ctx| {
clear_texture_tests(&ctx, TEXTURE_FORMATS_ETC2, false, true);
},
Expand Down
29 changes: 21 additions & 8 deletions wgpu/tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ pub fn initialize_test(parameters: TestParameters, test_function: impl FnOnce(Te
queue,
};

let failure_reason = parameters.failures.iter().find_map(|failure| {
let expected_failure_reason = parameters.failures.iter().find_map(|failure| {
let always =
failure.backends.is_none() && failure.vendor.is_none() && failure.adapter.is_none();

Expand Down Expand Up @@ -261,25 +261,38 @@ pub fn initialize_test(parameters: TestParameters, test_function: impl FnOnce(Te
}
});

if let Some((reason, true)) = failure_reason {
if let Some((reason, true)) = expected_failure_reason {
println!("EXPECTED TEST FAILURE SKIPPED: {:?}", reason);
return;
}

let panicked = catch_unwind(AssertUnwindSafe(|| test_function(context))).is_err();
let canary_set = hal::VALIDATION_CANARY.get_and_reset();

let expect_failure = failure_reason.is_some();
let failed = panicked || canary_set;

if panicked == expect_failure {
let failure_cause = match (panicked, canary_set) {
(true, true) => "PANIC AND VALIDATION ERROR",
(true, false) => "PANIC",
(false, true) => "VALIDATION ERROR",
(false, false) => "",
};

let expect_failure = expected_failure_reason.is_some();

if failed == expect_failure {
// We got the conditions we expected
if let Some((reason, _)) = failure_reason {
if let Some((expected_reason, _)) = expected_failure_reason {
// Print out reason for the failure
println!("GOT EXPECTED TEST FAILURE: {:?}", reason);
println!(
"GOT EXPECTED TEST FAILURE DUE TO {}: {:?}",
failure_cause, expected_reason
);
}
} else if let Some((reason, _)) = failure_reason {
} else if let Some((reason, _)) = expected_failure_reason {
// We expected to fail, but things passed
panic!("UNEXPECTED TEST PASS: {:?}", reason);
} else {
panic!("UNEXPECTED TEST FAILURE")
panic!("UNEXPECTED TEST FAILURE DUE TO {}", failure_cause)
}
}

0 comments on commit 4d7f6eb

Please sign in to comment.