-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
LGTM. 👍 |
@bors r+ |
📌 Commit dd53fa3 has been approved by |
} | ||
|
||
#[rustc_mir] | ||
fn test10(i: i32, j: i32, k: i32) -> Vec<raw::c_char> { |
There was a problem hiding this comment.
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.
@bors r- The test causes some sort of heap corruption on Windows. |
Testing that test in the x86_64 msvc nightly without the |
dd53fa3
to
54af95a
Compare
@bors try |
Supersedes #30517 Fixes #29575 cc @luqmana r? @nikomatsakis
54af95a
to
38bed95
Compare
@bors try- (we have no MSVC try bots) |
@bors r=nikomatsakis (adjusted the test with MSVC workarounds, sending to buildbot to see how it goes this time around) |
📌 Commit 38bed95 has been approved by |
@bors retry force |
@bors r- try- r=nmatsakis retry force p=1 38bed95 Hmm. |
📌 Commit 38bed95 has been approved by |
⌛ Testing commit 38bed95 with merge eaa5059... |
💔 Test failed - auto-win-msvc-64-opt |
The reason has been narrowed down. The original trans also fails to translate that test, as has been shown by #30921 and its result. |
Okay, so the reason ends up being |
edbf99f
to
627b886
Compare
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) |
OK
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? |
259a483
to
da3bfa0
Compare
da3bfa0
to
99e8b4d
Compare
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. |
@bors r+ |
📌 Commit 99e8b4d has been approved by |
Supersedes #30517 Fixes #29575 cc @luqmana r? @nikomatsakis
Supersedes #30517
Fixes #29575
cc @luqmana
r? @nikomatsakis