Skip to content

Bug in optimiser(maybe) #22008

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

Closed
eklavya opened this issue Feb 6, 2015 · 5 comments · Fixed by #22385
Closed

Bug in optimiser(maybe) #22008

eklavya opened this issue Feb 6, 2015 · 5 comments · Fixed by #22385
Labels
A-codegen Area: Code generation P-medium Medium priority

Comments

@eklavya
Copy link

eklavya commented Feb 6, 2015

Hi,

The rust optimiser or some other component is not behaving correctly resulting in wrong results.
I have produced a small example here : https://github.com/eklavya/rust-bug

This was reproduced on
Mac yosemite:
cargo 0.0.1-pre-nightly (2b9a3d6 2015-01-30 02:47:30 +0000)
rustc 1.0.0-nightly (3b2ed14 2015-02-03 14:56:32 +0000)
and
centos7 x64, rustc 1.0.0-nightly (706be5b 2015-02-05 23:14:28 +0000)

Please let me know if you need any more details.

@renato-zannon
Copy link
Contributor

Reduced test case:

fn main() {
    let command = "a";

    match command {
        "foo" => println!("foo"),
        _     => println!("{}", command),
    }
}

This segfaults with opt-level >= 2 (on Linux x86-64, rustc 1.0.0-dev (f3573aa83 2015-02-06 05:52:20 +0000))

@kmcallister kmcallister added A-codegen Area: Code generation I-nominated labels Feb 6, 2015
@renato-zannon
Copy link
Contributor

This goes back to at least 0.12 - though it prints garbage values instead of segfaulting. 0.11 seems unaffected.

@eddyb
Copy link
Member

eddyb commented Feb 7, 2015

I've done some hacky introspection.
I'm not seeing the original data in the IR.
cc @dotdash Is this another memcpy metadata bug? Or a lifetime intrinsic issue.

EDIT: turns out str::eq_slice, which is used here, calls llvm.lifetime.end on its arguments, as if it was certain they were temporaries that didn't outlive the call (which is wrong, in this case).
Not sure if trans::_match needs to create a temporary for the value it's trying to compare with a constant, or the lifetime intrinsic calls shouldn't be emitted for str::eq_slice.

@dotdash
Copy link
Contributor

dotdash commented Feb 7, 2015

This has two parts. When I extended rustc to emit lifetime intrinsics, I chose to put a call to lifetime.end at the end of functions to mark arguments as dead that are passed by value on the stack. This allows LLVM to remove dead stores at the end of functions that it normally can't remove. This works because normally, the caller makes a copy of the value it passes in.

Now, the call that match generates to compare the strings is to eq_slice, which accepts two slices by-value, and slices are large enough to be passed on the stack. Thus, calls to lifetime.end are emitted. But, the call the match generates does not actually make copies of the slices, but passes in pointers to the original slices. That means that the lifetime.end call marks the original alloca as dead, and then things fall apart.

Question: Do we want to drop the lifetime.end call for by-value on-stack arguments, or fix the call to eq_slice to make copies?

cc @nikomatsakis @brson

@pnkfelix
Copy link
Member

just-a-bug; P-high, not 1.0 milestone.

@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Feb 12, 2015
dotdash added a commit to dotdash/rust that referenced this issue Feb 15, 2015
When matching against strings/slices, we call the comparison function
for strings, which takes two string slices by value. The slices are
passed in memory, and currently we just pass in a pointer to the
original slice. That can cause misoptimizations because we emit a call
to llvm.lifetime.end for all by-value arguments at the end of a
function, which in this case marks the original slice as dead.

So we need to properly create copies of the slices to pass them to the
comparison function.

Fixes rust-lang#22008
Manishearth added a commit to Manishearth/rust that referenced this issue Feb 17, 2015
…tsakis

When matching against strings/slices, we call the comparison function
for strings, which takes two string slices by value. The slices are
passed in memory, and currently we just pass in a pointer to the
original slice. That can cause misoptimizations because we emit a call
to llvm.lifetime.end for all by-value arguments at the end of a
function, which in this case marks the original slice as dead.

So we need to properly create copies of the slices to pass them to the
comparison function.

Fixes rust-lang#22008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants