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

Bring back the ring::c internal C types module. #840

Merged
merged 1 commit into from
Jun 14, 2019
Merged

Conversation

briansmith
Copy link
Owner

No description provided.

@briansmith briansmith self-assigned this Jun 14, 2019
@briansmith
Copy link
Owner Author

@josephlr @elichai @jonas-schievink PTAL.

A long time ago we had a ring::c module like this and we defined all the C types ourselves. We even had tests to verify that the size and alignment of each type matched between Rust and C. We got rid of all of that because somebody suggested it would be better to just delegate all of this to libc, thinking there's no reason libc wouldn't define these types when it makes sense to do so. Apparently that's wrong. So, this reverts to an internal API that is similar to the old ring::c API, but where the types are still implemented as aliases of libc types. This should reduce the amount of code that would need to be directly impacted by supporting non-libc sources of these types. In particular, this gives us the flexibility to, on a per-target basis, choose to use libc's types, another crate's types, or an internal definitions of the types (like ring originally did).

Copy link
Contributor

@josephlr josephlr 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 a good first step. Just a few comment related nits and some issues that should probably be addressed, but can wait for future PRs.

pub(crate) type int = libc::c_int;
pub(crate) type uint = libc::c_uint;

#[cfg(all(
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment explaining that this is to support cpu::arm::linux_setup() and the cfg should be kept in sync with the one there.

Copy link
Owner Author

Choose a reason for hiding this comment

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

In general I don't add comments like this because they get out of date. One can use Find Usages or similar in their IDE to see what is using what.

src/c.rs Outdated
))]
pub(crate) type ulong = libc::c_ulong;

#[cfg(target_os = "linux")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment mentioning that this is to support getrandom(2) or merge #83, then this can be removed.


use libc;

pub(crate) type size_t = libc::size_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could just use usize instead throughout ring as suggested in #837 (comment). Note that many of the places ring uses size_t it should really be using uintptr_t; however, BoringSSL also makes this "mistake" in thinking that sizeof(size_t) == sizeof(void *), so it might just be easier to assume size_t == uintptr_t in which case (as libc::uintptr_t == usize), we can just use usize throughout ring.

This could be addressed now, or in a future PR. We might also want to wait until rust-lang/libc#1400 is resolved.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Note that many of the places ring uses size_t it should really be using uintptr_t

Could you provide an example?

Rust has made a mistake where it is only well defined on platforms where uintptr_t and size_t and usize are the same type; whenever size_t and uintptr_t are incompatible then Rust can't work given how usize is defined. I'm hoping a future edition of Rust will correct this. This is actually the reason I prefer to keep using size_t everywhere in Rust where size_t is used in the C code.

BoringSSL also makes this "mistake" in thinking that sizeof(size_t) == sizeof(void *)

I think you may be misunderstanding the example you pointed out; the test in that code fragment has to do with whether 64-bit arithmetic is native or not, nothing about pointers. https://boringssl.googlesource.com/boringssl/+/8f574c37dae935a29bff56e750101e5b354503de/crypto/fipsmodule/bn/exponentiation.c#895 would be an example where size_t is used instead of uintptr_t in BoringSSL. I believe we've eliminated all such confusion in ring already; see #68, #151, and in particular #101.

use libc;

pub(crate) type size_t = libc::size_t;
pub(crate) type int = libc::c_int;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rust has a target-c-int-width attribute that we could use to define c::int instead of relying on libc. However, this is best addressed in a future PR.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Rust has a target-c-int-width attribute that we could use to define c::int instead of relying on libc. However, this is best addressed in a future PR.

That would be awesome to use if it works! Can you give an example? I tried #[cfg(target_c_int_width = "32")] type int = i32; but target_c_int_width doesn't seem to be exposed for #[cfg(...)] to use.

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.

2 participants