Skip to content

Conversation

anforowicz
Copy link
Contributor

PTAL?

This PR fixes https://crbug.com/433254489 (/cc @ShabbyX as FYI - IIUC they've patched this PR onto their local machine and verified that it does indeed unblock them).

The first commit fixes some existing code duplication and prevents more code duplication around how fn syntax::parse_items consumes a syntax::Module. I think this commit is okay and moves things in a reasonable direction, but I acknowledge that adding more partial consumption/destructuring/taking is a bit icky. More discussion about this aspect of the changes can be found in a comment from a semi-internal review at anforowicz#2 (comment) and anforowicz#2 (comment).

I also note that some existing variants of syntax::Api enum have Rust and C++ versions - e.g. RustFunction and CxxFunction as well as RustType and CxxType. We could also do that for the TypeAlias variant which this PR doesn't touch just yet. Please provide feedback on whether I should also submit an additional commit into this or a separate PR - see https://github.com/anforowicz/cxx/tree/api-rust-type-alias-separate-variant

/cc @zetafunction who has kindly provided initial, semi-internal feedback at anforowicz#2

@@ -163,3 +163,46 @@ mod ffi {

Bounds on a lifetime (like `<'a, 'b: 'a>`) are not currently supported. Nor are
type parameters or where-clauses.

## Reusing existing binding types
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing an existing defined pair of C++ and Rust type is already supported. I don't understand the distinction between the extern C++ type aliases which are already supported, and the extern Rust type aliases in this PR. What is the situation where it would be correct to write extern "Rust" { type T = path::to::U; } and not extern "C++" { type T = path::to::U; }?

Copy link
Contributor Author

@anforowicz anforowicz Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing an existing defined pair of C++ and Rust type is already supported. I don't understand the distinction between the extern C++ type aliases which are already supported, and the extern Rust type aliases in this PR. What is the situation where it would be correct to write extern "Rust" { type T = path::to::U; } and not extern "C++" { type T = path::to::U; }?

In the PR, in the end-to-end test under tests/ffi/... the CrossModuleRustType is a Rust type, rather than a C++ type. This is why extern "C++" { type T = path::to::U; } will not work and we need to support extern "Rust" { type T = path::to::U; }.

Let me consider different scenarios:

  • If Foo is a C++ type (declared in another #[cxx::bridge]'s extern "C++" section) then extern "C++" { type Foo = crate::some::other::module::Foo; } will indeed work.
  • If Foo is a Rust type (declared in another Rust module, and also covered by another #[cxx::bridge]) then:
    • extern "C++" { type Foo = crate::some::other::module::Foo; } will not work, because Foo is a Rust type rather than a C++ type/binding. In particular, if I change tests/ffi/lib.rs in the PR to put the type alias in an extern "C++" section, then cargo test rightfully fails saying: "the trait bound module::CrossModuleRustType: ExternType is not satisfied".
    • I can't just say extern "Rust" { type Foo; /* ... */ } and have type Foo = crate::some::other::module::Foo outside of cxx::bridge, because this will error out with: "conflicting implementations of trait RustType for type module::CrossModuleRustType" (or Foo, this error message was captured by tweaking the end-to-end tests from the PR).

verify
verify
}
Lang::Rust => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not sufficient for safety. Consider adding:

        // inside extern "Rust"
        type Any = crate::module::CrossModuleRustType;
        fn repro(bad: &mut CrossModuleRustType) -> &mut Any;
...

fn repro(bad: &mut ffi::CrossModuleRustType) -> &mut ffi::Any {
    bad
}

Nothing here enforces what type the C++ Any is. We can make it anything:

namespace tests {
using Any = std::array<char, 1000>;
}

and arbitrarily stomp on memory.

repro(*r_boxed_cross_module_rust_type(123)).fill('?');
     Running tests/test.rs (target/debug/deps/test-f2f5723622a08a56)

running 15 tests
free(): invalid next size (fast)
malloc(): corrupted top size
error: test failed, to rerun pass `--test test`

Caused by:
  process didn't exit successfully: `target/debug/deps/test-f2f5723622a08a56` (signal: 6, SIGABRT: process abort signal)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you say that Any is a C++ type. It is a Rust type, right? (by virtue of ::cxx::private::verify_rust_type which ensures that T: RustType which means the aliased type must have been used in another cxx::bridge's extern "Rust" section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, if I change the new tests/ffi/lib.rs snippet from the PR to:

extern "Rust" {
    type CrossModuleRustType = crate::ffi::C;     // `C` is a C++ type - it won't work here.
    fn r_get_value_from_cross_module_rust_type(
        value: &CrossModuleRustType,              
    ) -> i32;
}                                                 

then cargo test expectedly reports an error that says this is disallowed: "the trait bound ffi::C: RustType is not satisfied".


Maybe I just have trouble figuring out what scenario you have in mind. If the error above (i.e. "free(): invalid next size" and "malloc(): corrupted top size") can be reproed, then I would really appreciate if you could share the repro as a commit on top/after my PR?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the error above (i.e. "free(): invalid next size" and "malloc(): corrupted top size") can be reproed, then I would really appreciate if you could share the repro as a commit on top/after my PR?

This is what I mean: anforowicz#3. This would need to not compile in order for the implementation to be sound.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That helped. I understand the problem now.

The specific scenario that you've pointed out should now be fixed in the latest version of the PR. The fix is to check on C++ side that the type alias is derived from ::rust::Opaque. OTOH, the fix doesn't seem sufficient to solve the general problem - transmuting into an unrelated type seems still posslble with something like:

#[cxx::bridge]
mod ffi {
    extern "Rust" {
        type RustType;
        type OtherRustType;

        #[cxx_name = "OtherRustType"]
        type RustAlias = RustType;

        fn bad_transmute(x: &RustType) -> &RustAlias;
        fn do_something_with_other_rust_type(x: &OtherRustType);
    }
}

To prevent the above, we'd need to assert on C++ side not only that RustAlias aliases an opaque Rust type, but that it aliases the same, specific Rust type. In other words, this probably requires a C++ equivalent of ExternType::Id checks from Rust side.

Q1: Does the above sound more or less correct to you?

Q2: Do you have suggestions on how to implement the C++ equivalent of ExternType::Id checks? Some initial thoughts below:

  • It seems that the type id of a Rust type should include the type name, the module path, and the crate name. The latter seems difficult, because 1) IIUC cxxbridge does not currently know/get the crate name, and 2) crate names can be aliased.
  • One mechanism to add a mechanism for type id checks would be to tweak write_opaque_type so that the generated struct has an extra field like so:
