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

Stack unwinding for 32 bit Windows builds #8596

Closed
wants to merge 8 commits into from

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Aug 18, 2013

This resolves issue #908.

Notable changes:

  • On Windows, LLVM integrated assembler emits bad stack unwind tables when segmented stacks are enabled. However, unwind info directives in the assembly output are correct, so we generate assembly first and then run it through an external assembler, just like it is already done for Android builds.
  • Linker is invoked via "g++" command instead of "gcc": g++ passes the appropriate magic parameters to the linker, which ensure correct registration of stack unwind tables in dynamic libraries.

@brson
Copy link
Contributor

brson commented Aug 18, 2013

/me faints

Well, this is unexpected! I guess this is using dwarf unwinding then?

There are a lot more tests that should work now too - most everything that says #[ignore(cfg(target_os = "win32"))] or #[ignore(cfg(windows))].

👯

@alexcrichton
Copy link
Member

Does this pass make check locally? Because that would be awesome!

@emberian
Copy link
Member

I super can't wait to feature this on TWiR.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 19, 2013

@brson: yes, this is dwarf unwinding.
I didn't find any files containing #[ignore(cfg(target_os = "win32"))] or #[ignore(cfg(windows))] in src/test... Can you provide an example?

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 19, 2013

@alexcrichton: yes, make check passes

@vadimcn vadimcn closed this Aug 19, 2013
@vadimcn vadimcn reopened this Aug 19, 2013
@alexcrichton
Copy link
Member

The ignore tags are mostly in libstd/extra on the unit tests that are also marked should_fail

git grep 'ignore.*windows' should show up a few things.

@alexcrichton
Copy link
Member

Also, awesome!

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 20, 2013

Okay, I've enabled all unit tests I could find. They all seem to pass.

I am seeing, however, failures in unrelated tests:
f32::tests::test_abs_sub
f32::tests::test_frexp
f64::tests::test_abs_sub
f64::tests::test_frexp
float::tests::test_abs_sub
float::tests::test_frexp
os::tests::memory_map_file

They also seem to have been be failing in the build off of mozilla/rust/master. Not sure what's going on here...

@huonw
Copy link
Member

huonw commented Aug 20, 2013

What's the failure output? (Awesome work, btw!)

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 20, 2013

@huonw:

task <unnamed> failed at 'assertion failed: NaN.abs_sub(&-1f).is_NaN()', C:\rust\src\libstd\num\float.rs:1100
task <unnamed> failed at 'assertion failed: `(left == right) && (right == left)` (left: `NaN`, right: `inf`)', C:\rust\src\libstd\num\f32.rs:1224
task <unnamed> failed at 'assertion failed: NaN.abs_sub(&-1f64).is_NaN()', C:\rust\src\libstd\num\f64.rs:1163
task <unnamed> failed at 'assertion failed: `(left == right) && (right == left)` (left: `NaN`, right: `inf`)', C:\rust\src\libstd\num\f64.rs:1274
task <unnamed> failed at 'assertion failed: NaN.abs_sub(&-1f).is_NaN()', C:\rust\src\libstd\num\float.rs:1100
task <unnamed> failed at 'assertion failed: `(left == right) && (right == left)` (left: `NaN`, right: `inf`)', C:\rust\src\libstd\num\float.rs:1212
task <unnamed> failed at 'CreateFileMappingW failure=8', C:\rust\src\libstd\os.rs:2116

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 20, 2013

So... r?

@auroranockert
Copy link
Contributor

The frexp failures are a known difference between Windows / most unixes. (It is an issue with libc)

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 21, 2013

Looks like buildbots don't run unit tests on Windows, so the above must have been broken all along.

@brson, @alexcrichton: So what shall we do? Is this good to go in?

@alexcrichton
Copy link
Member

I'd defer to @brson on this one. It looks good to me, but I think that @bors should change his configuration after this lands. If we want to keep exercising this functionality we'd want to change the subset of test which were run.

Those failures from make check are a bit odd, though, and ideally they' wouldn't be failing. They should probably be either investigated, or marked with ignore(windows) and a corresponding FIXME.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 21, 2013

Tracked down the cause of test_abs_sub failure: #8663

@alexcrichton
Copy link
Member

Would it be possible to redefine that library function in rust for just this specific platform? It would be a shame to have different behavior across platforms because of library bugs. It also doesn't look like it'd be too bad to implement this for just this platform.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 21, 2013

Could be done. But probably not in this PR?
Also, I've heard that Rust was looking to switch to mingw-w64. That would make this issue moot.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 22, 2013

Here are the unit tests that failed in Linux build:
vec::tests::test_flat_map_fail
vec::tests::test_map_fail
vec::tests::test_permute_fail
vec::tests::test_rposition_fail

Curiously, in unmodified master branch build, these are showing in the log file as "ignored". I also notice that they were marked with #[ignore(windows)], instead of #[ignore(cfg(windows))]. My experiments show that #[ignore(windows)] makes a test be ignored on all platforms, not just on Windows, so I conclude that these were broken all along.

Also, looking at the source, I can't figure out how these tests could have ever passed. For example:

    #[test]
    #[should_fail]
    fn test_flat_map_fail() {
        let v = [(~0, @0), (~0, @0), (~0, @0), (~0, @0)];
        let mut i = 0;
        do flat_map(v) |_elt| {
            if i == 2 {
                fail!()
            }
            i += 0;
            ~[(~0, @0)]
        };
    }

How can (i == 2) be true?!

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 22, 2013

@brson: Looks like you added them in 24265b1. Can you please take a look?

@brson
Copy link
Contributor

brson commented Aug 23, 2013

@vadimcn Yeah, that's pretty funny, and I can take a look. Can you mark them #[ignore] for the time being and open an issue on the test bogosity?

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 23, 2013

Ok, done: #8698
r?

bors added a commit that referenced this pull request Aug 23, 2013
This resolves issue #908.  

Notable changes:
-  On Windows, LLVM integrated assembler emits bad stack unwind tables when segmented stacks are enabled.  However, unwind info directives in the assembly output are correct, so we generate assembly first and then run it through an external assembler, just like it is already done for Android builds.

- Linker is invoked  via "g++" command instead of "gcc": g++ passes the appropriate magic parameters to the linker, which ensure correct registration of stack unwind tables in dynamic libraries.
@bors bors closed this Aug 23, 2013
@alexcrichton
Copy link
Member

@graydon, @brson, does this mean that we can enable more tests to be run on the windows bots? I know that cycle time is a bit of a worry, but I think running the basic std/extra library tests on windows would be hugely benficial

@bstrie
Copy link
Contributor

bstrie commented Aug 26, 2013

@alexcrichton The windows test suite seems to already take significantly less time than the full test suite, tests should be gradually enabled until it actually starts becoming a bottleneck.

@graydon
Copy link
Contributor

graydon commented Aug 26, 2013

Windows runs make check-fast currently, which is a very different target.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 7, 2022
Fix unnecessary_cast suggestion for type aliasses

Fix rust-lang#6923. The [`unnecessary_cast`] lint now will skip casting to non-primitive type.

changelog: fix lint [`unnecessary_cast `]
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.

9 participants