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

c_char has changed on ARM in 1.6 #29867

Closed
alexcrichton opened this Issue Nov 16, 2015 · 14 comments

Comments

Projects
None yet
6 participants
@alexcrichton
Copy link
Member

alexcrichton commented Nov 16, 2015

Android was changed in #29546 and Linux ARM was changed in #29775. Both are bug fixes as the definitions have been corrected, but this is causing breakage:

Can we help mitigate this breakage? Should we revert "the fix" for now?

Opening a tracking issue (and nominating it) to discuss this.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Nov 16, 2015

Some other points:

  • Updating to libc 0.2 and using Rust nightly should not cause errors to show up
  • ARM is not a tier 1 platform right now, but it's kinda close to one
  • libc 0.1 is no longer compatible with nightly std on ARM (due to this and CString)
  • libc 0.2 is not compatible with Rust 1.5 and prior (due to this and CString)
@arcnmx

This comment has been minimized.

Copy link
Contributor

arcnmx commented Nov 16, 2015

Note that #29775 only changed raw::os::c_char to be consistent, which probably no one seemed to be using at all. ARM Linux was also broken in #29546 since the main observable problem was with the type CStr::as_ptr returned.

It sucks. My solution has been to pin specific versions via hacking on Cargo.lock. I'm not sure that reverting back to the wrong type is the right course of action to take, though.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 16, 2015

Should library use types from std::os::raw when they exist instead of libc? (Maybe depending on whether or not they need other things from libc?) Should rust-bindgen be changed to use std::os::raw for types that exist there?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Nov 16, 2015

That's a good question! I don't think it should matter as the types should in theory always be the same.

@arcnmx

This comment has been minimized.

Copy link
Contributor

arcnmx commented Nov 16, 2015

I think they should, tbh. In theory they should be the same, but if you plan on using std methods, you should be using the types it exports, and not assuming some external crates.io crate matches up.

For bonus points: make them quasi-newtypes so a cast between the two is required :P

Also: All Android platforms (including x86) seem to be affected.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 16, 2015

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Nov 17, 2015

Can we reorganize this somehow so that we use a single source of truth? Defining c_char in libstd, in-tree liblibc and crates.io liblibc is in two places too many right.

@SimonSapin SimonSapin referenced this issue Nov 19, 2015

Closed

Update libc to 0.2 #8608

52 of 52 tasks complete
@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Nov 19, 2015

The libs team discussed this during triage yesterday and the conclusion was that we're unlikely to go back to the old incorrect definitions and otherwise we'll basically just do what we can to help mitigate this breakage.

@bluss that's possible, yeah, although it wouldn't have fixed this problem because the "single source of truth" from before was wrong (both libstd and libc disagreed).

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 19, 2015

I hope that updating all the things (servo/servo#8608) to libc 0.2 will fix the build errors related to this for Servo.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Nov 19, 2015

@alexcrichton But the major fallout comes from incompatible types, not from the change per se? If all the crate including libc used c_char reexported from libstd, and I think you can't compile against more than one version of libstd, they'd all be using the same type.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Nov 19, 2015

Yeah that's true and it would help mitigate problems like this. There's a number of interesting issues around libc reexporting types from the standard library, however, so it's currently not done.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Nov 19, 2015

@bluss but libc is the source of c_char for libstd, so the problem comes when your external libc crate doesn't match the one built into libstd. (edit: I take that back, raw::c_char is defined on its own. TIL)

Maybe the libc crate should use a cfg feature for this? Set that feature to provide its own good c_char when used within libstd, else just reexport std::os::raw::c_char. This way external use always agrees with libstd.

@MagaTailor

This comment has been minimized.

Copy link

MagaTailor commented Feb 4, 2016

@MagaTailor

This comment has been minimized.

Copy link

MagaTailor commented Jun 26, 2016

Even aarch64 is affected to this very day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.