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

trait that omits & on self type signals LLVM assertion failure #4406

Closed
jbclements opened this issue Jan 9, 2013 · 6 comments
Closed

trait that omits & on self type signals LLVM assertion failure #4406

jbclements opened this issue Jan 9, 2013 · 6 comments
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-traits Area: Trait system I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️

Comments

@jbclements
Copy link
Contributor

Compiling this program:

trait BogusTrait {
  fn its1(self) -> int;
}

impl BogusTrait for int {
  fn its1(self) -> int {1}
}

#[test] fn t1 () {assert! ((13).its1() == 1);}

produces this error message:

% ./x86_64-apple-darwin/stage2/bin/rustc --test /tmp/bug4.rs
Assertion failed: (S->getType()->isPtrOrPtrVectorTy() && "Invalid cast"), function CreatePointerCast, file /Users/fklock/Dev/Mozilla/rust.git/src/llvm/lib/IR/Instructions.cpp, line 2394.
Abort trap: 6

(Original bug report follows)

Compiling this program:

trait BogusTrait {
  fn its1(self) -> int;
}

impl int: BogusTrait {
  fn its1(self) -> int {1}
}

#[test] fn t1 () {assert 13.its1() == 1;}

Produces this error message:

jclements-09740:rust-experimenting clements$ rustc --test trait-testing.rs
Assertion failed: (S->getType()->isPointerTy() && "Invalid cast"), function CreatePointerCast, file /Users/clements/rust/src/llvm/lib/VMCore/Instructions.cpp, line 2383.
Abort trap: 6
@ghost ghost assigned catamorphism Feb 20, 2013
@pnkfelix
Copy link
Member

I just ran into this. I don't think your trait is bogus, just in case that was implied by its name; by-value Self is part of the language (see #5086). (Though the fact that we assert-fail obviously is bogus.)

@nikomatsakis
Copy link
Contributor

I agree, nothing bogus about this, just a bug.

@nikomatsakis
Copy link
Contributor

Not critical for 0.6; de-milestoning

@jdm
Copy link
Contributor

jdm commented Mar 24, 2013

Probably the same problem as #5321.

@pnkfelix
Copy link
Member

I believe I just ran into this again (while attempting to double-check if the ICE for Issue #4776 has indeed gone away), though the assertion error message has changed slightly.

% cat /tmp/bug2.rs
trait Op<T> { fn call(self, T, T) -> T; }

impl<T> Op<T> for int   { fn call(self,  x:T, _y:T) -> T { x } }
impl<T> Op<T> for float { fn call(self, _x:T,  y:T) -> T { y } }

fn main() {
    let a : int   = 3i;
    let b : float = 4f;
    io::println(fmt!("a: %d", a.call(4,5)));
    io::println(fmt!("b: %d", b.call(4,5)));
}
% ./x86_64-apple-darwin/stage2/bin/rustc /tmp/bug2.rs
Assertion failed: (S->getType()->isPtrOrPtrVectorTy() && "Invalid cast"), function CreatePointerCast, file /Users/fklock/Dev/Mozilla/rust.git/src/llvm/lib/IR/Instructions.cpp, line 2394.
Abort trap: 6

(yes, actually, by porting John's original test forward to modernized syntax, I confirmed that the error message has changed on the LLVM side, it seems).

dotdash added a commit to dotdash/rust that referenced this issue Jun 29, 2013
Currently we pass all "self" arguments by reference, for the pointer
variants this means that we end up with double indirection which causes
a unnecessary performance hit.

The fix itself is pretty straight-forward and just means that "self"
needs to be handled like any other argument, except for by-value "self"
which still needs to be passed by reference. This is because
non-pointer types can't just be stuffed into the environment slot which
is used to pass "self".

What made things tricky is that there was also a bug in the typechecker
where the method map entries are created. For type impls, that stored
the base type instead of the actual self-type in the method map, e.g.
Foo instead of &Foo for &self. That worked with pass-by-reference, but
fails with pass-by-value which needs the real type.

Code that makes use of methods seems to be about 10% faster with this
change. Also, build times are reduced by about 4%.

Fixes rust-lang#4355, rust-lang#4402, rust-lang#5280, rust-lang#4406 and rust-lang#7285
bors added a commit that referenced this issue Jun 29, 2013
Currently we pass all "self" arguments by reference, for the pointer
variants this means that we end up with double indirection which causes
a unnecessary performance hit.

The fix itself is pretty straight-forward and just means that "self"
needs to be handled like any other argument, except for by-value "self"
which still needs to be passed by reference. This is because
non-pointer types can't just be stuffed into the environment slot which
is used to pass "self".

What made things tricky is that there was also a bug in the typechecker
where the method map entries are created. For type impls, that stored
the base type instead of the actual self-type in the method map, e.g.
Foo instead of &Foo for &self. That worked with pass-by-reference, but
fails with pass-by-value which needs the real type.

Code that makes use of methods seems to be about 10% faster with this
change. Also, build times are reduced by about 4%.

Fixes #4355, #4402, #5280, #4406 and #7285
@thestinger
Copy link
Contributor

Fixed by #7452.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-traits Area: Trait system I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️
Projects
None yet
Development

No branches or pull requests

6 participants