Beta regression: References to what's supposed to be static singleton don't pointer compare as equal on 1.23.0-beta.1

[hsivonen]

Steps to reproduce:

1. git clone https://github.com/hsivonen/encoding_rs
2. cd encoding_rs
3. rustup default beta
4. cargo test --release

Actual results:

failures:

---- test_labels_names::test_all_labels stdout ----
	thread 'test_labels_names::test_all_labels' panicked at 'assertion failed: `(left == right)`
  left: `Some(Encoding { windows-1252 })`,
 right: `Some(Encoding { windows-1252 })`', src/test_labels_names.rs:11:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- tests::test_label_resolution stdout ----
	thread 'tests::test_label_resolution' panicked at 'assertion failed: `(left == right)`
  left: `Some(Encoding { UTF-8 })`,
 right: `Some(Encoding { UTF-8 })`', src/lib.rs:4419:8

Note how left and right in both cases have the same debug stringification.

The eq implementation assumes that there is one instance of each encoding so it's OK to pointer compare them.

Various previous stable channel releases work. Nightly works. Beta works in debug mode. 1.23.0-beta.1 fails in release mode.

encoding_rs issue, Firefox bug

[nagisa]

Will probably be the codegen-units thing. Singleton statics should use linkonce/linkonce_odr linkage, although it is not entirely clear to me whether it is up to user to do so or up to us (the compiler).

cc @michaelwoerister @alexcrichton

EDIT: It is certainly a user responsibility if a singleton common for multiple crate versions is desired.

[michaelwoerister]

Hm, statics should always have external linkage (we don't use linkonce or linkonce_odr anywhere these days). Maybe the symbol internalizer wrongly decides that it can make multiple internal copies?

[alexcrichton]

This may also be related to #46239 and fixed by #46253, the IR doesn't obviously contain multiple instances of the static here and there's no codegen units involved, so I'd be surprised if the collector came into this!

[rillian]

Being fixed by #46253 is possible; I can't reproduce with today's nightly rust. @michaelwoerister, can you make sure that gets into to the next beta, please?

[nagisa]

I don't think this was fixed by that PR.

[michaelwoerister]

@rillian I'll do a backport of #46253 tomorrow.

@nagisa Do you have an alternative theory?

[michaelwoerister]

I cannot reproduce this with the latest beta anymore. Could you give it a try, @hsivonen?

Also, regarding the question why crater did not find this issue: it seems that crater does not do optimized builds and thus could not have run into this.

[pnkfelix]

Note that @arielb1 claimed that #46925 fixed this regression.

(Though, oddly enough, @michaelwoerister 's statement of being unable to reproduce preceded that PR landing...)

Anyway, without triggering a beta .3 release (that would, if I understand correctly, get the latest commit on the beta channel into rustup), it might be difficult for users to test out the latest beta...

[eddyb]

@pnkfelix There was a backport of #46253 before @arielb1 reverted the layout PR and the bugfix backports related to it, from beta.

[pnkfelix]

@eddyb so was @nagisa wrong when they said that they thought #46253 would not fix this?

[nagisa]

Yeah, I probably was wrong. I didn’t look at the example closely enough before making the statement and forgot to edit it later on. Sorry.

[hsivonen]

Could you give it a try, @hsivonen?

It seems that stable is 1.22.1 and beta is 1.24.0-beta.1. Those both work. Nightly works, too.

What should I tell rustup to test the latest 1.23?

[michaelwoerister]

What should I tell rustup to test the latest 1.23?

Good question. @alexcrichton would know best, I guess?

[alexcrichton]

There's info on how to test it in the prerelease announcement, lemme know if it doesn't work though!

[hsivonen]

The 1.23.0 release candidate works.

Thank you!