#include <cstring>

struct S /* : ::rust::Opaque */ {
    static constexpr const char* __cxxbridge1_rust_type_id = "module::path::RustType";
};

static_assert(
    0 == strcmp(S::__cxxbridge1_rust_type_id, "module::path::OtherRustType"),
    "blah"
);

(having a new public field is a bit icky but is not the end of the world; one alternative would be to introduce template parameters into ::rust::Opaque, but changing this type should probably be treated as a breaking change which seems to make this approach undesirable)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel that all the use cases of this functionality are already well supported, with all the static checking required for soundness, by extern "C++". An extern "C++" type alias means "I already have a C++ definition of a type (the left-hand side) and a Rust definition of a type (right-hand side) that refer to the same type". This is verified in Rust via ExternType::Id and redoing an equivalent check on the C++ side is unnecessary.

If I wipe out all the changes in {gen,macro,src,syntax} from this PR, but keep the changes in tests/, with this conversion to extern "C++" the new tests pass:

--- a/tests/ffi/lib.rs
+++ b/tests/ffi/lib.rs
@@ -370,8 +370,11 @@ pub mod ffi {
     impl SharedPtr<Private> {}
     impl UniquePtr<Array> {}
 
-    extern "Rust" {
+    extern "C++" {
         type CrossModuleRustType = crate::module::CrossModuleRustType;
+    }
+
+    extern "Rust" {
         fn r_get_value_from_cross_module_rust_type(value: &CrossModuleRustType) -> i32;
     }
 }
--- a/tests/ffi/module.rs
+++ b/tests/ffi/module.rs
@@ -82,6 +82,7 @@ pub mod ffi2 {
 #[cxx::bridge(namespace = "tests")]
 pub mod ffi3 {
     extern "Rust" {
+        #[derive(ExternType)]
         type CrossModuleRustType;
 
         #[allow(clippy::unnecessary_box_returns)]

In 2.x it would be a good idea to provide ExternType impls for every opaque Rust type automatically, but this is a breaking change because they conflict with handwritten impls which may currently exist.

Other than that I remain unsure what is left to implement. Would it be less confusing if type aliases are placed at the module level outside of extern? Or if RustType is changed to carry an Id associated type (and potentially made public)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected that treating a Rust type as an opaque C++ type will lead to considerable trouble, but so far I can only think of Box<CrossModuleRustType> as one specific broken scenario. fn check_type_box explicitly excludes cx.types.aliases from its checks, but it seems that CrossModuleRustType type still cannot be used as T in Box or Vec - after adding fn c_boxed_cross_module_rust_type() -> Box<CrossModuleRustType>; to extern "C++" { ... } (or to extern "Rust" { ... }) I get:

cxxbridge/sources/tests/ffi/lib.rs.cc:1489:58: error: static assertion failed: type tests::CrossModuleRustType should be trivially move constructible and trivially destructible in C++ to be used as type Box<CrossModuleRustType> in Rust
warning: [email protected]:  1489 |     ::rust::IsRelocatable<::tests::CrossModuleRustType>::value,
warning: [email protected]:       |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~

After tweaking fn write_opaque_type to emit using IsRelocatable = std::true_type; I can get past the error above, but then another error happens:

> type mismatch resolving `<CrossModuleRustType as ExternType>::Kind == Trivial`

This error can be worked around by tweaking fn expand_rust_type_impl to emit type Kind = ::cxx::kind::Trivial (rather than Opaque). But then this also doesn't work with an error that suggests that Box<T> wouldn't work even with my original approach:

> symbol `tests$cxxbridge1$r_boxed_cross_module_rust_type` is already defined

Interestingly &mut T works fine if I retain the IsRelocatable and kind::Trivial tweaks. So maybe this is indeed a better way forward compared to my original approach.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Box and Vec case is solved in #1605, &mut T in #1610, and &[T]/&mut [T] in #1613.

`syntax::parse::parse_items` is currently called from two places:
from `macro/src/expand.rs` and from `gen/src/mod.rs`.  (In the future
it may be also called from a `syntax/test_support.rs` helper.)

Before this commit, all those places had to destructure/interpret
a `syntax::file::Module` into `content`, `namespace`, and `trusted`.

After this commit, this destructuring/interpretation is deduplicated
into `syntax::parse::parse_items`.  This requires some minor gymnastics:

* `gen/src/mod.rs` has to call `std::mem::take(&mut bridge.attrs)`
  instead of doing a partial destructuring and passing `bridge.attrs`
  directly.  This is an extra complexity, but we already do the same
  thing in `macro/src/expand.rs` before this commit, so hopefully this
  is ok.
* `fn parse_items` takes `&mut Module` rather than `Module` and
  "consumes" / `std::mem::take`s `module.content`, because
  `macro/src/expand.rs` needs to retain ownership of other `Module`
  fields.  This also seems like an unfortunate extra complexity, but
  (again) before this commit we already do this for `bridge.attrs` in
  `macro/src/expand.rs`.
Before this commit `fn parse_apis` in `syntax/test_support.rs` would
ignore the namespace in `#[cxx::bridge(namespace = "ignored")]`.
In other words, the new test added in `syntax/namespace.rs` would
fail before this commit.  This commit fixes this.

At a high-level this commit:

* Moves two pieces of code from `gen/src/file.rs` into `syntax/...`
  to make them reusable from `syntax/test_support.rs`:
    - `fn find_cxx_bridge_attr` (moved into `syntax/attrs.rs`)
    - `Namespace::parse_attr` (moved into
      `syntax/namespace.rs`)
* Reuses these pieces of code from `syntax/test_support.rs`
* Renames `Namespace::parse_bridge_attr_namespace` to
  `Namespace::parse_stream` so that all 3 parse methods are named
  after their input: `parse_attr`, `parse_stream`, `parse_meta`.
* Adds a `syntax/`-level unit test that verifies that the namespace
  is indeed getting correctly parsed and propagated
@anforowicz anforowicz force-pushed the reusing-rust-type-binding branch from 8dbb7b2 to ab7b1cd Compare September 4, 2025 20:21
@ShabbyX
Copy link

ShabbyX commented Sep 4, 2025

@dtolnay (regarding #1539 (comment) so I don't intrude on that thread), the use case I have that doesn't work with existing cxx is this:

  • I have a(n old) C++ library, it's a part of a bigger project
  • I'm replacing parts of it with a (new) Rust implementation. Parts of it, because it's a big project that I need to replace piecemeal
  • In Rust, I have a type that represents the main object of the library: struct IR
  • I need to pass this Rust object (in a Box) to C++ (say, in phase 1 (that is parse)), so cxx in one file defines extern Rust { type IR; }
  • I need to receive this Rust object from C++ (say, in phase 2 (that is codegen)), so cxx in another file defines extern Rust { type IR; }

Note again that IR is a Rust type.

This results in a build error:

error[E0119]: conflicting implementations of trait `RustType` for type `ir::IR`
    --> ../../src/compiler/translator/ir/src/compile.rs:54:9
     |
54   |         type IR;
     |         ^^^^^^^ conflicting implementation for `ir::IR`
     |
    ::: ../../src/compiler/translator/ir/src/builder.rs:2949:9
     |
2949 |         type IR;
     |         ------- first implementation here

error[E0119]: conflicting implementations of trait `ImplBox` for type `ir::IR`
    --> ../../src/compiler/translator/ir/src/compile.rs:59:17
     |
59   |             ir: Box<IR>,
     |                 ^^^^^^ conflicting implementation for `ir::IR`
     |
    ::: ../../src/compiler/translator/ir/src/builder.rs:2952:64
     |
2952 |         fn builder_finish(mut builder: Box<BuilderWrapper>) -> Box<IR>;
     |                                                                ------ first implementation here

If I attempt to alias them with type IR = crate::builder::ffi::IR;, I get:

error[cxxbridge]: type alias in extern "Rust" block is not supported
   ┌─ ../../src/compiler/translator/ir/src/compile.rs:56:9
   │
56 │         type IR = crate::builder::ffi::IR;
   │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type alias in extern "Rust" block is not supported

So in response to "Other than that I remain unsure what is left to implement", do you have a suggestion on how to support this use case? Ideally, I wouldn't have to put all the (huge) cxx code from multiple files into one as a workaround.

@anforowicz
Copy link
Contributor Author

So in response to "Other than that I remain unsure what is left to implement", do you have a suggestion on how to support this use case? Ideally, I wouldn't have to put all the (huge) cxx code from multiple files into one as a workaround.

I think the summary is that:

  • Your type IR = crate::builder::ffi::IR; should (a bit counterintuitevly) go into extern "C++" section (rather than extern "Rust" as you've originally tried, and as I've originally tried to support in this PR)
  • You also need to add #[derive(ExternType)] above your original type IR definition (in the builder::ffi module).

This should work (as pointed out in #1539 (comment)) although there may be some rough edges around support for Box<IR> and/or &mut IR (see #1539 (comment)).

If the above is hard to understand and/or apply, then maybe you can share a link to your WIP Gerrit CL - this would let us comment directly on your CL / on lines that need to be tweaked.

@ShabbyX
Copy link

ShabbyX commented Sep 5, 2025

Thanks. So the link is already there in my message above, but here it is again: https://chromium-review.googlesource.com/c/angle/angle/+/5898761. In the message above, I linked to the three pieces that matter (the struct IR definition, and the type IR; declarations in the cxx modules.

And here are some tries:

Try 1

builder.rs has this:

    extern "Rust" {
        #[derive(ExternType)]
        type IR;

compile.rs has this:

    extern "C++" {
        type IR = crate::builder::ffi::IR;

I get:

gen/src/compiler/translator/ir/src/compile.rs.cc:930:42: error: no member named 'IR' in namespace 'sh::ir::ffi'
  930 |     ::rust::IsRelocatable<::sh::ir::ffi::IR>::value,
      |                                          ^~
gen/src/compiler/translator/ir/src/compile.rs.cc:937:55: error: no type named 'IR' in namespace 'sh::ir::ffi'
  937 | void sh$ir$ffi$cxxbridge1$generate_ast(::sh::ir::ffi::IR *ir, ::sh::TCompiler *compiler, ::sh::ir::ffi::CompileOptions const &options, ::sh::ir::ffi::Output *return$) noexcept;
      |                                        ~~~~~~~~~~~~~~~^
gen/src/compiler/translator/ir/src/compile.rs.cc:940:63: error: no member named 'IR' in namespace 'sh::ir::ffi'
  940 | ::sh::ir::ffi::Output generate_ast(::rust::Box<::sh::ir::ffi::IR> ir, ::sh::TCompiler *compiler, ::sh::ir::ffi::CompileOptions const &options) noexcept {
      |                                                               ^~

Try 2

builder.rs has this:

    extern "C++" {
        #[derive(ExternType)]
        type IR;
    }

I get:

error[cxxbridge]: derive(ExternType) on opaque C++ type is not supported yet
     ┌─ ../../src/compiler/translator/ir/src/builder.rs:2948:18
     │
2948 │         #[derive(ExternType)]
     │                  ^^^^^^^^^^ derive(ExternType) on opaque C++ type is not supported yet

I understood your instructions to be what I did in Try 1. Perhaps we can sync on this over Chat, see what we can do to fix it.

@anforowicz
Copy link
Contributor Author

@ShabbyX, I don't know how to upload a new patchset for your CL. But the initial required delta that dtolnay suggests is this (also uploaded as https://chromium-review.googlesource.com/c/angle/angle/+/6918105) looks like this:

$ git diff
diff --git a/src/compiler/translator/ir/src/builder.rs b/src/compiler/translator/ir/src/builder.rs
index 3477b2c1ba..9872488748 100644
--- a/src/compiler/translator/ir/src/builder.rs
+++ b/src/compiler/translator/ir/src/builder.rs
@@ -2946,6 +2946,8 @@ pub mod ffi {

     extern "Rust" {
         type BuilderWrapper;
+
+        #[derive(ExternType)]
         type IR;

         fn builder_new(shader_type: ASTShaderType) -> Box<BuilderWrapper>;
diff --git a/src/compiler/translator/ir/src/compile.rs b/src/compiler/translator/ir/src/compile.rs
index de24291ac7..933814098a 100644
--- a/src/compiler/translator/ir/src/compile.rs
+++ b/src/compiler/translator/ir/src/compile.rs
@@ -48,15 +48,14 @@ mod ffi {

         #[namespace = "sh"]
         type TIntermBlock = crate::output::legacy::ffi::TIntermBlock;
+
+        include!("compiler/translator/ir/src/builder.rs.h");
+        type IR = crate::builder::IR;
     }
-    extern "Rust" {
-        // TODO: doesn't compile, says conflicting type with builder's type.
-        type IR;
-        // TODO: doesn't compile, alias not supported in extern "Rust".
-        //type IR = crate::builder::ffi::IR;

+    extern "Rust" {
         unsafe fn generate_ast(
-            ir: Box<IR>,
+            ir: &mut IR,
             compiler: *mut TCompiler,
             options: &CompileOptions,
         ) -> Output;
@@ -64,13 +63,13 @@ mod ffi {
 }

 unsafe fn generate_ast(
-    mut ir: Box<IR>,
+    ir: &mut IR,
     compiler: *mut ffi::TCompiler,
     options: &ffi::CompileOptions,
 ) -> ffi::Output {
     // Passes required before AST can be generated:
-    transform::dealias::run(&mut ir);
-    transform::astify::run(&mut ir);
+    transform::dealias::run(ir);
+    transform::astify::run(ir);

     let mut ast_gen = output::legacy::Generator::new(compiler, options.is_es1);
     let mut generator = ast::Generator::new(&ir.meta);

The changes above:

  • Enable having an alias to type IR by adding #[derive(ExternType)]
  • Move the type alias from extern "Rust" to extern "C++" + add include! of the cxx-generated .rs.h file
  • Workaround 1: Stop using Box<IR> with the type alias and replace this parameter type with &mut IR

With the above, the original build error is gone, but now we are hitting the errors I've reported in one of the comments above:

  • Problem 2: static assertion failed due to requirement '::rust::IsRelocatablesh::ir::ffi::IR::value': type sh::ir::ffi::IR should be trivially move constructible and trivially destructible in C++ to be used as a non-pinned mutable reference in signature of generate_ast in Rust

I know that problem 2 should go away after my ad-hoc tweaks from #1539 (comment). I''ll try to spend some time today trying to understand if/why those tweaks are ok/safe (and then putting them into a separate PR and abandoning this PR):

  • Tweak A: macro/src/expand.rs: Having #[derive(ExternType)] emit type Kind = ::cxx::kind::Trivial instead of kind::Opaque
  • Tweak B: gen/src/write.rs: Having fn write_opaque_type emit using IsRelocatable = std::true_type

There is also a problem of duplicate monormorphization/impl thunks when Box<RustTypeAlias>, Vec<RustTypeAlias>, etc are used. I hope that this isn't immediately blocking for the ANGLE work. Some initial, tentatively fix ideas:

  • Idea 1: Avoid generating an implicit impl if T is a type alias. I like this approach best, but maybe this should be considered a breaking change? If so, then I am not sure if a breaking change is automatically a "no go", or if there are some options/actions we can still consider to proceed with this idea.
  • Idea 2: Make cxx::bridge stateful (somehow... maybe using proc-state crate?) and remember if Box<T> has already been implemented in another/previous macro invocation
  • Idea 3: Allow multiple implementations of Box<T> for the same T. This seems difficult. OTOH I know that I'll eventually have to think about this problem for Exploring general solutions for supporting arbitrary T in Vec<T>, CxxVector<T>, etc. #1538

@anforowicz
Copy link
Contributor Author

I know that problem 2 should go away after my ad-hoc tweaks from #1539 (comment). I''ll try to spend some time today trying to understand if/why those tweaks are ok/safe (and then putting them into a separate PR and abandoning this PR).

I've submitted #1606 to replace the current PR.

@anforowicz anforowicz closed this Sep 5, 2025
@ShabbyX
Copy link

ShabbyX commented Sep 5, 2025

There is also a problem of duplicate monormorphization/impl thunks when Box, Vec, etc are used. I hope that this isn't immediately blocking for the ANGLE work

Thanks. I have an ffi::Box on the C++ side, but if I can pass it to Rust as &mut IR, I don't really have a need to take it as Box<IR> (i.e. not a blocker)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants