From e7c503b64e3b47fa50d983c9056fbce48cd8c6af Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sun, 11 Oct 2020 21:10:41 -0700 Subject: [PATCH 1/3] Adding negative test facility. --- engine/src/integration_tests.rs | 84 +++++++++++++++++++++++++++++++-- 1 file changed, 81 insertions(+), 3 deletions(-) diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs index f2d8a76ec..513e84f71 100644 --- a/engine/src/integration_tests.rs +++ b/engine/src/integration_tests.rs @@ -90,6 +90,7 @@ fn write_to_file(tdir: &TempDir, filename: &str, content: &str) -> PathBuf { path } +/// A positive test, we expect to pass. fn run_test( cxx_code: &str, header_code: &str, @@ -97,6 +98,48 @@ fn run_test( allowed_funcs: &[&str], allowed_pods: &[&str], ) { + do_run_test( + cxx_code, + header_code, + rust_code, + allowed_funcs, + allowed_pods, + ) + .unwrap() +} + +fn run_test_expect_fail( + cxx_code: &str, + header_code: &str, + rust_code: TokenStream, + allowed_funcs: &[&str], + allowed_pods: &[&str], +) { + do_run_test( + cxx_code, + header_code, + rust_code, + allowed_funcs, + allowed_pods, + ) + .expect_err("Unexpected success"); +} + +/// In the future maybe the tests will distinguish the exact type of failure expected. +#[derive(Debug)] +enum TestError { + AutoCxx(autocxx_build::Error), + CppBuild(cc::Error), + RsBuild, +} + +fn do_run_test( + cxx_code: &str, + header_code: &str, + rust_code: TokenStream, + allowed_funcs: &[&str], + allowed_pods: &[&str], +) -> Result<(), TestError> { // Step 1: Write the C++ header snippet to a temp file let tdir = tempdir().unwrap(); write_to_file(&tdir, "input.h", &format!("#pragma once\n{}", header_code)); @@ -144,7 +187,8 @@ fn run_test( let target_dir = tdir.path().join("target"); std::fs::create_dir(&target_dir).unwrap(); let target = rust_info::get().target_triple.unwrap(); - let mut b = autocxx_build::Builder::new(&rs_path, tdir.path().to_str().unwrap()).unwrap(); + let mut b = autocxx_build::Builder::new(&rs_path, tdir.path().to_str().unwrap()) + .map_err(TestError::AutoCxx)?; b.builder() .file(cxx_path) .out_dir(&target_dir) @@ -154,7 +198,7 @@ fn run_test( .flag("-std=c++14") .include(tdir.path()) .try_compile("autocxx-demo") - .unwrap(); + .map_err(TestError::CppBuild)?; // Step 8: use the trybuild crate to build the Rust file. let r = BUILDER.lock().unwrap().build( &target_dir, @@ -163,10 +207,14 @@ fn run_test( "input.h", &rs_path, ); + if r.is_err() { + return Err(TestError::RsBuild); // details of Rust panic are a bit messy to include, and + // not important at the moment. + } if KEEP_TEMPDIRS { println!("Tempdir: {:?}", tdir.into_path().to_str()); } - r.unwrap() + Ok(()) } #[test] @@ -990,6 +1038,36 @@ fn test_i32_const() { run_test(cxx, hdr, rs, &["BOB"], &[]); } +#[test] +fn test_negative_rs_nonsense() { + // Really just testing the test infrastructure. + let cxx = indoc! {" + "}; + let hdr = indoc! {" + #include + const uint32_t BOB = 3; + "}; + let rs = quote! { + foo bar + }; + run_test_expect_fail(cxx, hdr, rs, &["BOB"], &[]); +} + +#[test] +fn test_negative_cpp_nonsense() { + // Really just testing the test infrastructure. + let cxx = indoc! {" + "}; + let hdr = indoc! {" + #include + const uint32_t BOB = CAT; + "}; + let rs = quote! { + assert_eq!(ffi::BOB, 3); + }; + run_test_expect_fail(cxx, hdr, rs, &["BOB"], &[]); +} + // Yet to test: // 1. Make UniquePtr in Rust // 3. Constants From 27cc9a0736a7c7a42f599e932f79c55eb980dacb Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sun, 11 Oct 2020 21:16:17 -0700 Subject: [PATCH 2/3] Do not allow allocation of opaque structs. --- engine/src/bridge_converter.rs | 6 +++++- engine/src/integration_tests.rs | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs index 96076be08..0ae3ea772 100644 --- a/engine/src/bridge_converter.rs +++ b/engine/src/bridge_converter.rs @@ -211,7 +211,11 @@ impl<'a> BridgeConversion<'a> { let should_be_pod = self.byvalue_checker.is_pod(&tyname); self.generate_type_alias(tyname, should_be_pod); if !should_be_pod { - s.fields = syn::Fields::Unit; + s.fields = syn::Fields::Named(parse_quote! { + { + do_not_attempt_to_allocate_nonpod_types: u8, + } + }); } self.bindgen_items.push(Item::Struct(s)); } diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs index 513e84f71..197f7a565 100644 --- a/engine/src/integration_tests.rs +++ b/engine/src/integration_tests.rs @@ -1068,6 +1068,41 @@ fn test_negative_cpp_nonsense() { run_test_expect_fail(cxx, hdr, rs, &["BOB"], &[]); } +#[test] +fn test_negative_make_nonpod() { + let cxx = indoc! {" + uint32_t take_bob(const Bob& a) { + return a.a; + } + std::unique_ptr make_bob(uint32_t a) { + auto b = std::make_unique(); + b->a = a; + return b; + } + "}; + let hdr = indoc! {" + #include + #include + struct Bob { + uint32_t a; + }; + std::unique_ptr make_bob(uint32_t a); + uint32_t take_bob(const Bob& a); + "}; + let rs = quote! { + ffi::cxxbridge::Bob {}; + }; + let rs2 = quote! { + ffi::cxxbridge::Bob { a: 12 }; + }; + let rs3 = quote! { + ffi::cxxbridge::Bob { do_not_attempt_to_allocate_nonpod_types: 12 }; + }; + run_test_expect_fail(cxx, hdr, rs, &["take_bob", "Bob", "make_bob"], &[]); + run_test_expect_fail(cxx, hdr, rs2, &["take_bob", "Bob", "make_bob"], &[]); + run_test_expect_fail(cxx, hdr, rs3, &["take_bob", "Bob", "make_bob"], &[]); +} + // Yet to test: // 1. Make UniquePtr in Rust // 3. Constants From 6958069b49458f8b33c7481727ba665b6964da5d Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Sun, 11 Oct 2020 22:18:01 -0700 Subject: [PATCH 3/3] Switch to non-Sync type. --- engine/src/bridge_converter.rs | 4 +++- engine/src/integration_tests.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs index 0ae3ea772..dc99b9a70 100644 --- a/engine/src/bridge_converter.rs +++ b/engine/src/bridge_converter.rs @@ -211,9 +211,11 @@ impl<'a> BridgeConversion<'a> { let should_be_pod = self.byvalue_checker.is_pod(&tyname); self.generate_type_alias(tyname, should_be_pod); if !should_be_pod { + // See cxx's opaque::Opaque for rationale for this type... in + // short, it's to avoid being Send/Sync. s.fields = syn::Fields::Named(parse_quote! { { - do_not_attempt_to_allocate_nonpod_types: u8, + do_not_attempt_to_allocate_nonpod_types: [*const u8; 0], } }); } diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs index 197f7a565..b8fa7c9cd 100644 --- a/engine/src/integration_tests.rs +++ b/engine/src/integration_tests.rs @@ -1096,7 +1096,7 @@ fn test_negative_make_nonpod() { ffi::cxxbridge::Bob { a: 12 }; }; let rs3 = quote! { - ffi::cxxbridge::Bob { do_not_attempt_to_allocate_nonpod_types: 12 }; + ffi::cxxbridge::Bob { do_not_attempt_to_allocate_nonpod_types: [] }; }; run_test_expect_fail(cxx, hdr, rs, &["take_bob", "Bob", "make_bob"], &[]); run_test_expect_fail(cxx, hdr, rs2, &["take_bob", "Bob", "make_bob"], &[]);