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

Add repr(transparent) to hyper_context #3498

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Manishearth
Copy link
Contributor

unsafe { std::mem::transmute::<&mut Context<'_>, &mut hyper_context<'_>>(cx) }

This comment is incorrect, repr(Rust) provides no such guarantees.

(Also we probably should make most of these FFI types repr(C)))

@seanmonstar
Copy link
Member

Looks like cbindgen changes the header generation from typedef struct hyper_context hyper_context; to typedef Context hyper_context;. Can we make cbindgen ignore that?

@Manishearth
Copy link
Contributor Author

No idea!

@Manishearth
Copy link
Contributor Author

repr(C) is fine too here.

@Manishearth
Copy link
Contributor Author

Actually I'm not sure of repr(C) on weirder platforms. repr(packed) probably?

@Manishearth Manishearth force-pushed the transparent branch 2 times, most recently from 3d73180 to bd15e65 Compare December 20, 2023 22:17
@Manishearth
Copy link
Contributor Author

Someone checked the ANSI C standard for me, repr(C) is fine

@Manishearth
Copy link
Contributor Author

@seanmonstar You should pick which one of transparent/packed/C you want here, all three are fine but I don't know enough about cbindgen's behavior. Ideally this would be configurable.

@hjr3
Copy link
Contributor

hjr3 commented May 5, 2024

We want two things:

  • a guaranteed layout for hyper_context so transmuate is safe
  • to treat hyper_context as an opaque type

cbindgen does not provide to force a type with a guaranteed layout to be opaque.

We have a few options here:

  1. Use a work-around where we use two different types and renaming. see hjr3@c5cc1e9#diff-fc56f58b6249ca3abcf76fb3641d25425c53e4d9f6c56514fcfe81fef72c8d3aR130-R143
  2. Accept that hyper_context will not be opaque and add support for Context. See Opaque type for Mutex mozilla/cbindgen#930 (comment) which suggests a patch to https://github.com/mozilla/cbindgen/blob/ca78140c01518a655355f84da1f3872939123b66/src/bindgen/parser.rs#L470
  3. Ask cbindgen to support an override to force an opaque type. See feat(ir) Add ability to ignore specific struct fields mozilla/cbindgen#579 (comment)

1 is the most pragmatic, 2 should probably be done anyways (adding Context to std types) and 3 is probably the best long-term approach.

If we want to go with 1, I can open a PR using my commit.

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