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

Update compiler-rt for Windows fixes #29478

Merged
merged 2 commits into from
Nov 4, 2015
Merged

Update compiler-rt for Windows fixes #29478

merged 2 commits into from
Nov 4, 2015

Conversation

angelsl
Copy link
Contributor

@angelsl angelsl commented Oct 30, 2015

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@angelsl
Copy link
Contributor Author

angelsl commented Oct 30, 2015

Pull request on compiler-rt: rust-lang/compiler-rt#14

@angelsl
Copy link
Contributor Author

angelsl commented Oct 30, 2015

Don't merge yet, doesn't fix Windows MSVC

@alexcrichton
Copy link
Member

Thanks for the quick update! I'd like to make sure we add regression tests if possible, however, to ensure that this doesn't happen again.

For the MinGW case I discovered that it's related to linking in C++ code, so could you also add a run-make? I don't think that our C++ compiler detection is the best, so it's fine for the test just to be enabled on MinGW for now and we can enable it elsewhere later. It just needs to be something like:

// foo.cpp
extern "C" void foo() {
    int *a = new int(3);
    delete a;
}
extern { fn foo(); }
fn main() {
    unsafe { foo(); }
}
$ g++ foo.cpp -c
$ ar crus libfoo.a foo.o
$ rustc foo.rs -L. -lfoo -lstdc++
$ ./foo.exe

@alexcrichton
Copy link
Member

OK, after further investigation about libcmt/msvcrt, it looks like this was just working by luck beforehand. A few of the objects compiled in that test were not compiled with /MD (which I believe msvcrt.lib corresponds to), and then when compiler-rt.lib was also imposing some restrictions on the CRT in use it ended up causing problems.

This brings up the interesting question, however, of how these files should be compiled. It looks like the MSVC build of compiler-rt may not be quite ready just yet? I would expect it to compile to a library that actually has 0 dependencies, but it looks like it even depends on symbols like VirtualProtect and other CRT functions like fflush perhaps.

I'm not entirely sure what compiler-rt is doing, but it seems like we're pulling in more than we may want.

@bors
Copy link
Contributor

bors commented Oct 31, 2015

☔ The latest upstream changes (presumably #29477) made this pull request unmergeable. Please resolve the merge conflicts.

@angelsl
Copy link
Contributor Author

angelsl commented Nov 3, 2015

Updated. @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+ 9bb2da3033357a9ab248040eff07a624e5ce75a6

@bors
Copy link
Contributor

bors commented Nov 3, 2015

⌛ Testing commit 9bb2da3 with merge 33424a8...

@bors
Copy link
Contributor

bors commented Nov 4, 2015

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

@angelsl
Copy link
Contributor Author

angelsl commented Nov 4, 2015

Fixed for proper MinGW-only detection. @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+ 9fe4e96

bors added a commit that referenced this pull request Nov 4, 2015
@bors bors merged commit 9fe4e96 into rust-lang:master Nov 4, 2015
@vadimcn
Copy link
Contributor

vadimcn commented Nov 5, 2015

Does anybody know, what happened to @vhbit's SjLj fix introduced in rust-lang/compiler-rt@df1e1ca? I don't see any of that stuff in the current compiler-rt branch. Was this change deemed unnecessary?

@angelsl
Copy link
Contributor Author

angelsl commented Nov 5, 2015

@vadimcn It seems like LLVM upstream incorporated something like @vhbit's changes, so I didn't port that change over.

Not sure if it works though, I have no way to test.

@vhbit
Copy link
Contributor

vhbit commented Nov 5, 2015

@vadimcn thanks for bringing this up!
@angelsl

LLVM upstream incorporated something like

nope. As I can see from link you've provided LLVM has just different names of function depending on conditional, while my code has also different handling which is crucial. I'd be happy if those changes are kept otherwise iOS support will be broken.

@angelsl
Copy link
Contributor Author

angelsl commented Nov 5, 2015

@vhbit OK, I cherry-picked your changes. Can you help me see if it looks correct?

Are there any other fixes I should port? ping: @vadimcn as well

@vadimcn
Copy link
Contributor

vadimcn commented Nov 5, 2015

@angelsl: Can you please rebase on top of the current master? I wanted to pick up the recently added _alloca functions.

@vhbit: If these changes are not Rust-specific, we should upstream them to prevent this kind of thing in the future.
Would be best if you did it yourself, so there'd be no questions about attribution.
But in any case, could you please add some comments? E.g., what's different between this and the DWARF personality routine? Is this code SjLj-specific or ARM-specific?
Thanks!

@alexcrichton alexcrichton mentioned this pull request Nov 6, 2015
@vhbit
Copy link
Contributor

vhbit commented Nov 6, 2015

@angelsl yep, looks correct

@vadimcn this code is specific to SjLj. I've actually tried to upstream it a while ago but got no response. I'll try to do it again soon or delegate it to someone if will not be able to allocate time.

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.

6 participants