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

Canonicalize a type (pointer type) first before checking for constness #1311

Merged
merged 4 commits into from
May 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/ir/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1194,7 +1194,23 @@ impl Type {
};

let name = if name.is_empty() { None } else { Some(name) };
let is_const = ty.is_const();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this works, but instead the better fix is (I think), changing the following line:

https://github.com/rust-lang-nursery/rust-bindgen/blob/515ca97b462f348b7b3bb9eaea98b2e0bb1b624b/src/codegen/mod.rs#L3057

To not check self.is_const(), pointing out that pointer-constness itself can't be represented in rust.

Or in any case that line could be cleaned up.

I think handling it in codegen is better though, since there are a couple other places where we use libclang's is_const in context.rs.

Also, could you test this with constants? This would solve #511. The test-case in that issue's description is pretty good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhhh. It's somewhat strange, I tried to apply your suggestion but I am getting test failures. It seems that ctx.resolve_type(inner) resolves to an opaque type? I am not sure though, I didn't have much time to debug this (I guess, I'll give it another shot later today).

But this PR should also fix #511 (Well, it seems like it).

Copy link
Contributor

Choose a reason for hiding this comment

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

If you get stuck let me know, we can merge this for now and I can take a look at that more carefully. Please do file an issue for that if we leave it as a followup and add the tests for #511 if you can though.

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's best for now. I'll create a new issue after this PR is merged.
I've also pushed the test from issue #511.


// Just using ty.is_const() is wrong here, because when we declare an
// argument like 'int* const arg0', arg0 is considered
// const but the pointer itself points to mutable data.
//
// Without canonicalizing the type to the pointer type, we'll get the
// following mapping:
//
// arg0: *const c_int
//
// So by canonicalizing the type first, we can check constness by
// calling is_const() on the pointer type.
let is_const = if let Some(pty) = ty.pointee_type() {
pty.is_const()
} else {
ty.is_const()
};

let ty = Type::new(name, layout, kind, is_const);
// TODO: maybe declaration.canonical()?
Expand Down
4 changes: 1 addition & 3 deletions tests/expectations/tests/const_tparam.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
/* automatically generated by rust-bindgen */


#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]


#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct C<T> {
pub foo: *const T,
pub bar: *mut T,
pub bar: *const T,
pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell<T>>,
}
impl<T> Default for C<T> {
Expand Down
5 changes: 1 addition & 4 deletions tests/expectations/tests/derive-hash-struct-with-pointer.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
/* automatically generated by rust-bindgen */


#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]



/// Pointers can derive Hash/PartialOrd/Ord/PartialEq/Eq
#[repr(C)]
#[derive(Debug, Copy, Clone, Hash, PartialOrd, Ord, PartialEq, Eq)]
pub struct ConstPtrMutObj {
pub bar: *const ::std::os::raw::c_int,
pub bar: *mut ::std::os::raw::c_int,
}
#[test]
fn bindgen_test_layout_ConstPtrMutObj() {
Expand Down
20 changes: 20 additions & 0 deletions tests/expectations/tests/issue-511.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/* automatically generated by rust-bindgen */

#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]

extern "C" {
#[link_name = "\u{1}a"]
pub static mut a: *mut ::std::os::raw::c_char;
}
extern "C" {
#[link_name = "\u{1}b"]
pub static mut b: *const ::std::os::raw::c_char;
}
extern "C" {
#[link_name = "\u{1}c"]
pub static mut c: *mut ::std::os::raw::c_char;
}
extern "C" {
#[link_name = "\u{1}d"]
pub static mut d: *const ::std::os::raw::c_char;
}
2 changes: 1 addition & 1 deletion tests/expectations/tests/layout_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub type rte_mempool_free_t = ::std::option::Option<unsafe extern "C" fn(mp: *mu
pub type rte_mempool_enqueue_t = ::std::option::Option<
unsafe extern "C" fn(
mp: *mut rte_mempool,
obj_table: *const *const ::std::os::raw::c_void,
obj_table: *const *mut ::std::os::raw::c_void,
n: ::std::os::raw::c_uint,
) -> ::std::os::raw::c_int,
>;
Expand Down
4 changes: 4 additions & 0 deletions tests/headers/issue-511.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
char * a;
const char * b;
char * const c;
const char * const d;