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

Miscompilation of mutable reference to const temporary #31234

Closed
jethrogb opened this Issue Jan 27, 2016 · 18 comments

Comments

Projects
None yet
8 participants
@jethrogb
Copy link
Contributor

jethrogb commented Jan 27, 2016

trait XLen {
    fn xlen(&mut self) -> usize;
}

impl<'a> XLen for &'a [u8] {
    fn xlen(&mut self) -> usize {
        self.len()
    }
}

fn main() {
    const A: &'static [u8] = b"x";
    println!("{} {}",A.len(),A.xlen());
}

This code compiles, but yields:

1 2097865012304223517

The second number may vary.

This used to lead to segfaults (in 1.5.0 and earlier) in code such as:

use std::io::Read;
fn main() {
    const A: &'static [u8] = b"x";
    let mut a=[0u8;32];
    A.read(&mut a);
}

This doesn't segfault in 1.6.0 anymore, but that is due to an unrelated change.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jan 27, 2016

@nikomatsakis I wish there was an easy way to dump the const qualifications as annotations on expressions.

My guess is that we're missing the borrow somehow, or if const qualification is correct, then maybe the const handling code in trans::expr isn't.

@jethrogb jethrogb changed the title Can call methods with &mut receiver on constants Miscompilation of mutable reference to const temporary Jan 27, 2016

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Jan 27, 2016

Using {A} instead of A yields the correct results in both examples.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jan 27, 2016

Using &'static usize instead of a slice, I've found this IR (simplified for presentation):

@ref = constant i64 0
@const = constant i64* @ref
...
  %1 = load i64**, i64*** bitcast (i64** @const to i64***)
  %2 = call i64 @method(i64** %1)

Which is roughly the same as:

@ref = constant i64 0
...
  %2 = call i64 @method(i64** bitcast (i64* @ref to i64**))

So trans::expr is taking an A: &T constant and casting it to &mut &T without actually performing the autoref (which involves taking a &mut to a stack temporary containing the &T).

cc @dotdash

@dotdash dotdash self-assigned this Jan 27, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 2, 2016

Just to be clear: the problem is in trans, right? That is, I expect this example to compile, but it seems like the temporary stack slot is being dropped too early?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 2, 2016

@nikomatsakis Yes, it's supposed to compile, and no, there is no temporary stack slot, &'static T is effectively transmuted to &mut &T by some incorrect adjustment handling.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 3, 2016

@eddyb

no, there is no temporary stack slot,

But there ought to be one, right? The second call requires an autoref (it is an &self method defined on a &[T]) -- this creates a temporary (which contains an &'static value).

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 3, 2016

@nikomatsakis There should be, but there isn't. That's the bug.

@dotdash dotdash removed their assignment Mar 4, 2016

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Apr 2, 2016

Ping. Can someone at least assign some labels to this issue?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 7, 2016

In theory this is probably fixed with -Z orbit, we should test.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 7, 2016

Given that this works with -Z orbit, assigning low priority. Will be fixed in MIR transition.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 7, 2016

triage: P-low

@sanmai-NL

This comment has been minimized.

Copy link

sanmai-NL commented Feb 19, 2017

It appears this issue is related to one I have. Why is it possible to make mutable references to const items but not to let-bound items? This is very counterintuitive (at least to a beginner) and seems hardly productive to me.

See this playground: https://is.gd/6eVLzl

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 19, 2017

You shouldn't be able to get a mutable reference that you can use to mutate the constant but instead a copy (e.g. just like you can do &mut 0).

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Feb 19, 2017

This issue has been fixed in the MIR rewrite.

@sanmai-NL What you're seeing is not related and is also working as intended. A const does not define a variable but rather you can think of it as an “alias” for writing out the const value in place. Whereever you are using MY_STRUCT in your code, you are creating a temporary with the value specified. Temporaries are mutable.

@jethrogb jethrogb closed this Feb 19, 2017

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 19, 2017

There are two issues about issuing warnings for this situation - rust-lang/rust-clippy#829 and rust-lang/rfcs#885.

@sanmai-NL

This comment has been minimized.

Copy link

sanmai-NL commented Feb 20, 2017

@eddyb: In my understanding, the struct in my example has move semantics, so creating a temporary in the way shown should move it and I should be unable to use the constant after creating the mutable reference to the temporary, no? (I'm just sounding off my intuition here, I expect this analysis to be wrong.)

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

jethrogb commented Feb 20, 2017

Every time you write MY_STRUCT you're instantiating a new temporary with the value MyStruct { title: "Original" }. Again, it's exactly the same as if you would just write MyStruct { title: "Original" } instead.

@sanmai-NL

This comment has been minimized.

Copy link

sanmai-NL commented Feb 20, 2017

@jethrogb: Thanks for explaining.

From what I read in the Rust book:

const items are not necessarily guaranteed to refer to the same memory address

, which is misleading if they practically always refer to a different address in memory. Your issue is not a docs issue, but I hope I may take the opportunity to point out another confusing or misleading sentence there:

Constants live for the entire lifetime of a program.

They don't, they do not have a single lifetime at all, they have lifetimes corresponding to each scope in which they are written (not ‘used’).

I really wonder what the use of const items is when it is alternatively possible to write a placeholder macro that provides a literal? const items cannot be used as literals with e.g. std::format!, they are distinct from literals in disadvantageous ways to programmers, but in the ‘mutably referencing constants’ case they behave similarly to literals.

(Okay now, I won't discuss this side issue further even in response.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.