Support `-Cpanic=unwind` (on nightly), by just changing documentation.
Crubit is made sound for free by the changes in nightly to abort when panicking through `extern "C"`. These are documented here: https://doc.rust-lang.org/nomicon/ffi.html#ffi-and-unwinding -- however, note that this functionality has not yet hit Rust stable. The documentation change was made too soon, as far as I can tell. Still, this _will_ be the behavior, and when it is, we will be sound.
For `extern "C-unwind"`, I think our docs here were inaccurate, and it's not sound unless the caller enables exceptions. I suspect we should not release this functionality to end-users yet, as it's dangerous. Maybe we should instead default to the sound option of aborting.
We probably shouldn't say the issue of `panic=unwind` support is "solved" until we both remove the `-Cpanic=abort`, and the behavior reaches a stable release of rustc.
PiperOrigin-RevId: 663349913
Change-Id: I1ab8cb1131c038a023fceae123927eb22c1d0a0f
diff --git a/cc_bindings_from_rs/README.md b/cc_bindings_from_rs/README.md
index e94f72c..b2f0c08 100644
--- a/cc_bindings_from_rs/README.md
+++ b/cc_bindings_from_rs/README.md
@@ -21,8 +21,7 @@
--clang-format-exe-path=<path_of_clang_format_executable> -- \
--rustfmt-exe-path=<path_of_rustfmt_executable> -- \
$HOME/scratch/test.rs \
- --crate-type=lib \
- --codegen=panic=abort
+ --crate-type=lib
$ cat $HOME/scratch/test.h
// Automatically @generated C++ bindings for the following Rust crate:
diff --git a/cc_bindings_from_rs/bazel_support/cc_bindings_from_rust_rule.bzl b/cc_bindings_from_rs/bazel_support/cc_bindings_from_rust_rule.bzl
index 3391903..dc545ba 100644
--- a/cc_bindings_from_rs/bazel_support/cc_bindings_from_rust_rule.bzl
+++ b/cc_bindings_from_rs/bazel_support/cc_bindings_from_rust_rule.bzl
@@ -135,7 +135,7 @@
# can be interleaved with rustc flags in any order, and if we used _cc_bindings_from_rs_tool
# as the tool_path for construct_arguments, then this could be `args.all` instead.
- # TODO(b/254049425): We shouldn't override the panic arg, and instead work fine in any case.
+ # TODO(b/254049425): Remove `-Cpanic=abort` after crosstool contains cl/657372371.
arguments = [args.process_wrapper_flags, "--", ctx.executable._cc_bindings_from_rs_tool.path, crubit_args, "--", args.rustc_flags, "-Cpanic=abort"],
)
diff --git a/cc_bindings_from_rs/bindings.rs b/cc_bindings_from_rs/bindings.rs
index f88d573..d566e14 100644
--- a/cc_bindings_from_rs/bindings.rs
+++ b/cc_bindings_from_rs/bindings.rs
@@ -115,10 +115,6 @@
pub fn generate_bindings(db: &Database) -> Result<Output> {
let tcx = db.tcx();
- match tcx.sess().panic_strategy() {
- PanicStrategy::Unwind => bail!("No support for panic=unwind strategy (b/254049425)"),
- PanicStrategy::Abort => (),
- };
let top_comment = {
let crate_name = tcx.crate_name(LOCAL_CRATE);
@@ -1195,25 +1191,20 @@
/// Otherwise returns an error the describes why the thunk is needed.
fn is_thunk_required(sig: &ty::FnSig) -> Result<()> {
match sig.abi {
- // "C" ABI is okay: Before https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html a
- // Rust panic that "escapes" a "C" ABI function leads to Undefined Behavior. This is
- // unfortunate, but Crubit's `panics_and_exceptions.md` documents that `-Cpanic=abort`
- // is the only supported configuration.
- //
- // After https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html a Rust panic that
- // tries to "escape" a "C" ABI function will terminate the program. This is okay.
+ // "C" ABI is okay: since https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html has been
+ // accepted, a Rust panic that "escapes" a "C" ABI function is a defined crash. See
+ // https://doc.rust-lang.org/nomicon/ffi.html#ffi-and-unwinding.
rustc_target::spec::abi::Abi::C { unwind: false } => (),
- // "C-unwind" ABI is okay: After
- // https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html a new "C-unwind" ABI may be
- // used by Rust functions that want to safely propagate Rust panics through frames that
- // may belong to another language.
+ // This requires a thunk if the calling C++ frames use `-fno-exceptions`, as it is
+ // UB. However, we leave this to the caller: if you use `extern "C-unwind"`, we assume you
+ // know what you are doing and do not block you from integrating with exception-enabled C++.
rustc_target::spec::abi::Abi::C { unwind: true } => (),
// All other ABIs trigger thunk generation. This covers Rust ABI functions, but also
// ABIs that theoretically are understood both by C++ and Rust (e.g. see
// `format_cc_call_conv_as_clang_attribute` in `rs_bindings_from_cc/src_code_gen.rs`).
- _ => bail!("Calling convention other than `extern \"C\"` requires a thunk"),
+ _ => bail!("Any calling convention other than `extern \"C\"` requires a thunk"),
};
ensure!(is_c_abi_compatible_by_value(sig.output()), "Return type requires a thunk");
@@ -7758,7 +7749,7 @@
(
"fn(i32) -> i32", // TyKind::FnPtr (default ABI = "Rust")
"Function pointers can't have a thunk: \
- Calling convention other than `extern \"C\"` requires a thunk",
+ Any calling convention other than `extern \"C\"` requires a thunk",
),
(
"extern \"C\" fn (SomeStruct, f32) -> f32",
diff --git a/cc_bindings_from_rs/cc_bindings_from_rs.rs b/cc_bindings_from_rs/cc_bindings_from_rs.rs
index 5560d8c..2abae7c 100644
--- a/cc_bindings_from_rs/cc_bindings_from_rs.rs
+++ b/cc_bindings_from_rs/cc_bindings_from_rs.rs
@@ -437,21 +437,27 @@
Ok(())
}
- /// `test_rustc_unsupported_panic_mechanism` tests that `panic=unwind`
- /// results in an error.
+ /// `test_rustc_with_panic_unwind` tests that `panic=unwind`
+ /// is supported at least nominally.
///
/// This is tested at the `cc_bindings_from_rs.rs` level instead of at the
/// `bindings.rs` level, because `run_compiler_test_support` doesn't
/// support specifying a custom panic mechanism.
#[test]
- fn test_rustc_unsupported_panic_mechanism() -> Result<()> {
- let err = TestArgs::default_args()?
+ fn test_rustc_with_panic_unwind() -> Result<()> {
+ let _ = TestArgs::default_args()?
.with_panic_mechanism("unwind")
.run()
- .expect_err("panic=unwind should trigger an error");
+ .expect("panic=unwind should not cause an error");
+ Ok(())
+ }
- let msg = format!("{err:#}");
- assert_eq!("No support for panic=unwind strategy (b/254049425)", msg);
+ #[test]
+ fn test_rustc_with_panic_abort() -> Result<()> {
+ let _ = TestArgs::default_args()?
+ .with_panic_mechanism("abort")
+ .run()
+ .expect("panic=abort should not cause an error");
Ok(())
}
diff --git a/cc_bindings_from_rs/cc_bindings_from_rs_sh_test.sh b/cc_bindings_from_rs/cc_bindings_from_rs_sh_test.sh
index 620a7fd..c2b8740 100755
--- a/cc_bindings_from_rs/cc_bindings_from_rs_sh_test.sh
+++ b/cc_bindings_from_rs/cc_bindings_from_rs_sh_test.sh
@@ -60,8 +60,7 @@
\"$RS_INPUT_PATH\" \
--crate-type=lib \
--sysroot=$SYSROOT_PATH \
- $(rustc_target_arg) \
- --codegen=panic=abort" \
+ $(rustc_target_arg)" \
"Expecting that this invocation of cc_bindings_from_rs will succeed"
EXPECT_STR_EMPTY "$(cat $STDOUT_PATH)"
@@ -146,8 +145,7 @@
\"$RS_INPUT_PATH\" \
--crate-type=lib \
--sysroot=$SYSROOT_PATH \
- $(rustc_target_arg) \
- --codegen=panic=abort" \
+ $(rustc_target_arg)" \
"Invalid --h-out path should result in non-0 exit code"
EXPECT_STR_EMPTY "$(cat $STDOUT_PATH)"
@@ -192,8 +190,7 @@
\"$RS_INPUT_PATH\" \
--crate-type=lib \
$(rustc_target_arg) \
- --sysroot=$SYSROOT_PATH \
- --codegen=panic=abort" \
+ --sysroot=$SYSROOT_PATH" \
"Expecting that this invocation of cc_bindings_from_rs will succeed"
EXPECT_STR_EMPTY "$(cat $STDOUT_PATH)"
diff --git a/cc_bindings_from_rs/run_compiler_test_support.rs b/cc_bindings_from_rs/run_compiler_test_support.rs
index 1ec8269..adf2ae0 100644
--- a/cc_bindings_from_rs/run_compiler_test_support.rs
+++ b/cc_bindings_from_rs/run_compiler_test_support.rs
@@ -99,12 +99,6 @@
("warnings".to_string(), rustc_lint_defs::Level::Deny),
("stable_features".to_string(), rustc_lint_defs::Level::Allow),
],
- cg: CodegenOptions {
- // As pointed out in `panics_and_exceptions.md` the tool only supports `-C
- // panic=abort` and therefore we explicitly opt into this config for tests.
- panic: Some(rustc_target::spec::PanicStrategy::Abort),
- ..Default::default()
- },
..Default::default()
};
diff --git a/cc_bindings_from_rs/test/bazel/unit_tests/generating_files/generating_files_test.bzl b/cc_bindings_from_rs/test/bazel/unit_tests/generating_files/generating_files_test.bzl
index a53a006..3dfe6bc 100644
--- a/cc_bindings_from_rs/test/bazel/unit_tests/generating_files/generating_files_test.bzl
+++ b/cc_bindings_from_rs/test/bazel/unit_tests/generating_files/generating_files_test.bzl
@@ -195,11 +195,6 @@
"--crate-type=rlib" in cmdline,
"Expected to find `--crate-type=rlib` on the command line, got {}.".format(cmdline),
)
- asserts.true(
- env,
- "-Cpanic=abort" in cmdline,
- "Expected to find `-Cpanic=abort` on the command line, got {}.".format(cmdline),
- )
# ":emptylib" is a dependency of the crate for which we are generating bindings.
# Similarly to how we pass `--extern=emptylib` to the command line for the `Rustc`
diff --git a/cc_bindings_from_rs/test/unwinding/BUILD b/cc_bindings_from_rs/test/unwinding/BUILD
new file mode 100644
index 0000000..c56084d
--- /dev/null
+++ b/cc_bindings_from_rs/test/unwinding/BUILD
@@ -0,0 +1,32 @@
+load(
+ "@rules_rust//rust:defs.bzl",
+ "rust_library",
+)
+load(
+ "//cc_bindings_from_rs/bazel_support:cc_bindings_from_rust_rule.bzl",
+ "cc_bindings_from_rust",
+)
+load("//common:crubit_wrapper_macros_oss.bzl", "crubit_cc_test")
+
+package(default_applicable_licenses = ["//:license"])
+
+rust_library(
+ name = "panic_function",
+ testonly = 1,
+ srcs = ["panic_function.rs"],
+)
+
+cc_bindings_from_rust(
+ name = "panic_function_cc_api",
+ testonly = 1,
+ crate = ":panic_function",
+)
+
+crubit_cc_test(
+ name = "unwinding_test",
+ srcs = ["unwinding_test.cc"],
+ deps = [
+ ":panic_function_cc_api",
+ "@com_google_googletest//:gtest_main",
+ ],
+)
diff --git a/cc_bindings_from_rs/test/unwinding/panic_function.rs b/cc_bindings_from_rs/test/unwinding/panic_function.rs
new file mode 100644
index 0000000..2a2585e
--- /dev/null
+++ b/cc_bindings_from_rs/test/unwinding/panic_function.rs
@@ -0,0 +1,11 @@
+// Part of the Crubit project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+pub fn panic_rust() {
+ panic!("this is a panic");
+}
+
+pub extern "C" fn panic_c() {
+ panic!("this is a panic");
+}
diff --git a/cc_bindings_from_rs/test/unwinding/unwinding_test.cc b/cc_bindings_from_rs/test/unwinding/unwinding_test.cc
new file mode 100644
index 0000000..014a734
--- /dev/null
+++ b/cc_bindings_from_rs/test/unwinding/unwinding_test.cc
@@ -0,0 +1,26 @@
+// Part of the Crubit project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "cc_bindings_from_rs/test/unwinding/panic_function_cc_api.h"
+
+namespace {
+
+using ::testing::AllOf;
+using ::testing::HasSubstr;
+
+TEST(PanicTest, PanicRust) {
+ EXPECT_DEATH(panic_function::panic_rust(),
+ AllOf(HasSubstr("this is a panic"),
+ HasSubstr("panic in a function that cannot unwind")));
+}
+
+TEST(PanicTest, PanicC) {
+ EXPECT_DEATH(panic_function::panic_c(),
+ AllOf(HasSubstr("this is a panic"),
+ HasSubstr("panic in a function that cannot unwind")));
+}
+
+} // namespace
diff --git a/docs/overview/panics_and_exceptions.md b/docs/overview/panics_and_exceptions.md
index 960dd49..4dcffed 100644
--- a/docs/overview/panics_and_exceptions.md
+++ b/docs/overview/panics_and_exceptions.md
@@ -1,82 +1,61 @@
# Panics and Exceptions in C++/Rust FFI bindings
-This document explains what Crubit's `rs_bindings_from_cc` and
-`cc_bindings_from_rs` tools do to handle Rust panics and C++ exceptions.
+SUMMARY: Crubit currently requires `-fno-exceptions`, and converts unwinding
+panics into aborts.
-## Aborting on panic
+## Unwinding and Aborting
> "Alright so I'm panicking, what else is there to do?" \
> -- "The Hitchhiker’s Guide to the Galaxy" by Douglas Adams
-Rust libraries built with
-[`-Cpanic=abort`](https://doc.rust-lang.org/rustc/codegen-options/index.html#panic)
-terminate their process upon panic and there isn't much else to do. Similarly,
-C++ libraries built with Clang's
-[`-fno-exceptions`](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fexceptions)
-flag terminate the process when a C++ exception is thrown.
+Rust and C++ both support a form of unwinding exception/panic, where ordinary
+control flow is terminated, and instead control proceeds up the stack, calling
+destructors along the way, until it is caught, converted to a termination, or
+completely unwinds the stack (which also leads to termination).
-In build environments that use the flags above, Rust panics and C++ exceptions
-don't unwind the callstack looking for appropriate handlers. This means that
-bindings generated by Crubit's `rs_bindings_from_cc` and `cc_bindings_from_rs`
-tools don't need to do anything special to handle panics and/or exceptions. In
-particular, Rust calls into C++ (and C++ calls into Rust) can just use the "C"
-ABI and assume that no panics and no exceptions will ever need to unwind across
-the FFI boundary.
+This has a performance cost (in that data must be managed soundly, destroyed if
+appropriate, in case of unwinding), and a code complexity cost (in that unsafe
+code must handle unexpected control flow edges soundly), so implementations of
+both languages allow for unwinding to be disabled, in favor of immediate process
+termination:
-Currently Crubit-generated bindings only support `-Cpanic=abort`,
-`-fno-exceptions` environment. See
-[the "Exceptions" section in the Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html#Exceptions)
-for discussion of some of the pros and cons of an `-fno-exceptions` environment.
+* Rust:
+ [`-Cpanic=abort`](https://doc.rust-lang.org/rustc/codegen-options/index.html#panic)
+* C++
+ [`-fno-exceptions`](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fexceptions)
-## Cross-language unwinding
+Crubit, and any FFI, will work if unwinding is disabled for both sides of the
+FFI boundary. But if it is enabled for one or both sides, and an exception or
+panic unwinds past an FFI boundary, we need special support to ensure that the
+behavior is defined.
-TODO(b/254049425): Add support for cross-FFI unwinding of Rust panics and C++
-exceptions.
+## Supported Configurations
> "Who said anything about panicking?" snapped Arthur. "This is still just the
> culture shock. You wait till I've settled down into the situation and found my
> bearings. Then I'll start panicking." \
> -- "The Hitchhiker’s Guide to the Galaxy" by Douglas Adams
-Crubit doesn't currently support unwinding of Rust panics or C++ exceptions
-across the FFI boundary. In other words, the generated bindings are only safe
-when `-Cpanic=abort` and `-fno-exceptions` are used - other configurations may
-lead to Undefined Behavior:
+**Rust:** Rust: Crubit can create bindings for libraries built with either
+`-Cpanic=abort` or `-Cpanic=unwind`.
-* C++ exception unwinding through Rust frames leads to Undefined Behavior
- (based on
- [this part](https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html#changes-to-the-behavior-of-existing-abi-strings)
- of RFC 2945).
+If a panic unwinds past a Crubit FFI boundary, the process will terminate
+(on rustc nightly[^terminate_requirements]), with
+the **sole** exception of `extern "C-unwind"` functions. If you define an
+`extern "C-unwind"` function, you must ensure that it is only called by C++ code
+which enables exceptions. This responsibility is left to the caller.
-* Rust panic unwinding through C++ frames leads to Undefined Behavior. Note
- that [RFC 2945](https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html)
- defines some of that behavior: "with the `panic=unwind` runtime, `panic!`
- will cause an abort if it would otherwise "escape" from a function defined
- with `extern "C"`."
+**C++:** Crubit can create bindings for libraries built with `-fno-exceptions`.
+We do not generate `extern "C-unwind"` interfaces which could propagate a C++
+exception, and the behavior is undefined if an exception propagates past a
+Crubit FFI boundary. (See b/200067087 for catching this at compile time.)
-Below is an incomplete, tentative list of steps needed to add Crubit support for
-cross-language unwinding of panics and exceptions:
+<!-- TODO(b/200067087): fail when exceptions are enabled, document above. -->
-* Support building Crubit's automated tests with `-Cpanic=unwind` and/or
- `-fexceptions`. Use this support to add test coverage for:
+[^terminate_requirements]: We use the behavior of
+ https://doc.rust-lang.org/nomicon/ffi.html#ffi-and-unwinding
+ to cause a crash. However, this is not yet
+ incorporated into a stable Rust release, and requires
+ nightly.
- - Propagating a Rust panic across C++ frames and catching it in another
- Rust frame.
- - Propagating a C++ exception across Rust frames and catching it in
- another C++ frame.
-
-* If one or both languages don't abort on unwind, then modify the generated
- Rust code to use the new `"C-unwind"` ABI string proposed by
- [RFC 2945](https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html) (instead
- of using the old `"C"` ABI string):
-
- - `rs_bindings_from_cc` should use `"C-unwind"` ABI when declaring C++
- thunks in `mod detail` in the generated `..._rs_api.rs`
- - `cc_bindings_from_rs` should use `"C-unwind"` ABI when defining C++
- thunks in `..._cc_api_impl.rs`
-
-* Investigate if Crubit needs to modify the generated C++ code. For example:
-
- - C++ thunk definitions generated by `rs_bindings_from_cc` in
- `..._rs_api_impl.cc`.
- - C++ declarations generated by `cc_bindings_from_rs` in `..._cc_api.h`.
+<!-- TODO: remove above note once crash reaches stable. -->