rustc_public: Make Id types !Send / !Sync

[aDotInTheVoid]

These types are Id's to a table stored in TLS, so using them from another tread will either panic, or give wrong results.

Therefor, I've added a ReferencesTlsThreadLocalIndex marker type, which ensures types arn't Send/Sync.

This is a breaking change for users of the rustc_public crate.

It also changes how DefId and similar are Serialized. It would be possible to preserve the old behavior if that's needed, but I couldn't see any tests for these. EDIT: Not anymore: #148261 (comment)

Zulip Discussion: https://rust-lang.zulipchat.com/#narrow/channel/320896-project-stable-mir/topic/WDYM.20.22should.20not.20.20be.20shared.20across.20threads.22/with/547374171

[rustbot]

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a

[rustbot]

r? @celinval

rustbot has assigned @celinval.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[celinval]

It's too bad we cannot use negative trait here. KMIR actually uses the serialization. See: https://github.com/runtimeverification/stable-mir-json. They use IDs to map components and correlate then AFAIK.

So can you please make sure you skip the serialization of the new field in the structs you added them?

View changes since this review

[celinval]

I agree... Please don't.

[aDotInTheVoid]

Done in 97e6df4. It's sadly not as simple as using #[serde(skip)], as that still moves serde from serializing just the single field, to a list containing the single field: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b4aafe4ff5312c4ddedd5e17ee361d5a

Is there somewhere that tests for this could go?

[celinval]

I would suggest adding unit tests for this.

[aDotInTheVoid]

Added tests: 7bdbf8e

[celinval]

why not include in this PR?

[aDotInTheVoid]

I wanted to keep this PR not doing too much at once. But I've added it in 8b3ee9f

[aDotInTheVoid]

I'd like to squash this after approval but before merging.

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[celinval]

Sorry, I just remembered that CrateNum can be sync + send. So there's no reason to change it.

[aDotInTheVoid]

How so? I don't think it can.

Otherwise, rustc_public::Crate would be send+sync. But Crate has a load of methods that can only be run on the thread with the TLVs:

rust/compiler/rustc_public/src/lib.rs

Lines 94 to 98 in 055d0d6

	impl Crate {
	/// The list of foreign modules in this crate.
	pub fn foreign_modules(&self) -> Vec<ForeignModuleDef> {
	with(|cx| cx.foreign_modules(self.id))
	}

[celinval]

But I don't think any of them require data from the TLV table. They will panic if you don't initialize the context, but that's the case for any function that doesn't take any argument.

[aDotInTheVoid]

Even so, letting you move a Crate/CrateNum across threads lets get wrong results:

let (tx, rx) = std::sync::mpsc::sync_channel(0);

let t1 = std::thread::spawn(move || {
    rustc_public::run!(&["rustc".to_owned(), "./demo/foo.rs".to_owned()], || {
        let this_crate = rustc_public::local_crate();
        print_crate_info(&this_crate);

        tx.send(this_crate).unwrap();

        ControlFlow::<()>::Continue(())
    })
    .unwrap();
});

let t2 = std::thread::spawn(move || {
    rustc_public::run!(
        &["rustc".to_owned(), "./demo/bar.rs".to_owned()],
        move || {
            let this_crate = rustc_public::local_crate();
            print_crate_info(&this_crate);

            let that_crate = rx.recv().unwrap();
            print_crate_info(&that_crate);

            ControlFlow::<()>::Continue(())
        }
    )
    .unwrap();
});

t1.join().unwrap();
t2.join().unwrap();

fn print_crate_info(krate: &rustc_public::Crate) {
    let tid = std::thread::current().id();

    for func in krate.fn_defs() {
        println!(
            "{tid:?} -- crate {} has function {}",
            krate.name,
            func.name()
        )
    }
}

This prints:

ThreadId(5) -- crate foo has function foo_1
ThreadId(5) -- crate foo has function foo_2
ThreadId(4) -- crate bar has function bar_1
ThreadId(4) -- crate bar has function bar_2
ThreadId(4) -- crate foo has function bar_1
ThreadId(4) -- crate foo has function bar_2

When we move crate foo to a different thread, it gets index'd into a different TLV, so gets results for a different crate. Full code: https://github.com/aDotInTheVoid/rust-148261

[celinval]

Oh, I see. You are building different rust files in each thread.

[celinval]

In this case, I don't think we need a separate macro either

[aDotInTheVoid]

Does anything else need doing before this can be approved?

[celinval]

I'd like to squash this after approval but before merging.

Can you do this please. Thanks!
@bors r+ rollup

[bors]

📌 Commit 7bdbf8e has been approved by celinval

It is now in the queue for this repository.

[aDotInTheVoid]

@bors r-, I’ll squash & re-approve when i’m at a laptop.

[aDotInTheVoid]

@bors r=celinval rollup

[bors]

📌 Commit e7b8460 has been approved by celinval

It is now in the queue for this repository.