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

Inconsistent opaque type representation #1194

Open
jsgf opened this issue Dec 15, 2017 · 9 comments
Open

Inconsistent opaque type representation #1194

jsgf opened this issue Dec 15, 2017 · 9 comments
Labels

Comments

@jsgf
Copy link
Contributor

jsgf commented Dec 15, 2017

Input C/C++ Header

template <class a> class Range { a b, c; };
typedef Range<const char *> StringPiece;
typedef Range<char> MutableStringPiece;
Range<char const *> d() {}

Bindgen Invocation

$ bindgen input.h --whitelist-type '(Mutable)?StringPiece'     --generate=types     --opaque-type '(Mutable)?StringPiece'     --no-layout-tests -- -x c++ -std=gnu++14

Actual Results

/* automatically generated by rust-bindgen */

pub type StringPiece = [u64; 2usize];
pub type MutableStringPiece = u8;

Expected Results

pub type StringPiece = [u64; 2usize];
pub type MutableStringPiece = [u64; 2usize];

StringPiece has a Rust type which correctly reflects its representation: [u64; 2size], which allows the Rust version of the type to be moved around etc. The MutableStringPiece is misrepresented by u8, which results in runtime errors because only the first byte is moved.

It looks like the d() function is what makes the difference; if I remove this then both StringPiece and MutableStringPiece get the same incorrect u8 representation.

This seems to be a regression introduced about 0.31, but I haven't checked specifically.

cc @kulshrax

@emilio
Copy link
Contributor

emilio commented Dec 15, 2017

So, this looks like something where libclang is lying at us, though I haven't actually checked much closer.

First of all, I assume MutableStringPiece actually looks like:

typedef Range<char*> MutableStringPiece;

right? (otherwise the expected representation would be [u8; 2]).

If I run a header that instantiates both types:

template <class a> class Range { a b, c; };
typedef Range<const char *> StringPiece;
typedef Range<char*> MutableStringPiece;
struct Foo {
  StringPiece foo;
  MutableStringPiece bar;
};

I get the expected:

/* automatically generated by rust-bindgen */

pub type StringPiece = [u64; 2usize];
pub type MutableStringPiece = [u64; 2usize];

@emilio emilio added the bug label Dec 15, 2017
@emilio
Copy link
Contributor

emilio commented Dec 15, 2017

In any case this is a bug of course, the type shouldn't change depending on whether it's instantiated or not...

@emilio
Copy link
Contributor

emilio commented Dec 15, 2017

Err, I lie, and this is introduced by 28ae2a1, very obviously on retrospective.

@emilio
Copy link
Contributor

emilio commented Dec 15, 2017

Hmm, not so obviously actually, I bet I messed up the bisect

@emilio
Copy link
Contributor

emilio commented Dec 15, 2017

Yeah, the bisect is incorrect, this is not a new regression at all it seems... We're not getting the proper layout off libclang unless the type is actually instantiated, wtf.

@emilio
Copy link
Contributor

emilio commented Dec 15, 2017

Yeah, 92c1a25 changed the output, but that was only from = [u8; 0] to = u8...

@emilio
Copy link
Contributor

emilio commented Dec 15, 2017

I think this is kind of expected, because based on the information clang has, the final size of the type is not known. Someone could do something like:

template <class a> class Range { a b, c; };
typedef Range<const char *> StringPiece;

template<>
class Range<const char*> { };

Or what not... Probably could write a libclang patch to try to instantiate the type if not done before? Not sure if that's trivial, would need to dig a bit...

@jsgf
Copy link
Contributor Author

jsgf commented Dec 15, 2017

typedef Range<char*> MutableStringPiece;

Yes it should be that - I left the creduce run under-constrained. But it doesn't look like it makes any material difference to the real bug.

@jsgf
Copy link
Contributor Author

jsgf commented Dec 15, 2017

OK, so I can work around this by adding

struct _dummy_instantiate {
  folly::MutableStringPiece a;
  folly::MutableByteRange b;
};

to my header. I'm pleased to have a workaround, but this definitely seems like something I shouldn't have to do.

I think this is probably a pervasive problem now that I think of it - I've seen a lot of inconsistencies between whether an opaque type is represented as u8 or [u64; X], but this was the first instance that was stark enough for me to investigate. But now I know what to look for...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants