Extend the BorTag GC to AllocIds

[saethlin]

As suggested in #3080 (comment)

We previously solved memory growth issues associated with the Stacked Borrows and Tree Borrows runtimes with a GC. But of course we also have state accumulation associated with whole allocations elsewhere in the interpreter, and this PR starts tackling those.

To do this, we expand the visitor for the GC so that it can visit a BorTag or an AllocId. Then when the GC runs, we use the reachable set of AllocIds to remove elements from the intptrcast state.

[RalfJung]

@rustbot author

[bors]

☔ The latest upstream changes (presumably #3122) made this pull request unmergeable. Please resolve the merge conflicts.

[saethlin]

This isn't ready, I'm still thinking about your two comments I didn't mark as resolved.

[RalfJung]

@rustbot author

[RalfJung]

We also have the secondary memory map in tcx. I guess we have to walk that one as well, judging from the test failures. The comment I suggested about not having to worry about tcx-managed allocation seems to be wrong.

In particular it is wrong for function and vtable allocations, which never get copied into the local alloc_map.

[saethlin]

CI has been running for an hour. That's not good. I'll look into it.

[saethlin]

The slow part of the CI is inserting all the live AllocIds into the reachable set. Aside from asking the interpreter to track a FxHashSet<AllocId> for us then hand it to the GC via clone I'm not sure what else to do other than run the GC less often, but the CI is slow because we're running the GC super often. Maybe trim down the size of that test?

The cde that's causing the GC to lose track of a pointer is here, and it has a wild diagnostic:

error: Undefined Behavior: using alloc850+0xfffffffffffffc91 as function pointer but it does not point to a function
  --> /home/ben/.cargo/registry/src/index.crates.io-6f17d22bba15001f/getrandom-0.1.16/src/macos.rs:21:32
   |
LL |             let ret = unsafe { func(chunk.as_mut_ptr(), chunk.len()) };
   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ using alloc850+0xfffffffffffffc91 as function pointer but it does not point to a function

https://github.com/rust-random/getrandom/blob/bd14b7a243c7cda78f698742a84d1a0bbfcba972/src/macos.rs#L14-L21

type GetEntropyFn = unsafe extern "C" fn(*mut u8, libc::size_t) -> libc::c_int;

pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
    static GETENTROPY: Weak = unsafe { Weak::new("getentropy\0") };
    if let Some(fptr) = GETENTROPY.ptr() {
        let func: GetEntropyFn = unsafe { mem::transmute(fptr) };
        for chunk in dest.chunks_mut(256) {
            let ret = unsafe { func(chunk.as_mut_ptr(), chunk.len()) };

[RalfJung]

The slow part of the CI is inserting all the live AllocIds into the reachable set. Aside from asking the interpreter to track a FxHashSet for us then hand it to the GC via clone I'm not sure what else to do other than run the GC less often, but the CI is slow because we're running the GC super often. Maybe trim down the size of that test?

That test? We run all tests on Linux with "do GC after every step".

miri/.github/workflows/ci.yml

Lines 38 to 40 in d36c3e7

	- name: Set the tag GC interval to 1 on linux
	if: runner.os == 'Linux'
	run: echo "MIRIFLAGS=-Zmiri-tag-gc=1" >> $GITHUB_ENV

And that seems to be good, we caught a bug?

For performance, here's one idea: we don't actually need the set. We need a function fn(AllocId) -> bool that tells us whether the AllocId is used anywhere. So instead of adding all live allocations to the set, we can make that function a 3-way disjunction, where it checks the global alloc_map, the local alloc_map, and the collected set. And then we don't need to put the live allocations into the set. That might help?

[RalfJung]

The cde that's causing the GC to lose track of a pointer is here, and it has a wild diagnostic:

Ah, that's the old macOS getrandom doing strange integer pointer shenanigans. If this triggers a GC bug then the first thing to do is extract it into a dedicated test so that we don't have to rely on a pass-dep test to catch this. It should then also reproduce on all targets; the reason it's macOS-only is that getrandom uses different code for different targets.

[saethlin]

Here's a minimization:

#![feature(rustc_private)]
extern crate libc;

extern "Rust" {
    pub fn miri_run_provenance_gc();
}

type GetEntropyFn = unsafe extern "C" fn(*mut u8, libc::size_t) -> libc::c_int;

fn main() {
    let name = "getentropy\0";
    let addr = unsafe { libc::dlsym(libc::RTLD_DEFAULT, name.as_ptr() as *const _) as usize };
    unsafe {
        miri_run_provenance_gc();
    }

    let ptr = addr as *mut libc::c_void;
    let func: GetEntropyFn = unsafe { std::mem::transmute(ptr) };
    let dest = &mut [0u8];
    unsafe { func(dest.as_mut_ptr(), dest.len()) };
}

Run with ./miri run -Zmiri-tag-gc=0 to get the error we get in CI.

[saethlin]

Note that this code runs fine without explicitly invoking the provenance GC, because in the minimization there is no basic block end in the important region of the program. I always add code to run the GC on every machine step while debugging to avoid getting tripped up by this.

Lol that's not even right I forgot that the default is to run sparingly 🤦

[saethlin]

Ah! I think the GC isn't picking up the contents of extra_fn_ptr_map: https://github.com/rust-lang/rust/blob/4d7f952a02d0bca67c98a6b74895b7e3fbe38341/compiler/rustc_const_eval/src/interpret/memory.rs#L106

[saethlin]

I think this is going to be easier to do as a rustc PR, so that I can iterate on the interface between Miri and the interpreter core code. I'll set up that later today.

[RalfJung]

I think this is going to be easier to do as a rustc PR, so that I can iterate on the interface between Miri and the interpreter core code. I'll set up that later today.

However, rustc doesn't run all tests with -Zmiri-tag-gc=1.

(We should probably rename that flag in this PR as well...)

[RalfJung]

Ah! I think the GC isn't picking up the contents of extra_fn_ptr_map: https://github.com/rust-lang/rust/blob/4d7f952a02d0bca67c98a6b74895b7e3fbe38341/compiler/rustc_const_eval/src/interpret/memory.rs#L106

Aha!

If we follow my proposal, the API we need is an efficient "is this AllocId live". So that's a single fn(&self, AllocId) -> bool, I think. Basically a more efficient version of get_alloc_info that doesn't compute the size. Maybe we just want that function to be split into two, one for size+align and one for AllocKind?

[saethlin]

Subsumed by rust-lang/rust#118029