Skip to content

Conversation

ekse
Copy link

@ekse ekse commented Nov 23, 2018

Issue: #5

The rust-bindgen package is not yet updated to 0.43.2 on crates.io, so this PR currently gets it from github. We can wait for the update before merging.

Copy link
Member

@jethrogb jethrogb left a comment

Choose a reason for hiding this comment

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

I left some general questions. I'll also want to double-check the generated bindings to make sure everything looks good, I'll do that next week.

let mut bindgen = bindgen::Builder::new(header.into_os_string().into_string().unwrap());
let bindings = bindgen
.log(&logger)
let bindings = bindgen::builder()
.clang_arg("-Dmbedtls_t_udbl=mbedtls_t_udbl;") // bindgen can't handle unused uint128
Copy link
Member

Choose a reason for hiding this comment

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

Is u128 supported now?

name.starts_with("MBEDTLS_CTR_DRBG_PR_") ||
name.starts_with("MBEDTLS_ENTROPY_SOURCE_") ||
name.starts_with("MBEDTLS_HMAC_DRBG_PR_") ||
name.starts_with("MBEDTLS_RSA_PKCS_")
Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine... Is there any method to this categorization? Does this work better than the value-based categorization that was being used before?

Copy link
Author

Choose a reason for hiding this comment

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

macro_int_types is not supported anymore by bindgen. I replaced the type for those that we expect to be i32 in the getter!/setter! macros.

Copy link
Member

Choose a reason for hiding this comment

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

Well, not directly, but value is being passed so you can just use the same logic.

Copy link
Member

Choose a reason for hiding this comment

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

The old configuration would basically just use c_int unless a c_longlong was needed for size. Maybe if we default to c_int here, the whitelist doesn't need to be this long?

header
.to_str()
.expect("failed to convert header path to string"),
).use_core()
.derive_debug(false) // buggy :(
Copy link
Member

Choose a reason for hiding this comment

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

Still buggy?

Copy link
Author

Choose a reason for hiding this comment

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

I will investigate if u128 and derive_debug are now working. Do you remember what the issue was with derive_debug?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's been a long time. Possibly it would add #[derive(Debug)] on types where not all fields where themselves Debug, resulting in compilation failure.

We use a ParseCallback to remove the "mbedtls_" prefix from
items.
@ekse
Copy link
Author

ekse commented Nov 26, 2018

The bindgen crate was updated, the PR is now using the package from crates.io.

@jethrogb
Copy link
Member

jethrogb commented Nov 28, 2018

These will need to be added, but it's buggy in combination with ParseCallbacks::item_name:

.whitelist_type("(?i)mbedtls.*")
.whitelist_var("(?i)mbedtls.*")
.whitelist_function("(?i)mbedtls.*")
.whitelist_recursively(false)

@jethrogb
Copy link
Member

I made some more changes, but still waiting for a couple bindgen bugs to be resolved:

diff --git a/mbedtls-sys/build/bindgen.rs b/mbedtls-sys/build/bindgen.rs
index ec0a1c7..454ae73 100644
--- a/mbedtls-sys/build/bindgen.rs
+++ b/mbedtls-sys/build/bindgen.rs
@@ -99,14 +99,15 @@ impl super::BuildConfig {
                     .to_str()
                     .expect("failed to convert header path to string"),
             ).use_core()
-            .derive_debug(false) // buggy :(
             .disable_name_namespacing()
             .prepend_enum_name(false)
             .ctypes_prefix("raw_types")
             .parse_callbacks(Box::new(ParseCallbacks{}))
-            // max_align_t is causing bindgen generated tests to fail an alignment check and
-            // is not needed by the bindings.
-            .blacklist_type("max_align_t")
+            .blacklist_type("mbedtls_iso_c_forbids_empty_translation_units")
+            .whitelist_type("(?i)mbedtls.*")
+            .whitelist_var("(?i)mbedtls.*")
+            .whitelist_function("(?i)mbedtls.*")
+            .whitelist_recursively(false)
             // Including the comments breaks the generated code because it contains formatting
             // that is interpreted as escaped characters.
             .generate_comments(false)
@@ -122,7 +123,7 @@ impl super::BuildConfig {
 
         let mod_bindings = self.out_dir.join("mod-bindings.rs");
         File::create(&mod_bindings)
-            .and_then(|mut f| f.write_all(b"mod bindings;\n"))
+            .and_then(|mut f| f.write_all(b"#[allow(bad_style)] mod bindings;\n"))
             .expect("mod-bindings.rs I/O error");
     }
 }

rust-lang/rust-bindgen#1452
rust-lang/rust-bindgen#1454
rust-lang/rust-bindgen#1455

@awakecoding
Copy link

@jethrogb any update on this?

@awakecoding
Copy link

I rebased and fixed the original branch from this PR: https://github.com/awakecoding/rust-mbedtls/tree/bindgen-upgrade-v2

@jethrogb
Copy link
Member

@awakecoding Thanks, but I'm still waiting for rust-lang/rust-bindgen#1454 and rust-lang/rust-bindgen#1455.

@jack-fortanix jack-fortanix mentioned this pull request Oct 8, 2019
@haraldh haraldh mentioned this pull request Oct 25, 2019
@ekse
Copy link
Author

ekse commented Nov 12, 2020

Obsoleted by #72

@ekse ekse closed this Nov 12, 2020
@jethrogb jethrogb mentioned this pull request Mar 11, 2021
1 task
@jethrogb
Copy link
Member

Final PR: #152

bors bot added a commit that referenced this pull request Apr 6, 2021
152: Update bindgen r=raoulstrackx a=jethrogb

I took some time this week to make the necessary bindgen changes (rust-lang/rust-bindgen#2004 rust-lang/rust-bindgen#2006 rust-lang/rust-bindgen#2007). This PR updates the bindgen build to use that.

Breaking changes:
* unnamed types are renamed
* C-unions are now actual unions
  * [x] the field accessor functions that used to exist are easy enough to add back
* bitfields are done differently now
* some fn-ptrs are now unsafe
* some fns and fn-ptrs now take const pointers as arguments
* havege.c and timing.c are now not compiled on non-unix platforms, i.e. SGX (probably wasn't working properly anyway)

Fixes #5 #14 #61 #72 #88 #121

Co-authored-by: Jethro Beekman <[email protected]>
@jethrogb
Copy link
Member

jethrogb commented Apr 6, 2021

Fixed in #152

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