Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass through cfg attrs #732

Closed
wants to merge 1 commit into from
Closed

Conversation

j-vanderstoep
Copy link

Otherwise things like cfg(target_os = "android") cannot be used
within a cxx bridge.

Resolves error:
error: unsupported attribute
--> system/bt/gd/rust/common/src/sys_prop.rs:4:1
|
4 | #[cfg(target_os = "android")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

When updating to cxx version 1.0.x in AOSP.
https://cs.android.com/android/platform/superproject/+/master:system/bt/gd/rust/common/src/sys_prop.rs;l=3

Otherwise things like cfg(target_os = "android") cannot be used
within a cxx bridge.

Resolves error:
error: unsupported attribute
 --> system/bt/gd/rust/common/src/sys_prop.rs:4:1
  |
4 | #[cfg(target_os = "android")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

When updating to cxx version 1.0.x in AOSP.
https://cs.android.com/android/platform/superproject/+/master:system/bt/gd/rust/common/src/sys_prop.rs;l=3
@adetaylor
Copy link
Collaborator

@j-vanderstoep I'm interested in whether you need to apply this attribute within a cxx::bridge (as your description suggests) or to the entire cxx::bridge (per your AOSP link).

I added some tests here: master...adetaylor:cfg-test to play with the two options.

I think a bit more work will be required either way. Your change may pass this attribute through to the output code expanded by the procedural macro, but the C++ code generator (cxxbridge or its build.rs equivalent) independently parses the .rs file and probably isn't aware of cfg attributes at all. I suspect you'd find therefore that the C++ code generator always generates the C++ side of the bindings, even if you manage to disable parts or all of the bindings on the Rust side.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This is problematic because it potentially generates ABI-incompatible bindings, for example if you add to demo::ffi::BlobMetadata:

      struct BlobMetadata {
+         #[cfg(x)]
          size: usize,
          tags: Vec<String>,
      }

the result is a segfault because it's produced a struct with 1 field on the Rust side and 2 fields on the C++ side, and tried to read the wrong bits as a Vec.

$ cargo run
blobid = 9851996977040795552
Segmentation fault (core dumped)

Thoughts on what to do about that?

@dtolnay dtolnay closed this Aug 21, 2021
@dtolnay
Copy link
Owner

dtolnay commented Dec 27, 2021

#989 (comment) has a proposal for how to implement this. Cargo-based builds need to examine the CARGO_CFG_... environment variables provided by Cargo to the build script, while non-Cargo needs to pass its configuration to cxxbridge using something like --cfg=target_os=android. This way both the C++ and Rust side have agreement on what fields/arguments/functions/types are present.

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.

None yet

4 participants