Breaking change: c_char sign

[SimonSapin]

#40317 landed without ceremony, including a7add43 "Fix c_char (u8 -> i8) definition for i686-linux-android" which is a breaking change. Even if the previous definition was wrong, changing it is still backward incompatible in the sense that code that used to build correctly now fails to build when updating the compiler. Maybe we’ll want to make the change anyway eventually, but such breakage should be handled with more care, at least with an announcement in advance.

CC @nox, @jdm

[steveklabnik]

@rust-lang/libs nominating

[SimonSapin]

servo/rust-mozjs#345

error[E0308]: mismatched types
  --> src/error.rs:24:11
   |
24 |     name: b"RUSTMSG_TYPE_ERROR\0" as *const _ as *const libc::c_char,
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u8, found i8

The issue is that the generated bindings use std::os::raw::c_char, and our code uses libc::c_char. In the past these types have been identical, and now they are not for some reason.

[jdm]

Just to be clear, the rust-mozjs issue that was just quoted doesn't show an example of a rust breaking change biting us. #40317 makes that error disappear.

[aturon]

@SimonSapin I think it's very likely that this was just missed; I know we're all very wary about making any breaking changes around libc.

@alexcrichton is traveling right now, but I will try to dig into this ASAP.

[bluss]

See also the C Types RFC rust-lang/rfcs#1783

Since one problem encountered here (@jdm) is about libc and libstd disagreeing about the types.

[malbarbo]

I made both the PR in rust and libc. I'm sorry for not being explicit of the breaking change...

The break change only affect i686-linux-android, which is listed as tier-3 platform.

What should we do?

[SimonSapin]

@aturon I do believe that any std breakage here was an honest mistake, sorry if my earlier message implied otherwise.

[nagisa]

Note that there is still a silent change in behaviour that is easy to miss. That is the right shift.

Previously (with c_char = u8) it would have been a logical shift, but now (c_char = i8) its arithmetic, and will extend the sign. This is silent breakage for anybody who does such shifting.

[aturon]

I spoke a bit with @alexcrichton to get more context for this change.

Echoing @malbarbo, he was aware of the breakage here but was under the impression that this platform was essentially unused (and at tier 3 status), and not considered fully stable.

I'll leave the nomination here so that the libs team can discuss the policy around such cases.

For the moment, the question is what steps we should take to deal with the problem at hand. From what I can tell, updating to the latest nightly addresses the problem in rust-mozjs, at least. But I'm guessing that, for Servo proper, this would require a rustup, which may be problematic? Can you provide more detail about the status of Servo with respect to this change?

We could consider reverting the change, but I worry that that will make matters even worse.

[jdm]

I do not believe updating Rust for Servo will be an issue.

[nox]

servo/servo#16181

[aturon]

Thanks @nox, and sorry for the trouble.

[SimonSapin]

I think the population of Rust users is well past the point where we can assume that anything is completely unused. Even if Tier 3 means we reserve the right to break things, it would be nice to announce known breakage.