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

ICE; two sources of truth on how stuff is returned do not agree with each other #38727

Closed
nagisa opened this issue Dec 31, 2016 · 7 comments
Closed
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Dec 31, 2016

Consider this code:

#![feature(no_core, lang_items,start)]
#![no_std]
#![no_core]
#[lang="sized"] trait Sized {}
#[lang="copy"] trait Copy {}

#[repr(u64)]
enum A {
    A = 0u64,
    B = !0u64,
}

fn cmp() -> A {
    A::B
}

#[start]
fn main(b:isize, a: *const *const u8)->isize { 0 }

if compiled targetting i686 target, this ICEs with

error: internal compiler error: ../src/librustc_trans/mir/operand.rs:82: impossible case reached

This is because ABI code claims that this u64 enum has to be returned via value (i.e. ret i64 %smth):

FnType { args: [], ret: ArgType { kind: Direct, original_ty: i64, ty: i64, signedness: Some(false), cast: None, ... }

Whereas MIR only has OperandRef(Ref(...)) because the common::is_immediate_type returns false for this enum. Calling immediate() on this OperandRef then causes this ICE.

This is a regression from the old->MIR trans move.

@brson brson added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 31, 2016
nagisa added a commit to est31/rust that referenced this issue Dec 31, 2016
Since discriminants do not support i128 yet, lets just calculate the boundaries within the 64 bits
that are supported. This also avoids an issue with bootstrapping on 32 bit systems due to rust-lang#38727.
@nagisa nagisa changed the title ICE; two sources of truth on how stuff is returned does not agree with each other ICE; two sources of truth on how stuff is returned do not agree with each other Dec 31, 2016
@nikomatsakis
Copy link
Contributor

cc @eddyb, who has done a lot of poking at this infrastructure.

@nagisa, did you have a feeling for which is correct?

@eddyb
Copy link
Member

eddyb commented Jan 4, 2017

Ugh, .immediate() is so risky. IMO the correct thing to do is to force a load if it's by-ref (can't remember exactly what APIs are available atm, but worst-case it's a match with bcx.load(llref) for Ref(llref)).

@nagisa
Copy link
Member Author

nagisa commented Jan 4, 2017

Why common::is_immediate_type returns false here? Can we somehow unify the codepath that figure out expected immediate-ness of return value in signature and the codepath which does actual translation?


Or actually, why is it decided that the value ought to be returned by value? Its a 64 bit return on a 32 bit target. Seems sketchy?

@eddyb
Copy link
Member

eddyb commented Jan 5, 2017

Both the ABI and the immediate check are correct IMO.

The problem isn't that they don't agree, the problem is that we have code that incorrectly assumes that they always do.

The ABI rules can be weird and we might even end up supporting several conflicting calling convention ABIs per platform, so we should just do the load there instead of trying to force it to be immediate.

@nikomatsakis
Copy link
Contributor

@eddyb I haven't really looked at the code in question, but do you (or @nagisa) have a fix in mind?

@eddyb
Copy link
Member

eddyb commented Jan 5, 2017

Yes, @Mark-Simulacrum has just started playing with it. The short version is "always load if you need an immediate instead of panicking".

@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added the P-high High priority label Jan 5, 2017
bors added a commit that referenced this issue Jan 7, 2017
Fix ICE on i686 when calling immediate() on OperandValue::Ref in return

Fixes #38727, and adds a test case.

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

5 participants