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

[MIR] Implement extern call support #30845

Merged
merged 1 commit into from
Jan 19, 2016
Merged

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jan 12, 2016

Supersedes #30517
Fixes #29575

cc @luqmana
r? @nikomatsakis

@luqmana
Copy link
Member

luqmana commented Jan 12, 2016

LGTM. 👍

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 12, 2016

📌 Commit dd53fa3 has been approved by nikomatsakis

@Manishearth
Copy link
Member

}

#[rustc_mir]
fn test10(i: i32, j: i32, k: i32) -> Vec<raw::c_char> {
Copy link
Member Author

Choose a reason for hiding this comment

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

So… this test seems to cause STATUS_HEAP_CORRUPTION on Windows… Hmm.

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors r-

The test causes some sort of heap corruption on Windows.

@retep998
Copy link
Member

Testing that test in the x86_64 msvc nightly without the#[rustc_mir], I failed to replicate any sort of heap corruption.

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors try

bors added a commit that referenced this pull request Jan 14, 2016
@bors
Copy link
Contributor

bors commented Jan 14, 2016

⌛ Trying commit 54af95a with merge 29d24a3...

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors try-

(we have no MSVC try bots)

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors r=nikomatsakis

(adjusted the test with MSVC workarounds, sending to buildbot to see how it goes this time around)

@bors
Copy link
Contributor

bors commented Jan 14, 2016

📌 Commit 38bed95 has been approved by nikomatsakis

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors retry force

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2016

@bors r- try- r=nmatsakis retry force p=1 38bed95

Hmm.

@bors
Copy link
Contributor

bors commented Jan 14, 2016

📌 Commit 38bed95 has been approved by nmatsakis

@bors
Copy link
Contributor

bors commented Jan 14, 2016

⌛ Testing commit 38bed95 with merge eaa5059...

@bors
Copy link
Contributor

bors commented Jan 14, 2016

💔 Test failed - auto-win-msvc-64-opt

@nagisa
Copy link
Member Author

nagisa commented Jan 15, 2016

The reason has been narrowed down. The original trans also fails to translate that test, as has been shown by #30921 and its result.

@nagisa
Copy link
Member Author

nagisa commented Jan 15, 2016

Okay, so the reason ends up being LLVM itself. I’ll just ignore the test in question on msvc like it is done in the other related test for now. windows itself being broken. Case solved!

@nagisa nagisa force-pushed the mir-extern-calls branch 2 times, most recently from edbf99f to 627b886 Compare January 15, 2016 17:55
@nagisa
Copy link
Member Author

nagisa commented Jan 15, 2016

I couldn’t reproduce the original failure with VS2013-based rustc locally – the test runs, produces expected results and quits successfully. In the current iteration, the test is disabled on msvc (just like the regular-trans variadic-ffi) and the PR should pass through build bots this time. That being said, the original error is still unsettling and I would like to try sending re-enabled test over to msvc bots again. Is that fine?

(Trivia: I once had encountered a SIGSEGV on a unix builder, which turned out to be spurious, before, so the original failure could also be spurious in some way? /me shrugs)

@nikomatsakis
Copy link
Contributor

@nagisa

In the current iteration, the test is disabled on msvc (just like the regular-trans variadic-ffi)

OK

That being said, the original error is still unsettling and I would like to try sending re-enabled test over to msvc bots again

OK -- do you want to land this PR, then open another that re-enables the test? We can also use the try bots, in that case.

@nagisa
Copy link
Member Author

nagisa commented Jan 19, 2016

OK -- do you want to land this PR, then open another that re-enables the test? We can also use the try bots, in that case.

I think what I’d like to do is try to land this with the test enabled now, and then, provided this PR manages to land, disable it for MSVC in a follow-up PR. The test has to stay disabled, because otherwise it will break everybody’s MSVC-2015-based setups (remember, in that particular version of MSVC sprintf does not exist as a function).

Does that sound good to you?

@nagisa nagisa force-pushed the mir-extern-calls branch 3 times, most recently from 259a483 to da3bfa0 Compare January 19, 2016 13:13
@nagisa
Copy link
Member Author

nagisa commented Jan 19, 2016

Upon suggestion of @dotdash I changed the test to use our own variadic function (defined inside rt/rust_test_helpers.c) instead so we don’t have to worry about the MSVC strangeness.

@dotdash
Copy link
Contributor

dotdash commented Jan 19, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jan 19, 2016

📌 Commit 99e8b4d has been approved by dotdash

bors added a commit that referenced this pull request Jan 19, 2016
@bors
Copy link
Contributor

bors commented Jan 19, 2016

⌛ Testing commit 99e8b4d with merge 41b74b1...

@bors bors merged commit 99e8b4d into rust-lang:master Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants