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

Use fibers on windows #8

Merged
merged 17 commits into from
Dec 10, 2018
Merged

Use fibers on windows #8

merged 17 commits into from
Dec 10, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 23, 2018

No description provided.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks! My main hesitation here continues to be that there's no comments anywhere and I have no idea how any of this works, so some comments would be greatly welcome!

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
}
} else {
#[inline(always)]
fn get_thread_stack_guarantee() -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

Is this used because aren't all windows platforms x86/x86_64 righ tnow basically?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did see someone trying to use Rust on ARM64

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/arch/windows.c Outdated Show resolved Hide resolved
src/arch/i686.S Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 23, 2018

My main hesitation here continues to be that there's no comments anywhere and I have no idea how any of this works, so some comments would be greatly welcome!

That's why I didn't want to touch that PR :/

cc @Zoxc can you do a first round of documentation, maybe even with some links into windows-documentation? If it's completely out of your cache I can start reading up on the docs myself though.

src/lib.rs Outdated Show resolved Hide resolved
src/arch/x86_64.asm Outdated Show resolved Hide resolved
src/arch/i686.asm Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 28, 2018

@alexcrichton we lost appveyor on this repo it seems?

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 29, 2018

@Zoxc can you review the comments I added in the code?

src/lib.rs Outdated
cfg_if! {
if #[cfg(any(target_arch = "x86_64", target_arch = "x86"))] {
#[inline(always)]
// We cannot know the initial stack size on x86
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zoxc this seems suspicious.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 30, 2018

@alexcrichton deadlines are coming up fast. Do you think we could merge this and address additional documentation later?

@alexcrichton
Copy link
Member

I think this is way too high risk to land in the edition release, and I would still personally prefer that someone takes the time to understand and document this.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 6, 2018

mingw failures are probably related to rust-lang/rust#53454

@alexcrichton
Copy link
Member

Ok great! I'll take a closer look at this on Monday where I should have access to a windows computer and can dig in a bit more

@mati865
Copy link
Contributor

mati865 commented Dec 9, 2018

Appveryor is configured to use headers from MSYS2 and libs copied from mingw-builds GCC 7.2 package. This ABI mismatch is only getting worse as the time goes by.
Oldest issue about this is rust-lang/rust#47048

Another error (this time caused by this PR) is multiple definition of __stacker_black_box() which is defined in both .S and .c files.

src/arch/asm.h Outdated
@@ -1,4 +1,4 @@
#if defined(APPLE) || (defined(WINDOWS) && defined(X86))
Copy link
Contributor

Choose a reason for hiding this comment

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

32 bit Windows does use additional leading _, you'll have to revert this change.

fn __stacker_black_box(t: *const u8);
}
#[inline(never)]
fn __stacker_black_box(_: *const u8) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a not a black box. LLVM can see what is inside ;)

@alexcrichton
Copy link
Member

Alright I've done a once-over of all this and it all looks good to me. Local testing looks good too although I'm not configured locally to do all the MinGW targets. That's what we have bors for though!

Thanks again for pushing this through @oli-obk and @Zoxc!

@alexcrichton alexcrichton merged commit 6e5d422 into rust-lang:master Dec 10, 2018
@alexcrichton alexcrichton mentioned this pull request Dec 10, 2018
@Zoxc
Copy link
Contributor

Zoxc commented Dec 10, 2018

This did fail on my Fedora VM last time I tried. That means that there's probably something wrong in the unix code :/

@alexcrichton
Copy link
Member

Ah ok, if you can get a reproduction we can dig in!

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.

4 participants