Skip to content

rustc_llvm: remove stale references#46280

Merged
bors merged 3 commits intorust-lang:masterfrom
tamird:remove-old-refs
Dec 2, 2017
Merged

rustc_llvm: remove stale references#46280
bors merged 3 commits intorust-lang:masterfrom
tamird:remove-old-refs

Conversation

@tamird
Copy link
Contributor

@tamird tamird commented Nov 26, 2017

...that were removed in 77c3bfa.

r? @alexcrichton

alexcrichton
alexcrichton reviewed Nov 27, 2017
View reviewed changes
src/librustc_llvm/ffi.rs Outdated
Copy link
Member

@alexcrichton alexcrichton Nov 27, 2017

Choose a reason for hiding this comment

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

This line shouldn't be removed as I believe it's required for msvc

Copy link
Contributor Author

@tamird tamird Nov 27, 2017

Choose a reason for hiding this comment

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

Are you sure? cc::Build::compile prints to stdout a cargo directive that makes the link work, which I think is how this works. https://docs.rs/cc/1.0.3/src/cc/lib.rs.html#781

Copy link
Member

@alexcrichton alexcrichton Nov 27, 2017

Choose a reason for hiding this comment

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

Yes, I am quite sure.

Copy link
Contributor Author

@tamird tamird Nov 27, 2017

Choose a reason for hiding this comment

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

Can you help me understand how hoedown, profiler-rt, binaryen_wrapper, and others work? AFAICT, none of them have #[link(...)] attributes.

Copy link
Member

@alexcrichton alexcrichton Nov 28, 2017

Choose a reason for hiding this comment

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

Sure yes. This annotation is primarily needed for MSVC where attributes like dllimport/dllexport are applied and need to be correct for everything to link successfully. The #[link] annotation here says "these symbols are included statically" which means that they're all exported with dllexport and from the rustc_llvm dynamic library. Otherwise the rustc_trans dynamic library would not be able to access these symbols.

The other libraries we have just happen to not run into this situation where they cross dynamic library boundaries.

Copy link
Contributor Author

@tamird tamird Nov 28, 2017

Choose a reason for hiding this comment

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

Thanks! I've included your explanation in the code.

src/librustc_llvm/ffi.rs Outdated
Copy link
Member

@alexcrichton alexcrichton Nov 27, 2017

Choose a reason for hiding this comment

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

Isn't this comment still largely applicable with some editing?

Copy link
Contributor Author

@tamird tamird Nov 27, 2017

Choose a reason for hiding this comment

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

Sort of, but there's no longer anything here to anchor it to, and the relevant code (in build.rs) is rather self explanatory.

Copy link
Member

@alexcrichton alexcrichton Nov 27, 2017

Choose a reason for hiding this comment

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

Ok yeah on rereading let's leave out

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2017
@tamird
rustc_llvm: remove stale references
658ea38 
...that were removed in 77c3bfa.
@tamird
rustc_llvm: use cc::Build::define
9067d97
@tamird
*: strip calls to cc::Build::compile
94d02b8 
The documentation states: "The name output should be the name of the
library." and this is already done in more recently-added callers.
@alexcrichton
Copy link
Member

alexcrichton commented Nov 29, 2017

@bors: r+

@bors
Copy link
Collaborator

bors commented Nov 29, 2017

📌 Commit 94d02b8 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 29, 2017

⌛ Testing commit 94d02b8 with merge 24cbcaedb1f02525771187f5d944f23daa365e56...

@bors
Copy link
Collaborator

bors commented Nov 29, 2017

💔 Test failed - status-travis

@tamird
Copy link
Contributor Author

tamird commented Nov 29, 2017

Something to do with travis using an older xcode?

