Enforce C,packed, not just packed, on ULE types

[Manishearth]

Fixes #5039

Caused by rust-lang/rust#125360. We were assuming that packed meant C, packed already. This is an assumption I've seen throughout the Rust ecosystem so there may be reasons to revert.

[Manishearth]

Given that this breaks data, I would suggest we release:

• zerovec & zerovec_derive 0.10.x, perhaps 0.9.x?
• icu_calendar, icu_properties, icu_casemap: 1.5.x, 1.4.x, perhaps 1.3.x?

[sffc]

Since these are ULE types, do we need the repr at all? Or maybe just repr C? I recall this causing compiler issues multiple times.

[Manishearth]

No, we absolutely need packed for safety, that is a strong safety requirement. Otherwise we make padding bytes readable.

[Manishearth]

We have already fixed the compiler issues.

[sffc]

Under what conditions would padding bytes be inserted, given that all of the types are align 1?

[sffc]

What I mean is, a better assertion might be repr(C) with the proc macros adding an assertion that size_of::<U> is equal to the sum of the size of its fields.

[Manishearth]

Under what conditions would padding bytes be inserted, given that all of the types are align 1?

It's unclear, there's some iffy behavior around generics and types C does not natively support, and I would rather be safe than sorry here. We want it to be packed, so let's say we want it to be packed. There's very little weird compiler behavior here, I do not consider it a major risk. None of it is prohibitive in nature.

---

we could add autogenerated assertions if we like, however I am not in favor of replacing the packed directive with something with something else

[sffc]

I didn't double check that you touched all places you were supposed to touch. I can do that if you think it's important.

[Manishearth]

I think it's fine, yeah! We should figure out backports next week.

[Manishearth]

needs-approval for backport plan, seeking input on what backports to do

[sffc]

Yes on rereleasing things in the 1.5 branch of course. For the older versions, I remember we decided to support Rust versions N-6 to N-4 into the past. Did we decide how far to support into the future?

[Manishearth]

I don't recall, but given that it's an easy backport I figured there's no harm in doing it anyway if someone (me) is happy to put in the effort.

[sffc]

Notes from 2024-06-20

• @Manishearth - Much of our zerovec code was not relying on a stable invariant regarding repr(packed). We missed the early warning because our nightly CI was not working. The fix is changing it to repr(C,packed) and enforcing this. Only a few crates need code fixes: zerovec, calendar, casemap, ... I suggest we fix 1.5 for sure, but I think we could also fix 1.4.
• @sffc - I see in Amend MSRV policy for special cases to handle forward breakages #3706 some discussion about forward compatibility but I don't see any hard conclusions. We say we support ~6 months of old Rust versions, so we should probably support at least ~6 months of new Rust versions
• @sffc - I would like to see 1.5, 1.4, 0.10, 0.9
• @robertbastian - I would like to see the old versions yanked, it's very annoying to have non-building things on crates.io, and even worse to have buggy unsafe things
• @sffc - I think it's not necessary to yank all the versions; maybe just the latest version on each stream?
• @Manishearth - fine with yanking
• @sffc - Given that we're putting in the time to release 0.9.x, it seems harmless to yank the old versions even though I think it's probably not necessary; people using really old zerovec probably have really old rust?
• @robertbastian - I think it's not necesarilly true.

Conclusion:

• icu4x patches for 1.5, 1.4
• zerovec patches for 0.10, 0.9
• yank old zerovec versions in 0.9 and 0.10 series

LGTM: @Manishearth @robertbastian @sffc

[Manishearth]

Oh, I forgot to yank

[Manishearth]

done