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.

Sign up for GitHub

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

Jump to bottom

Fix ICE: inject bitcast if types mismatch for invokes/calls/stores #37112

Merged
merged 3 commits into from
Oct 17, 2016
Merged

Fix ICE: inject bitcast if types mismatch for invokes/calls/stores #37112

merged 3 commits into from
Oct 17, 2016

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Oct 12, 2016 •
edited by arielb1
Loading

Fix ICE: inject bitcast if types mismatch for invokes/calls

Partial fix for #36744

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix
Copy link
Member Author

(oh I forgot a regression test; will put into followup commit.)

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 12, 2016 •
edited
Loading

So I just realized that this fix as originally written only handles the case of calls, which does fix the specific report of #36744 but does not address the other cases that @arielb1 pointed out here: #36744 (comment)

I'm looking into generalizing it slightly now to deal with more of those.

Ideally we would handle the following cases (I will check off the ones that the PR handles as I go):

  • call arguments (when parameters have non-aggregate type)
  • stores (of any kind)
  • call arguments of aggregate type (cannot use naive bitcast...)
    • hmm, I have not yet managed to create an test case that exposes the issue I worry about here.
    • niko notes that we don't pass tuples directly in calls; we instead allocate the tuple locally and pass a pointer to it.
    • ariel notes that stores can happen via intrinsics or elsewhere when one has RVO.
  • call arguments of aggregate type (#[repr(C)] struct) to an extern "C" fn
    • this is an exception to above claim that aggregates are handled via passing pointers, and will not be handled properly in bitcasts
  • return instruction
    • It looks like this case is covered by the code I added for building store instructions
  • I'm also considering taking a different tack of extending MIR with an explicit upcast operator so that trans can remain simple. But I would be less inclined to back port a PR that takes that route.

@pnkfelix
Copy link
Member Author

While working on this PR, I realized that one flaw in this strategy is that you can not apply the LLVM bitcast instruction to aggregates. This may mean that we have other cases to handle that simply cannot be dealt with via the means used here.

@pnkfelix
Copy link
Member Author

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned Aatch Oct 13, 2016
@pnkfelix pnkfelix changed the title Fix ICE: inject bitcast if types mismatch for invokes/calls Fix ICE: inject bitcast if types mismatch for invokes/calls/stores Oct 13, 2016
@pnkfelix
Copy link
Member Author

FYI @arielb1 mentioned over IRC discussion that he would prefer a more targeted fix oriented around changes to MIR-trans.

@brson brson mentioned this pull request Oct 14, 2016
Merged
@pnkfelix
Copy link
Member Author

@nikomatsakis @arielb1 :

We should decide whether we are going to land this patch or not.

my opinion is that we should put this PR in. It solves the bulk of the occurrences of this problem (see above comment), and guards against adding the bitcast when it is unneeded by first checking if the types already match.

@arielb1
Copy link
Contributor

arielb1 commented Oct 17, 2016 •
edited
Loading

Sure. My patch turned out to be more complicated than it seems.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2016

📌 Commit 0562654 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Oct 17, 2016

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Oct 17, 2016

📌 Commit 0562654 has been approved by arielb1

bors added a commit that referenced this pull request Oct 17, 2016
@bors
Auto merge of #37112 - pnkfelix:fix-issue-36744, r=arielb1
ce31626 
Fix ICE: inject bitcast if types mismatch for invokes/calls/stores

Fix ICE: inject bitcast if types mismatch for invokes/calls

Fix #36744
@bors
Copy link
Contributor

bors commented Oct 17, 2016

⌛ Testing commit 0562654 with merge ce31626...

nikomatsakis
nikomatsakis approved these changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me modulo nit

src/librustc_trans/builder.rs

fn check_call(typ: &str, llfn: ValueRef, args: &[ValueRef]) {
if cfg!(debug_assertions) {
fn check_call<'b>(&self,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a comment here would be nice-- what is this Cow that is being returned?

@nikomatsakis
Copy link
Contributor

(Oh, I see @arielb1 already r+'d. Seems fine.)

@bors
Copy link
Contributor

bors commented Oct 17, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: arielb1
Pushing ce31626 to master...

@bors bors merged commit 0562654 into rust-lang:master Oct 17, 2016
@brson brson added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 18, 2016
@brson brson mentioned this pull request Oct 18, 2016
Merged
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 18, 2016
@nikomatsakis
Copy link
Contributor

Accepting for beta. Low risk patch, regression.

cc @rust-lang/compiler

@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 18, 2016
@nikomatsakis nikomatsakis mentioned this pull request Oct 18, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pnkfelix @rust-highfive @arielb1 @bors @nikomatsakis @brson @Aatch