[00:13:17] In file included from /Users/travis/build/rust-lang/rust/src/llvm/include/llvm/Support/ThreadPool.h:30:
[00:13:17] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:1447:23: error: 'future_error' is unavailable: introduced in macOS 10.8
[00:13:17]                       future_error(make_error_code(future_errc::broken_promise))
[00:13:17]                       ^
[00:13:17] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:502:63: note: 'future_error' has been explicitly marked unavailable here
[00:13:17] class _LIBCPP_EXCEPTION_ABI _LIBCPP_AVAILABILITY_FUTURE_ERROR future_error
[00:13:17]                                                               ^
[00:13:17] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:1621:23: error: 'future_error' is unavailable: introduced in macOS 10.8
[00:13:17]                       future_error(make_error_code(future_errc::broken_promise))
[00:13:17]                       ^
[00:13:17] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:502:63: note: 'future_error' has been explicitly marked unavailable here
[00:13:17] class _LIBCPP_EXCEPTION_ABI _LIBCPP_AVAILABILITY_FUTURE_ERROR future_error
[00:13:17]                                                               ^
[00:13:17] 2 errors generated.

Doesn't seem related to this PR.

@scottmcm scottmcm mentioned this pull request Nov 29, 2017
Closed
@kennytm
Copy link
Member

kennytm commented Nov 29, 2017

@bors retry — travis-ci/travis-ci#8821

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2017
arielb1 pushed a commit to arielb1/rust that referenced this pull request Nov 30, 2017
Rollup merge of rust-lang#46280 - tamird:remove-old-refs, r=alexcrichton
b54f43a 
rustc_llvm: remove stale references

...that were removed in 77c3bfa.

r? @alexcrichton
@arielb1 arielb1 mentioned this pull request Nov 30, 2017
Closed
@arielb1 arielb1 mentioned this pull request Nov 30, 2017
Closed
@arielb1 arielb1 mentioned this pull request Nov 30, 2017
Closed
arielb1 pushed a commit to arielb1/rust that referenced this pull request Nov 30, 2017
Rollup merge of rust-lang#46280 - tamird:remove-old-refs, r=alexcrichton
9d974ef 
rustc_llvm: remove stale references

...that were removed in 77c3bfa.

r? @alexcrichton
arielb1 pushed a commit to arielb1/rust that referenced this pull request Nov 30, 2017
Rollup merge of rust-lang#46280 - tamird:remove-old-refs, r=alexcrichton
2b52ae0 
rustc_llvm: remove stale references

...that were removed in 77c3bfa.

r? @alexcrichton
@arielb1 arielb1 mentioned this pull request Nov 30, 2017
Closed
arielb1 pushed a commit to arielb1/rust that referenced this pull request Nov 30, 2017
Rollup merge of rust-lang#46280 - tamird:remove-old-refs, r=alexcrichton
80c0824 
rustc_llvm: remove stale references

...that were removed in 77c3bfa.

r? @alexcrichton
@arielb1 arielb1 mentioned this pull request Nov 30, 2017
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Dec 1, 2017
@kennytm
Rollup merge of rust-lang#46280 - tamird:remove-old-refs, r=alexcrichton
263eb4d 
rustc_llvm: remove stale references

...that were removed in 77c3bfa.

r? @alexcrichton
@kennytm kennytm mentioned this pull request Dec 1, 2017
Merged
@shepmaster
Copy link
Member

shepmaster commented Dec 1, 2017

@bors retry

bors added a commit that referenced this pull request Dec 1, 2017
@bors
Auto merge of #46430 - kennytm:rollup, r=kennytm
7051754 
Rollup of 13 pull requests

- Successful merges: #45880, #46280, #46373, #46376, #46385, #46386, #46387, #46392, #46400, #46401, #46405, #46412, #46421
- Failed merges:
@bors bors merged commit 94d02b8 into rust-lang:master Dec 2, 2017
@tamird tamird deleted the remove-old-refs branch December 2, 2017 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@alexcrichton alexcrichton

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tamird @alexcrichton @bors @kennytm @shepmaster