From 1c99c21f952b909f7a5e0a757d106f18494c14da Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Mon, 24 Feb 2025 09:04:08 +0000 Subject: [PATCH] Maintenance documentation part one. --- book/src/SUMMARY.md | 1 + book/src/contributing.md | 200 ++++++++++++++++++++++++++++++------- book/src/reporting_bugs.md | 61 +++++++++++ 3 files changed, 226 insertions(+), 36 deletions(-) create mode 100644 book/src/reporting_bugs.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index fcedb9966..b2c208699 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -18,6 +18,7 @@ - [Large codebases](large_codebase.md) - [Examples](examples.md) - [Credits](credits.md) +- [Reporting bugs](reporting_bugs.md) - [Contributing](contributing.md) - [Code of Conduct](code-of-conduct.md) diff --git a/book/src/contributing.md b/book/src/contributing.md index d82e64e04..9ca1cfed6 100644 --- a/book/src/contributing.md +++ b/book/src/contributing.md @@ -53,8 +53,8 @@ to `cxx`. However, most of the actual code is in `engine/src/conversion/mod.rs`. At the moment we're using a slightly branched version of `bindgen` called `autocxx-bindgen`. -It's hoped this is temporary; some of our changes are sufficiently weird that it would be -presumptious to try to get them accepted upstream until we're sure `autocxx` has roughly the right approach. +It's hoped this is temporary (see [here](https://github.com/google/autocxx/issues/124) +for status.) ## How to develop @@ -79,40 +79,7 @@ parameterize the test suite to do both.) ## Reporting bugs -If you've found a problem, and you're reading this, *thank you*! Your diligence -in reporting the bug is much appreciated and will make `autocxx` better. In -order of preference here's how we would like to hear about your problem: - -* Raise a pull request adding a new failing integration test to - [`integration_test.rs`](https://github.com/google/autocxx/blob/main/integration-tests/tests/integration_test.rs) -* Minimize the test using `tools/reduce`, something like this: - `target/debug/autocxx-reduce file -d "safety!(unsafe_ffi)" -d - 'generate_pod!("A")' -I ~/my-include-dir -h my-header.h -p - problem-error-message -- --remove-pass pass_line_markers` - This is a wrapper for the amazing `creduce` which will take thousands of lines - of C++, preprocess it, and then identify the minimum required lines to - reproduce the same problem. -* Use the C++ preprocessor to give a single complete C++ file which demonstrates - the problem, along with the `include_cpp!` directive you use. - Alternatively, run your build using `AUTOCXX_REPRO_CASE=repro.json` which should - put everything we need into `output.h`. If necessary, you can use the `CLANG_PATH` - or `CXX` environment variables to specify the path to the Clang compiler to use. -* Failing all else, build using - `cargo clean -p && RUST_LOG=autocxx_engine=info cargo build -vvv` - and send the _entire_ log to us. This will include two key bits of logging: - the C++ bindings as distilled by `bindgen`, and then the version which - we've converted and moulded to be suitable for use by `cxx`. - -## Bugs related to linking problems - -Unfortunately, _linking_ C++ binaries is a complex area subject in itself, and -we won't be able to debug your linking issues by means of an autocxx bug report. -Assuming you're using autocxx's build.rs support, the actual C++ build and -managed by the [`cc`](https://crates.io/crates/cc) crate. You can find -many of its options on its [`Build` type](https://docs.rs/cc/latest/cc/struct.Build.html). -If you need to bring in an external library, you may also need to emit certain -[print statements from your `build.rs`](https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-lib) -to instruct cargo to link against that library. +Moved to [its own document](reporting_bugs.md). ## How to contribute to this manual @@ -121,3 +88,164 @@ More examples in this manual are _very_ welcome! Because `autocxx` examples require both Rust and C++ code to be linked together, a custom preprocessor is used for this manual. See one of the existing examples such as in `index.md` to see how to do this. + +## Maintenance responsibilities + +autocxx is currently in maintenance mode. In future, it may undergo further +feature development, but for now the job is just to keep it stable +and functional. + +Events may occur which nonetheless require changes: + +* Changes to Rust +* Changes to `bindgen` +* Pull requests raised against autocxx +* Issues reported against autocxx + +Here's what to do. If issues are reported, encourage the reporter +to raise a PR with a minimized test case by pointing them at the +["reporting bugs"](reporting_bugs.md) page. This resolves all concerns about +reproducibility. autocxx is quite sensitive to the environment in which it +runs, e.g. standard library header files in use, and it's rare to be able +to reproduce a reporter's bug without them raising a reproducible test case +like this. + +If CI starts to fail due to a Rust change (or, +if we [manage to unfork bindgen, a bindgen change](https://github.com/google/autocxx/issues/124)) +then raise a pull request to fix it. (Most commonly CI starts to fail +because of more aggressive clippy lints). + +For user-contributed pull requests, merge them if you can! For reproducible +bug reports, try to investigate them. A fair proportion of the bug reports +boil down to certain key known areas of technical debt or limitations, +described below, and you can mark them as a duplicate or similar. + +autocxx tends to make a release every month or two, dependent on what changes +have been made. Ideally, autocxx would release after every single pull +request but the process takes about 15 minutes (see below) so it's not +that frequent. + +## Release process + +To make a new release of autocxx, + +* First ensure there's green CI on github. +* Check out `main` locally and ensure it's up to date with `origin/main`. +* Make a new branch +* Run `tools/upgrade-version.sh OLD NEW` where `OLD` is the previous + released version number, and `NEW` is the new version number. +* Commit that and make a PR; ensure it passes tests on github CI. +* If so, merge that PR, and update locally. +* Run `tools/publish-all.sh`. This will do the actual `cargo publish` + for all the various crates. +* On [github releases](https://github.com/google/autocxx/releases), + choose Draft a new release. Add a tag for `v0.X.Y`. Go through + the process to automatically create release notes. + +## Rolling bindgen + +autocxx currently depends upon a fork of bindgen called `autocxx-bindgen`. +[This issue](https://github.com/google/autocxx/issues/124) is an attempt +at entirely unforking bindgen. There are about 14 upstream PRs required; +we should work hard to make required changes to get them accepted, +because then this section becomes irrelevant and you'll never need to follow +these steps. + +Otherwise, periodically, you'll need to merge bindgen into autocxx-bindgen. +[`autocxx-bindgen` is kept in this repository](https://github.com/adetaylor/rust-bindgen) +in the `master` branch (which should be renamed eventually if we don't +unfork bindgen). + +To update it, + +* Check out the master branch of that repo +* Make a new branch +* `git pull main` (note that we use merge rather than + rebase) +* Resolve conflicts and commit. +* Push this branch to [the autocxx-bindgen repo](https://github.com/adetaylor/rust-bindgen) +* Make a new branch of autocxx +* Amend [`engine/Cargo.toml`](https://github.com/google/autocxx/blob/main/engine/Cargo.toml#L34) + to point to the new git branch of autocxx-bindgen instead of using + a published version (see the commented-out line) +* Maybe `cd integration-tests; cargo test` to ensure things build and seem to + work... but you won't really know until you push to github CI +* So, push your autocxx and make a pull request +* If everything passes on CI, bump the `autocxx-bindgen` version number. + This needs to be done in `bindgen/Cargo.toml` and `Cargo.toml`. Commit that. +* Push directly onto the master branch of + [the autocxx-bindgen repo](https://github.com/adetaylor/rust-bindgen). + You can't raise this roll as a pull request because Google's CLA tooling + will reject it. +* `cargo publish` the new autocxx-bindgen version. +* Amend your `autocxx` PR to switch to the published `autocxx-bindgen` version, + and push that. +* Ensure CI still passes. +* If so, merge your `autocxx` PR. + +## Major areas of tech debt + +The core of autocxx is the `Api` enum. That's solid. It's created by parsing +bindgen output, and exists all the way through to Rust and C++ codegen. +It is annotated by various analysis phases; it's parameterized by +the analysis phase so it's literally impossible to access those annotations +before they exist. + +Because this core is solid, most of the areas of tech debt are discrete and +don't really overlap with each other. If you wish to tackle them, they +can be tackled fairly independently. + +Without further ado, they are: + +1. **Naming**. autocxx deals with lots of names - the original C++ name, + the name chosen by bindgen, the name we want to give to cxx, etc. + When one kind of name is used for another purpose, certain invariants + need to be applied, e.g. avoiding conflicting overloads, or avoiding + built-in cxx names such as `UniquePtr`. So, these different kinds of + names really need to be encapsulated in newtype wrappers which + enforce these invariants. This has been [started here](https://github.com/google/autocxx/issues/520). + The other tricky aspect here is that we deal with name conflicts in + two different ways. What are name conflicts? bindgen gives us + a hierarchical namespace which we have to map to a flat namespace to give + to cxx. For types, we just reject any duplicate names. For funtions, though, + we go to some efforts to rename them to be non-conflicting. We should + become uniform here by abstracting the name deconfliction stuff from the + function analysis. + +2. **Fork of bindgen**. As noted under [rolling bindgen, above](#rolling-bindgen), + we currently use a fork of bindgen called `autocxx-bindgen`. This carries + a maintenance burden as we roll upstream. Also as noted above, we're + making good progress on undoing this. Tracked + [here](https://github.com/google/autocxx/issues/124). + +3. **Sensitivity to bindgen bugs caused by constructor calculations**. + `bindgen` isn't perfect. When it mis-generates a type, users can + ask bindgen to mark that type as "opaque", in which case it's represented + as just an array of bytes. This is especially useful for complex templated + C++ types where bindgen still has a number of bugs. Unfortunately, users + of autocxx don't get that luxury. In order to calculate whether types + are movable, autocxx requires visibility into all the fields. + That means that any types marked opaque by bindgen are pretty useless, + and we never currently ask bindgen for opaque types. + `bindgen` might be in a better place to request implicit constructors, + then we can solve these problems. This is tracked + [here](https://github.com/google/autocxx/issues/1461). + +4. **`CppRef` and friends.** `cxx` (and therefore `autocxx`) can be used + to create multiple mutable references to the same Rust memory. + This is UB. In cxx this problem applies only to types marked `Trivial` + since others are zero-sized. In `autocxx`, we give all types a size + so they can be allocated on the stack, and so the problem is worse. + (This is explained more + [here](https://github.com/google/autocxx/blob/main/engine/src/conversion/codegen_rs/non_pod_struct.rs#L41)). + The intended solution here is to cease to use Rust references to + represent C++ references - instead using a newtype wrapper called + `CppRef`. `autocxx` can already work in this mode using + [`unsafe_references_wrapped`](https://docs.rs/autocxx/latest/autocxx/macro.safety.html) + but for now it works well only on nightly Rust, pending the merge of the + [arbitrary self types v2 RFC](https://github.com/rust-lang/rust/pull/135881). + Even then, this isn't quite perfect because `cxx` expects to use + Rust references, and its key types (such as `cxx::UniquePtr`) provide + `Deref` implementations which point in that direction. This isn't a + huge problem but I mention it here because it's one of the very few + occasions in which our dependence on `cxx` is limiting for us. \ No newline at end of file diff --git a/book/src/reporting_bugs.md b/book/src/reporting_bugs.md new file mode 100644 index 000000000..09a68ac85 --- /dev/null +++ b/book/src/reporting_bugs.md @@ -0,0 +1,61 @@ + +## Reporting bugs + +If you've found a problem, and you're reading this, *thank you*! Your diligence +in reporting the bug is much appreciated and will make `autocxx` better. + +However, we get a lot of incoming bugs, and we are limited by our ability +to triage them. Please help us by making a good bug report. + +Much of this is about minimizing the size of your test case so it does not +depend on your system's C++ headers or any copyrighted code. To minimize the +test case, you can use `tools/reduce`, something like this: + +`cd tools/reduce; cargo run -- file -d "safety!(unsafe_ffi)" -d +'generate_pod!("A")' -I ~/my-include-dir -h my-header.h -p +problem-error-message -- --remove-pass pass_line_markers` +This is a wrapper for the amazing `creduce` which will take thousands of lines +of C++, preprocess it, and then identify the minimum required lines to +reproduce the same problem. This can sometimes take _days_ to run - that's +normal - if this is a problem, try to minimize the test case manually, or +do a combination. + +Once you have a minimal test case: + +* First see if it triggers a problem when you're using pure `bindgen`. + autocxx is very dependent on `bindgen` generating correct code. + To do this, fetch the [`bindgen` code](https://github.com/rust-lang/rust-bindgen) + then run + `cargo run -- XYZ.hpp --no-layout-tests --enable-cxx-namespaces --allowlist-type ABC-- -std=c++14` + `XYZ.hpp` is your header (the extension is important) and `ABC` is the type + you wish to generate. Assuming you minimized your test case in advance, + this should generate just a few lines of Rust code and you can inspect it + (or even build it) to see if it seems to be problematic. +* If the problem is in `bindgen`, report it to `bindgen`. +* If the problem is in `autocxx`, please report a bug to us. In order of preference: + * Raise a pull request adding a new minimized failing integration test to + [`integration_test.rs`](https://github.com/google/autocxx/blob/main/integration-tests/tests/integration_test.rs) + * Simply raise an issue with the minimized code. + +If you can't get minimization to work: +* Use the C++ preprocessor to give a single complete C++ file which demonstrates + the problem, along with the `include_cpp!` directive you use. + Alternatively, run your build using `AUTOCXX_REPRO_CASE=repro.json` which should + put everything we need into `output.h`. If necessary, you can use the `CLANG_PATH` + or `CXX` environment variables to specify the path to the Clang compiler to use. +* Failing all else, build using + `cargo clean -p && RUST_LOG=autocxx_engine=info cargo build -vvv` + and send the _entire_ log to us. This will include two key bits of logging: + the C++ bindings as distilled by `bindgen`, and then the version which + we've converted and moulded to be suitable for use by `cxx`. + +## Bugs related to linking problems + +Unfortunately, _linking_ C++ binaries is a complex area subject in itself, and +we won't be able to debug your linking issues by means of an autocxx bug report. +Assuming you're using autocxx's build.rs support, the actual C++ build and +managed by the [`cc`](https://crates.io/crates/cc) crate. You can find +many of its options on its [`Build` type](https://docs.rs/cc/latest/cc/struct.Build.html). +If you need to bring in an external library, you may also need to emit certain +[print statements from your `build.rs`](https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-lib) +to instruct cargo to link against that library. \ No newline at end of file