Lookup exported statics when encountering an unsupported imported static

[hkalbasi]

Fix #5186

The implemented behavior is fairly liberal and doesn't emit immediate UB as long as allocation size and alignment matches. This enables patterns like this:

#[no_mangle]
static INTERIOR_MUT: SyncUnsafeCell<i32> = SyncUnsafeCell::new(42);

fn main() {
    unsafe {
        extern "C" {
            static mut INTERIOR_MUT: i32;
        }
        (&raw mut INTERIOR_MUT).write(7);
    }
}

I don't know if there is a decided semantic for these statics, tell me if it needs to be changed.

[hkalbasi]

Also, I treated extern "C" and extern "Rust" as equal, which might be wrong.

[RalfJung]

Also, I treated extern "C" and extern "Rust" as equal, which might be wrong.

Hm, good question... @bjorn3 does the "ABI" make any difference for statics?

[RalfJung]

This makes a lot of sense, thank you. :)

@rustbot author

View changes since this review

[RalfJung]

For exported functions, we are very careful to check that the shims we support do not overlap with an exported function. We should do the same here: if a static is both in machine.extern_statics and exported, we should error.

[hkalbasi]

I added this error, I also added check for conflicts between function shims and bad static definitions, and static shims and bad function definitions. Two more cases are left (function shim used by static declaration and static shim used by function declaration), but they don't fit nicely in the miri code, and given that miri can't detect all possible link problems, and that I'm not a fan of this diagnostic, and that it wasn't diagnosed (the function case) before, I tend to not add them, at least in this PR.

[RalfJung]

Two more cases are left (function shim used by static declaration and static shim used by function declaration),

What do you mean by that?

[hkalbasi]

https://godbolt.org/z/r1GbWfz9M

This is accepted currently in miri master, but you may want to reject it (since environ conflicts with miri static shim). This PR improves the situation (by rejecting another case when a fn shim is shadowed by a static), but still allows this one. It is somehow irrelevant to this PR (it has a sister case for static shims which is more relevant) I just noticed it and wanted to share it with you.

[RalfJung]

Yeah that would be better to reject. It seems relevant to this PR to me -- before this PR, Miri entirely ignored the fact that one can also have statics in that global namespace. Now that we're teaching Miri about this we should do it properly: check_shim_symbol_clash should use lookup_exported_symbol (not the new wrappers that filter for functions / statics), and the logic for statics should invoke check_shim_symbol_clash. No need to have separate codepaths. Then we should get the intended behavior fairly easily?

[hkalbasi]

I already use lookup_exported_symbol (that's the case that I added support) and the other one indeed needs a separate code path (or a refactor to put all shims (static and fn) in a single symbol hashmap so that a shim get selected in this case).

[RalfJung]

Oh I see... one of those directions is fairly easy since we have a list of Miri-shimmed statics we can check against, but the other direction is hard since we don't have a list of all Miri-shimmed functions.

For now, let's open an issue and leave a FIXME referencing the issue.

[RalfJung]

@rustbot author

View changes since this review

[RalfJung]

This is actually a good question, I do not know if the declaration is fine. Maybe LLVM can assume that this memory is implicitly writable. I think we should error on any mutability mismatch.

[hkalbasi]

I made it UB. I would be surprised if llvm actually does unprovoked writes, specially since that allocations are mutable as a whole. I don't want llvm to kill my &'static i32 obtained from a (i32, Cell) static, since it thinks the whole allocation is mutable.

But making things UB is easier than doing research. If it bites someone, they can check llvm and relax it if there is no problem.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[RalfJung]

Just a few minor nits. :)

(Also remember to rustbot ready when you want another round of review.)

View changes since this review

[hkalbasi]

@rustbot ready

[RalfJung]

This looks great, thanks! Please squash the commits. You can squash manually if there are multiple independent commits you want to preserve, or use ./miri squash (make sure to pick a suitable commit message). Then write @rustbot ready after you force-pushed the squashed PR.

@rustbot author

[hkalbasi]

@rustbot ready

Btw was the temporary commits helpful in reviewing? I'm using jj and it's easier for me to edit my last commit than to create temporary commits, but IIRC there was a rustbot message inviting people to not do rebases in the middle of reviews. I didn't see that message in this and other PRs. Is this restriction lifted? Or maybe it was another project?

[RalfJung]

We used to have such a message, but these days we have "view changes since review" links for reviewing which is good enough for small PRs like this. So it's fine to force-push, though the diff is still easier to deal with if you only "amend", not "amend + rebase" (or whatever the jj terms for that are).