-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
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. |
Pull request on |
Don't merge yet, doesn't fix Windows MSVC |
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 // foo.cpp
extern "C" void foo() {
int *a = new int(3);
delete a;
} extern { fn foo(); }
fn main() {
unsafe { foo(); }
}
|
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 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 I'm not entirely sure what compiler-rt is doing, but it seems like we're pulling in more than we may want. |
☔ The latest upstream changes (presumably #29477) made this pull request unmergeable. Please resolve the merge conflicts. |
Updated. @alexcrichton |
@bors: r+ 9bb2da3033357a9ab248040eff07a624e5ce75a6 |
⌛ Testing commit 9bb2da3 with merge 33424a8... |
💔 Test failed - auto-win-msvc-64-opt |
Fixed for proper MinGW-only detection. @alexcrichton |
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? |
@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. |
@vadimcn thanks for bringing this up!
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. |
@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 |
@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. |
r? @alexcrichton