Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can/should miri ensure that the addresses of consts/vtables/... are unstable? #1955

Open
saethlin opened this issue Jan 4, 2022 · 11 comments
Labels
A-interpreter Area: affects the core interpreter C-spec-question Category: it is unclear what the intended behavior of Miri for this case is

Comments

@saethlin
Copy link
Member

saethlin commented Jan 4, 2022

In the community Discord, someone recently found a bug in their code (or idea?) because the address of a const value wasn't stable in miri, but was stable in normal execution.

As demonstration, someone posted this example:

const FOO: &str = "";
fn main() {
    println!("{:p}", FOO);
    println!("{:p}", FOO);
}

Which on the playground currently prints

0x25ea8
0x2d638

But then OP responded with this example:

const FOO: &&str = &"";
fn main() {
    println!("{:p}", *FOO);
    println!("{:p}", *FOO);
    println!("{:p}", *FOO);
}

Which on the playground currently prints

0x25e96
0x25e96
0x25e96

This seems less-than-awesome. Is there something that miri can do to make sure that addresses of consts aren't accidentally stable across instantiations? Or at least, are very unlikely to be stable?

@chorman0773
Copy link

The code that was found was https://github.com/LightningCreations/lccc/blob/6377fff74e4de2b98eac1ea78db60baf87d75cbb/xlang/xlang_abi/src/string.rs#L380 (in my project, but not my code directly, credits in the comment).

I'd note that it seems like a potentially huge footgun that rust does not guarantee this, especially since, coming from C++, I wouldn't expect consts to act like basically typechecked macros (although, I'd admit this isn't the place for that discussion - and IRLO thread, or a message in the t-lang or wg-unsafe-code-guidelines stream in zulip would be better).

@RalfJung
Copy link
Member

RalfJung commented Jan 4, 2022

This seems related to #131.

I'd note that it seems like a potentially huge footgun that rust does not guarantee this, especially since, coming from C++, I wouldn't expect consts to act like basically typechecked macros (although, I'd admit this isn't the place for that discussion - and IRLO thread, or a message in the t-lang or wg-unsafe-code-guidelines stream in zulip would be better).

This is somewhat documented at https://github.com/rust-lang/const-eval/blob/master/const.md, but I agree it is subtle. The upshot is that if you care about address identity, you have to use static.

@RalfJung RalfJung added the A-interpreter Area: affects the core interpreter label Jan 4, 2022
@chorman0773
Copy link

The upshot is that if you care about address identity, you have to use static.

Sometimes you can't do that, though. My code was a macro intended to be used for intializating values of type StringView<'static> (a type within xlang abi that provides an abi-stable version of &str) in constant contexts, including for consts and inside const fns, which a static cannot be used to achieve. The version of the macro linked above (which was previously necessary, as it was compatible with rust 1.39, a previous MSRV), suffered from this bug, which I couldn't solve until it was pointed out that the uses of __STR wasn't guaranteed to produce the same value, because I had expected it to do so.

@saethlin
Copy link
Member Author

saethlin commented Jan 4, 2022

Are you saying you want the language changed to accommodate your implementation? I'm very confused

@bjorn3
Copy link
Member

bjorn3 commented Jan 5, 2022

The upshot is that if you care about address identity, you have to use static.

If the const is generic it is even impossible to guarantee address identity as two crates may instantiate it with the same address. Also note that even functions and vtables don't have a guaranteed identity. Vtables are always duplicated per cgu and functions may be duplicated and we tell LLVM that we don't care about address identity for functions.

@RalfJung RalfJung added the C-spec-question Category: it is unclear what the intended behavior of Miri for this case is label Jun 5, 2022
@RalfJung RalfJung changed the title Can/should miri ensure that the addresses of consts are unstable? Can/should miri ensure that the addresses of consts/vtables/... are unstable? Jul 22, 2022
@eddyb
Copy link
Member

eddyb commented Jul 22, 2022

For the record, this is how we handle having a constant of "significant address" in rustc, via static.
It's not pretty, and it does require limiting the alignment since the static can only guarantee so much.

But we can't really guarantee much more without limiting platform support.
I kept going back and forth given anecdotal evidence from people with various levels of experience/knowledge about linker-based deduplication, but what finally convinced me is libcxx's RTTI (typeinfo) implementation.

That is, being able to guarantee linker-based deduplication 100% of the time (e.g. by encoding the data in the symbol, though we'd have to use hashes for really large ones, and then having the linker deduplicate by symbol) is isomorphic to libcxx being able to use the Unique RTTI implementation on all platforms Rust supports.

This is also, FWIW, the same reason we don't offer anything resembling generic/associated statics.
It would be nice if someone could fix all the tooling out there to handle it for both build time and runtime (i.e. guaranteeing deduplication for dynamic loading, through dynamic linkers), but until then, we can't.

@RalfJung
Copy link
Member

Right, so this issue is basically the opposite -- in Miri, each const and vtable has a unique address, and it is non-trivial to change that. ;) But we probably do want to change it to get more test coverage.

@eddyb
Copy link
Member

eddyb commented Jul 22, 2022

Right, so this issue is basically the opposite -- in Miri, each const and vtable has a unique address, and it is non-trivial to change that.

Oh right, I did misunderstand the original issue description as saying the opposite, and just assumed a significant refactor had already happened.


Given we don't want to guarantee either == or !=, one potential (opt-in?) approach here is to make every e.g. Operand::Const use the RNG to decide whether to reuse an existing address (and if so which?) - theoretically this could even recurse and keep RNG for every reference to some non-static constant data.

(or to sound less sill: make every use of a constant reference have non-deterministic ==/!= with any other references to the same data, and then resolve that non-determinism with the RNG)

@RalfJung
Copy link
Member

Yeah, something like that. Though the nondet needs to happen when the & is evaluated, not when the == is evaluated. Or are you saying the following function might sometimes return false?

fn check<T>(x: *const T, y: *const T) -> bool {
  (x == y) == (x == y)
}

@eddyb
Copy link
Member

eddyb commented Jul 22, 2022

Though the nondet needs to happen when the & is evaluated, not when the == is evaluated.

I agree, I was just struggling to describe it precisely enough (that's why I mentioned Operand::Const fwiw).

@Nemo157
Copy link
Member

Nemo157 commented Jul 22, 2022

In the original testing, whether you get a unique address or not depends on what level you evaluate the const to. In the case of an const FOO: &str; you get unique addresses each time you evaluate it, in the case of a const FOO: &&str; you get the same address every time you evaluate *FOO, but this is not guaranteed (and the original idea in our discussion was that you shouldn't get the same address, to make testing code that assumes it gives the same address easier).

tom-anders added a commit to tom-anders/rlox that referenced this issue Mar 29, 2024
We previously assumed that &'static str would always have the same
address, but with miri it seems like this address is not actually stable
(see rust-lang/miri#1955).

So refactor the tests to let it use the StringInterner instead of
constructing InternedString by itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-interpreter Area: affects the core interpreter C-spec-question Category: it is unclear what the intended behavior of Miri for this case is
Projects
None yet
Development

No branches or pull requests

6 participants