From e20192af423164c05c6bce15dcdd8063c7ac0f71 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 20 Feb 2023 20:56:28 +0000 Subject: [PATCH 1/6] Add option to force wrapper generation. --- engine/src/builder.rs | 35 +++++++++++++++++------ engine/src/conversion/analysis/fun/mod.rs | 5 ++++ engine/src/conversion/conversion_tests.rs | 4 +-- engine/src/conversion/mod.rs | 19 ++++++++---- engine/src/lib.rs | 14 +++++++-- engine/src/parse_file.rs | 6 ++-- gen/cmd/src/main.rs | 10 +++++-- 7 files changed, 68 insertions(+), 25 deletions(-) diff --git a/engine/src/builder.rs b/engine/src/builder.rs index 15cc3353f..ca5c7f847 100644 --- a/engine/src/builder.rs +++ b/engine/src/builder.rs @@ -10,7 +10,7 @@ use autocxx_parser::file_locations::FileLocationStrategy; use miette::Diagnostic; use thiserror::Error; -use crate::generate_rs_single; +use crate::{generate_rs_single, CodegenOptions}; use crate::{strip_system_headers, CppCodegenOptions, ParseError, RebuildDependencyRecorder}; use std::ffi::OsStr; use std::ffi::OsString; @@ -73,7 +73,7 @@ pub struct Builder<'a, BuilderContext> { dependency_recorder: Option>, custom_gendir: Option, auto_allowlist: bool, - cpp_codegen_options: CppCodegenOptions<'a>, + codegen_options: CodegenOptions<'a>, // This member is to ensure that this type is parameterized // by a BuilderContext. The goal is to balance three needs: // (1) have most of the functionality over in autocxx_engine, @@ -108,7 +108,7 @@ impl Builder<'_, CTX> { dependency_recorder: CTX::get_dependency_recorder(), custom_gendir: None, auto_allowlist: false, - cpp_codegen_options: CppCodegenOptions::default(), + codegen_options: CodegenOptions::default(), ctx: PhantomData, } } @@ -130,7 +130,7 @@ impl Builder<'_, CTX> { where F: FnOnce(&mut CppCodegenOptions), { - modifier(&mut self.cpp_codegen_options); + modifier(&mut self.codegen_options.cpp_codegen_options); self } @@ -152,20 +152,33 @@ impl Builder<'_, CTX> { self } + #[doc(hidden)] + /// Whether to force autocxx always to generate extra Rust and C++ + /// side shims. This is only used by the integration test suite to + /// exercise more code paths - don't use it! + pub fn force_wrapper_generation(mut self, do_it: bool) -> Self { + self.codegen_options.force_wrapper_gen = do_it; + self + } + /// Whether to suppress inclusion of system headers (`memory`, `string` etc.) /// from generated C++ bindings code. This should not normally be used, /// but can occasionally be useful if you're reducing a test case and you /// have a preprocessed header file which already contains absolutely everything /// that the bindings could ever need. pub fn suppress_system_headers(mut self, do_it: bool) -> Self { - self.cpp_codegen_options.suppress_system_headers = do_it; + self.codegen_options + .cpp_codegen_options + .suppress_system_headers = do_it; self } /// An annotation optionally to include on each C++ function. /// For example to export the symbol from a library. pub fn cxx_impl_annotations(mut self, cxx_impl_annotations: Option) -> Self { - self.cpp_codegen_options.cxx_impl_annotations = cxx_impl_annotations; + self.codegen_options + .cpp_codegen_options + .cxx_impl_annotations = cxx_impl_annotations; self } @@ -208,7 +221,11 @@ impl Builder<'_, CTX> { write_to_file( &incdir, "cxx.h", - &Self::get_cxx_header_bytes(self.cpp_codegen_options.suppress_system_headers), + &Self::get_cxx_header_bytes( + self.codegen_options + .cpp_codegen_options + .suppress_system_headers, + ), )?; let autocxx_inc = build_autocxx_inc(self.autocxx_incs, &incdir); @@ -221,7 +238,7 @@ impl Builder<'_, CTX> { autocxx_inc, clang_args, self.dependency_recorder, - &self.cpp_codegen_options, + &self.codegen_options, ) .map_err(BuilderError::ParseError)?; let mut counter = 0; @@ -235,7 +252,7 @@ impl Builder<'_, CTX> { builder.includes(parsed_file.include_dirs()); for include_cpp in parsed_file.get_cpp_buildables() { let generated_code = include_cpp - .generate_h_and_cxx(&self.cpp_codegen_options) + .generate_h_and_cxx(&self.codegen_options.cpp_codegen_options) .map_err(BuilderError::InvalidCxx)?; for filepair in generated_code.0 { let fname = format!("gen{counter}.cxx"); diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index e05e55355..4402d0d97 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -284,6 +284,7 @@ pub(crate) struct FnAnalyzer<'a> { generic_types: HashSet, types_in_anonymous_namespace: HashSet, existing_superclass_trait_api_names: HashSet, + force_wrapper_generation: bool, } impl<'a> FnAnalyzer<'a> { @@ -291,6 +292,7 @@ impl<'a> FnAnalyzer<'a> { apis: ApiVec, unsafe_policy: &'a UnsafePolicy, config: &'a IncludeCppConfig, + force_wrapper_generation: bool, ) -> ApiVec { let mut me = Self { unsafe_policy, @@ -306,6 +308,7 @@ impl<'a> FnAnalyzer<'a> { generic_types: Self::build_generic_type_set(&apis), existing_superclass_trait_api_names: HashSet::new(), types_in_anonymous_namespace: Self::build_types_in_anonymous_namespace(&apis), + force_wrapper_generation, }; let mut results = ApiVec::new(); convert_apis( @@ -1249,6 +1252,7 @@ impl<'a> FnAnalyzer<'a> { _ if ret_type_conversion_needed => true, _ if cpp_name_incompatible_with_cxx => true, _ if fun.synthetic_cpp.is_some() => true, + _ if self.force_wrapper_generation => true, _ => false, }; @@ -1362,6 +1366,7 @@ impl<'a> FnAnalyzer<'a> { _ if any_param_needs_rust_conversion || return_needs_rust_conversion => true, FnKind::TraitMethod { .. } => true, FnKind::Method { .. } => cxxbridge_name != rust_name, + _ if self.force_wrapper_generation => true, _ => false, }; diff --git a/engine/src/conversion/conversion_tests.rs b/engine/src/conversion/conversion_tests.rs index c02819b6a..e74888e8e 100644 --- a/engine/src/conversion/conversion_tests.rs +++ b/engine/src/conversion/conversion_tests.rs @@ -11,7 +11,7 @@ use autocxx_parser::UnsafePolicy; use syn::parse_quote; use syn::ItemMod; -use crate::CppCodegenOptions; +use crate::CodegenOptions; use super::BridgeConverter; @@ -33,7 +33,7 @@ fn do_test(input: ItemMod) { input, UnsafePolicy::AllFunctionsSafe, inclusions, - &CppCodegenOptions::default(), + &CodegenOptions::default(), "", ) .unwrap(); diff --git a/engine/src/conversion/mod.rs b/engine/src/conversion/mod.rs index 5eff0dd34..1553ef2ad 100644 --- a/engine/src/conversion/mod.rs +++ b/engine/src/conversion/mod.rs @@ -28,7 +28,7 @@ use itertools::Itertools; use syn::{Item, ItemMod}; use crate::{ - conversion::analysis::deps::HasDependencies, CppCodegenOptions, CppFilePair, UnsafePolicy, + conversion::analysis::deps::HasDependencies, CodegenOptions, CppFilePair, UnsafePolicy, }; use self::{ @@ -122,7 +122,7 @@ impl<'a> BridgeConverter<'a> { mut bindgen_mod: ItemMod, unsafe_policy: UnsafePolicy, inclusions: String, - cpp_codegen_options: &CppCodegenOptions, + codegen_options: &CodegenOptions, source_file_contents: &str, ) -> Result { match &mut bindgen_mod.content { @@ -158,8 +158,12 @@ impl<'a> BridgeConverter<'a> { // part of `autocxx`. Again, this returns a new set of `Api`s, but // parameterized by a richer set of metadata. Self::dump_apis("adding casts", &analyzed_apis); - let analyzed_apis = - FnAnalyzer::analyze_functions(analyzed_apis, &unsafe_policy, self.config); + let analyzed_apis = FnAnalyzer::analyze_functions( + analyzed_apis, + &unsafe_policy, + self.config, + codegen_options.force_wrapper_gen, + ); // If any of those functions turned out to be pure virtual, don't attempt // to generate UniquePtr implementations for the type, since it can't // be instantiated. @@ -189,12 +193,15 @@ impl<'a> BridgeConverter<'a> { Self::dump_apis_with_deps("GC", &analyzed_apis); // And finally pass them to the code gen phases, which outputs // code suitable for cxx to consume. - let cxxgen_header_name = cpp_codegen_options.cxxgen_header_namer.name_header(); + let cxxgen_header_name = codegen_options + .cpp_codegen_options + .cxxgen_header_namer + .name_header(); let cpp = CppCodeGenerator::generate_cpp_code( inclusions, &analyzed_apis, self.config, - cpp_codegen_options, + &codegen_options.cpp_codegen_options, &cxxgen_header_name, ) .map_err(ConvertError::Cpp)?; diff --git a/engine/src/lib.rs b/engine/src/lib.rs index 4e8d47647..d2762bdd9 100644 --- a/engine/src/lib.rs +++ b/engine/src/lib.rs @@ -146,6 +146,16 @@ enum State { Generated(Box), } +/// Code generation options. +#[derive(Default)] +pub struct CodegenOptions<'a> { + // An option used by the test suite to force a more convoluted + // route through our code, to uncover bugs. + pub force_wrapper_gen: bool, + /// Options about the C++ code generation. + pub cpp_codegen_options: CppCodegenOptions<'a>, +} + const AUTOCXX_CLANG_ARGS: &[&str; 4] = &["-x", "c++", "-std=c++14", "-DBINDGEN"]; /// Implement to learn of header files which get included @@ -408,7 +418,7 @@ impl IncludeCppEngine { inc_dirs: Vec, extra_clang_args: &[&str], dep_recorder: Option>, - cpp_codegen_options: &CppCodegenOptions, + codegen_options: &CodegenOptions, ) -> Result<()> { // If we are in parse only mode, do nothing. This is used for // doc tests to ensure the parsing is valid, but we can't expect @@ -456,7 +466,7 @@ impl IncludeCppEngine { bindings, self.config.unsafe_policy.clone(), header_contents, - cpp_codegen_options, + codegen_options, &source_file_contents, ) .map_err(Error::Conversion)?; diff --git a/engine/src/parse_file.rs b/engine/src/parse_file.rs index b5b05a0be..4cf429115 100644 --- a/engine/src/parse_file.rs +++ b/engine/src/parse_file.rs @@ -12,7 +12,7 @@ use crate::{ cxxbridge::CxxBridge, Error as EngineError, GeneratedCpp, IncludeCppEngine, RebuildDependencyRecorder, }; -use crate::{CppCodegenOptions, LocatedSynError}; +use crate::{CodegenOptions, CppCodegenOptions, LocatedSynError}; use autocxx_parser::directive_names::SUBCLASS; use autocxx_parser::{AllowlistEntry, RustPath, Subclass, SubclassAttrs}; use indexmap::set::IndexSet as HashSet; @@ -372,7 +372,7 @@ impl ParsedFile { autocxx_inc: Vec, extra_clang_args: &[&str], dep_recorder: Option>, - cpp_codegen_options: &CppCodegenOptions, + codegen_options: &CodegenOptions, ) -> Result<(), ParseError> { let mut mods_found = HashSet::new(); let inner_dep_recorder: Option> = @@ -394,7 +394,7 @@ impl ParsedFile { autocxx_inc.clone(), extra_clang_args, dep_recorder, - cpp_codegen_options, + codegen_options, ) .map_err(ParseError::AutocxxCodegenError)? } diff --git a/gen/cmd/src/main.rs b/gen/cmd/src/main.rs index 001dc0ab1..5bb309639 100644 --- a/gen/cmd/src/main.rs +++ b/gen/cmd/src/main.rs @@ -247,6 +247,10 @@ fn main() -> miette::Result<()> { autocxxgen_header_namer, cxxgen_header_namer, }; + let codegen_options = autocxx_engine::CodegenOptions { + cpp_codegen_options: cpp_codegen_options, + ..Default::default() + }; let depfile = match matches.value_of("depfile") { None => None, Some(depfile_path) => { @@ -277,7 +281,7 @@ fn main() -> miette::Result<()> { incs.clone(), &extra_clang_args, dep_recorder, - &cpp_codegen_options, + &codegen_options, )?; } @@ -305,7 +309,7 @@ fn main() -> miette::Result<()> { .flat_map(|file| file.get_cpp_buildables()) { let generations = include_cxx - .generate_h_and_cxx(&cpp_codegen_options) + .generate_h_and_cxx(&codegen_options.cpp_codegen_options) .expect("Unable to generate header and C++ code"); for pair in generations.0 { let cppname = name_cc_file(counter); @@ -314,7 +318,7 @@ fn main() -> miette::Result<()> { counter += 1; } } - drop(cpp_codegen_options); + drop(codegen_options); // Write placeholders to ensure we always make exactly 'n' of each file type. writer.write_placeholders(counter, desired_number, name_cc_file)?; writer.write_placeholders( From dca93fa74f7c618826ca180801c02ae3ab35f5b6 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 20 Feb 2023 21:02:40 +0000 Subject: [PATCH 2/6] Add to test suite --- integration-tests/src/lib.rs | 37 ++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/integration-tests/src/lib.rs b/integration-tests/src/lib.rs index b4dcd6e72..2335ee5ab 100644 --- a/integration-tests/src/lib.rs +++ b/integration-tests/src/lib.rs @@ -421,6 +421,8 @@ pub fn do_run_test_manual( builder_modifier: Option, rust_code_checker: Option, ) -> Result<(), TestError> { + let builder_modifier = consider_forcing_wrapper_generation(builder_modifier); + const HEADER_NAME: &str = "input.h"; // Step 2: Write the C++ header snippet to a temp file let tdir = tempdir().unwrap(); @@ -510,3 +512,38 @@ pub fn do_run_test_manual( } Ok(()) } + +/// If AUTOCXX_FORCE_WRAPPER_GENERATION is set, always force both C++ +/// and Rust side shims, for extra testing of obscure code paths. +fn consider_forcing_wrapper_generation( + existing_builder_modifier: Option, +) -> Option { + if std::env::var("AUTOCXX_FORCE_WRAPPER_GENERATION").is_err() { + existing_builder_modifier + } else { + Some(Box::new(ForceWrapperGeneration(existing_builder_modifier))) + } +} + +struct ForceWrapperGeneration(Option); + +impl BuilderModifierFns for ForceWrapperGeneration { + fn modify_autocxx_builder<'a>( + &self, + builder: Builder<'a, TestBuilderContext>, + ) -> Builder<'a, TestBuilderContext> { + let builder = builder.force_wrapper_generation(true); + if let Some(modifier) = &self.0 { + modifier.modify_autocxx_builder(builder) + } else { + builder + } + } + fn modify_cc_builder<'a>(&self, builder: &'a mut cc::Build) -> &'a mut cc::Build { + if let Some(modifier) = &self.0 { + modifier.modify_cc_builder(builder) + } else { + builder + } + } +} From c34ae5e6d40bdaeb951c3060f03c2f087708c1fe Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 20 Feb 2023 21:08:51 +0000 Subject: [PATCH 3/6] Add github workflow. --- .github/workflows/ci.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bce927caf..563b8fffb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -177,6 +177,17 @@ jobs: RUST_BACKTRACE: "0" run: cargo -Z build-std test --workspace --target x86_64-unknown-linux-gnu + force-wrapper-generation: + name: Test forcing wrapper generation + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: Swatinem/rust-cache@v1 + - name: Tests generating all possible shims + env: + AUTOCXX_FORCE_WRAPPER_GENERATION: 1 + run: cargo test --workspace + # Clippy check clippy: name: Clippy From 4359edcf315cbeafdb8f69b9c2dcdc967f3cddc7 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 20 Feb 2023 21:09:26 +0000 Subject: [PATCH 4/6] Clippy fix. --- gen/cmd/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen/cmd/src/main.rs b/gen/cmd/src/main.rs index 5bb309639..95c7e3af8 100644 --- a/gen/cmd/src/main.rs +++ b/gen/cmd/src/main.rs @@ -248,7 +248,7 @@ fn main() -> miette::Result<()> { cxxgen_header_namer, }; let codegen_options = autocxx_engine::CodegenOptions { - cpp_codegen_options: cpp_codegen_options, + cpp_codegen_options, ..Default::default() }; let depfile = match matches.value_of("depfile") { From 52028c9a2c68c33d372c6dadd8dabd46545c73c2 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 20 Feb 2023 21:11:16 +0000 Subject: [PATCH 5/6] Add to book about this. --- book/src/contributing.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/book/src/contributing.md b/book/src/contributing.md index 77dc65d13..d82e64e04 100644 --- a/book/src/contributing.md +++ b/book/src/contributing.md @@ -71,7 +71,11 @@ RUST_BACKTRACE=1 RUST_LOG=autocxx_engine=info cargo test --all test_cycle_string This is especially valuable to see the `bindgen` output Rust code, and then the converted Rust code which we pass into cxx. Usually, most problems are due to some mis-conversion somewhere in `engine/src/conversion`. See [here](https://docs.rs/autocxx-engine/latest/autocxx_engine/struct.IncludeCppEngine.html) for documentation and diagrams on how the engine works. -You may also wish to set `AUTOCXX_ASAN=1` on Linux when running tests. +You may also wish to set `AUTOCXX_ASAN=1` on Linux when running tests. To exercise all +the code paths related to generating both C++ and Rust side shims, you can set +`AUTOCXX_FORCE_WRAPPER_GENERATION=1`. The test suite doesn't do this by default because +we also want to test the normal code paths. (In the future we might want to +parameterize the test suite to do both.) ## Reporting bugs From 2121c1a3051f49d4021d3557fba04c8bcf2ba590 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 20 Feb 2023 22:10:00 +0000 Subject: [PATCH 6/6] Disable the two tests which fail when wrappers are forced. --- integration-tests/tests/integration_test.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 42cea2a43..bf992012a 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -502,6 +502,7 @@ fn test_negative_take_as_pod_with_move_constructor() { run_test_expect_fail(cxx, hdr, rs, &["take_bob"], &["Bob"]); } +#[ignore] // https://github.com/google/autocxx/issues/1252 #[test] fn test_take_as_pod_with_is_relocatable() { let cxx = indoc! {" @@ -4980,6 +4981,7 @@ fn test_union_ignored() { run_test("", hdr, rs, &["B"], &[]); } +#[ignore] // https://github.com/google/autocxx/issues/1251 #[test] fn test_double_underscores_ignored() { let hdr = indoc! {"