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

check_static_recursion: Handle foreign items #18279

Merged
merged 1 commit into from
Oct 30, 2014

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Oct 24, 2014

I just found this patch which at some point solved a problem I encountered. Unfortunately I apparently dropped it before I managed to write a test case. I'll try to dig up the code that triggered the issue.

@alexcrichton
Copy link
Member

Thanks! Could you add a test for this as well?

@alexcrichton
Copy link
Member

Also, I think I saw an issue about this awhile ago, is this in relation to an open issue? If so, could you put Closes #XXXX in the message?

@bgamari
Copy link
Contributor Author

bgamari commented Oct 24, 2014

I just updated the text to describe the situation. I'm still trying to remember where I encountered this bug.

@bkoropoff
Copy link
Contributor

This will fix #18360

@alexcrichton
Copy link
Member

Thanks for tracking that down @bkoropoff! Could you add that test case @bgamari?

@bgamari
Copy link
Contributor Author

bgamari commented Oct 28, 2014

@bkoropoff good catch. @alexcrichton sure, it looks like that ticket should have enough to compose a test.

@ghost
Copy link

ghost commented Oct 28, 2014

@bgamari There's a competing patch for this issue in #17973. :-) I'm not sure what the best resolution is but maybe you and @whataloadofwhat can sync on IRC?

@bgamari bgamari force-pushed the check-static-recursion branch 2 times, most recently from 0000a4f to 2a3be29 Compare October 28, 2014 19:43
@bgamari
Copy link
Contributor Author

bgamari commented Oct 28, 2014

@alexcrichton there is now a test case.

@jakub- thanks for pointing this out. @alexcrichton which would you like to merge?

@ghost
Copy link

ghost commented Oct 28, 2014

Unfortunately the build-bots seemed to have trouble with my test case (seems to be the same as your own) and I'm not sure how to deal with it to be honest.

Here's the error from here:

    error: linking with `gcc` failed: exit code: 1
    note: gcc '-m64' '-L' 'C:\bot\slave\auto-win-64-nopt-t\build\obj\x86_64-w64-mingw32\stage2\bin\rustlib\x86_64-w64-mingw32\lib' '-o' 'x86_64-w64-mingw32\test\run-pass\static-reference-foreign-static.stage2-x86_64-w64-mingw32.exe' 'x86_64-w64-mingw32\test\run-pass\static-reference-foreign-static.o' '-Wl,--whole-archive' '-lmorestack' '-Wl,--no-whole-archive' '-fno-lto' '-fno-use-linker-plugin' '-Wl,--gc-sections' '-static-libgcc' '-Wl,--enable-long-section-names' '-Wl,--nxcompat' '-L' 'C:\bot\slave\auto-win-64-nopt-t\build\obj\x86_64-w64-mingw32\stage2\bin\rustlib\x86_64-w64-mingw32\lib' '-lnative-4e7c5e5c' '-L' 'C:\bot\slave\auto-win-64-nopt-t\build\obj\x86_64-w64-mingw32\stage2\bin\rustlib\x86_64-w64-mingw32\lib' '-lstd-4e7c5e5c' '-L' 'C:\bot\slave\auto-win-64-nopt-t\build\obj\x86_64-w64-mingw32\stage2\bin\rustlib\x86_64-w64-mingw32\lib' '-lsync-4e7c5e5c' '-L' 'C:\bot\slave\auto-win-64-nopt-t\build\obj\x86_64-w64-mingw32\stage2\bin\rustlib\x86_64-w64-mingw32\lib' '-lrustrt-4e7c5e5c' '-L' 'x86_64-w64-mingw32\test\run-pass' '-L' 'x86_64-w64-mingw32\test\run-pass\static-reference-foreign-static.stage2-x86_64-w64-mingw32libaux' '-L' 'x86_64-w64-mingw32\rt' '-L' 'C:\bot\slave\auto-win-64-nopt-t\build\obj\.rust' '-L' 'C:\bot\slave\auto-win-64-nopt-t\build\obj' '-Wl,--whole-archive' '-Wl,-Bstatic' '-Wl,--no-whole-archive' '-Wl,-Bdynamic' '-lws2_32' '-lcompiler-rt'
    note: x86_64-w64-mingw32\test\run-pass\static-reference-foreign-static.o:(.rdata+0x0): undefined reference to `x'
    collect2.exe: error: ld returned 1 exit status

    error: aborting due to previous error

It seems to try to link with the non-existant static for some reason. I've tried thinking of solutions but I just can't think of anything, I'm sorry.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 28, 2014

@whataloadofwhat very odd.

@alexcrichton unless you see any further issues perhaps we should just merge my branch?

@alexcrichton
Copy link
Member

I think that this is the same test as #17973 (which will probably fail for the same reason). You'll need to provide an actual definition of the symbol somehow to get it to link on windows I think. You should be able to get by with creating an auxiliary crate with a #[no_mangle] symbol at the top level named a

@alexcrichton
Copy link
Member

@whataloadofwhat, are you ok closing in favor of this for now?

@bgamari
Copy link
Contributor Author

bgamari commented Oct 29, 2014

@alexcrichton how does this look? It passes on my box but unfortunately I have no Windows machine to test on.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 29, 2014

@alexcrichton alright, let's try this one then.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 29, 2014

@alexcrichton could you give this one a try; I made the static definition pub.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 30, 2014

@alexcrichton I'm not sure why this fail! failure doesn't occur on my machine but this should probably be a span_err anyways. I've pushed another new branch fixing this.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 30, 2014

Note to self: fail! is now panic!

@alexcrichton
Copy link
Member

Ah yes it seems like you found it, be sure to blame @steveklabnik for any troubles you're having :)

bors added a commit that referenced this pull request Oct 30, 2014
…chton

I just found this patch which at some point solved a problem I encountered. Unfortunately I apparently dropped it before I managed to write a test case. I'll try to dig up the code that triggered the issue.
@bors bors closed this Oct 30, 2014
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