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.

[aturon]

Even if Tier 3 means we reserve the right to break things, it would be nice to announce known breakage.

I absolutely agree.

[alexcrichton]

Just to confirm, but it looks like the servo rustup went through to this is a non-issue for Servo at the moment? Just want to see if there's any remaining fires to put out before we discuss the broader policy issue here.

[jdm]

Yes, there is no Servo fire to put out.

[alexcrichton]

Ok we discussed this at the recent libs triage and our conclusions were:

• At this time because there are no active fires to be put out, we're not going to revert the change
• We'd like to reserve the ability to break tier 3 platforms because, well, that's the definition of tier 3
• Are there opinions about where to announce breakage like this though? This is typically known in advance.

Some ideas of where to announce this were "This Week in Rust", release notes for the libc crate, or maybe some dedicated thread on internals. Unfortunately though we weren't sure what the best option here was, so we haven't decided anything yet.

In the meantime though I'm going to close this as we've decided not to revert.

[SimonSapin]

Are there opinions about where to announce breakage like this though?

https://internals.rust-lang.org/c/announcements seems designed for this purpose? It is possible to "watch" this single category in order to be notified of new threads there without being notified of every new thread on i.r-l.o.

[alexcrichton]

I personally feel that internals is a bit too broad, you can probably count the number of people one one hand who work with each particular tier 3 platform...

[aturon]

@alexcrichton I'm a bit confused about the worry here -- how often do we expect this to be happening? I was concerned that internals wasn't broad enough to reach people who might care :-)

[alexcrichton]

Likely around once a month would be what my gut says, so yes it may not be that bad in practice.

[aturon]

OK, let's go with internals for now, and change it up if that becomes a problem.