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
Closed

Miscompilation of mutable reference to const temporary #31234

jethrogb opened this issue Jan 27, 2016 · 18 comments
Labels
P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jethrogb
Copy link
Contributor

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
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
Copy link
Contributor Author

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

@eddyb
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
Copy link
Contributor

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
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
Copy link
Contributor

@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
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
Copy link
Contributor Author

jethrogb commented Apr 2, 2016

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

@sfackler sfackler added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 2, 2016
@nikomatsakis
Copy link
Contributor

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

@nikomatsakis
Copy link
Contributor

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

@nikomatsakis
Copy link
Contributor

triage: P-low

@sanmai-NL
Copy link

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
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
Copy link
Contributor Author

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.

@petrochenkov
Copy link
Contributor

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

@sanmai-NL
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
Copy link
Contributor Author

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
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
Labels
P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants