Skip to content

rustc_llvm: remove stale references - #46280

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

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

Conversation

@tamird

@tamirdtamird commented Nov 26, 2017

Copy link
Copy Markdown
Contributor

...that were removed in 77c3bfa.

r? @alexcrichton

alexcrichton
alexcrichton reviewed Nov 27, 2017
View reviewed changes
Comment threadsrc/librustc_llvm/ffi.rs Outdated

@alexcrichtonalexcrichtonNov 27, 2017

Copy link
Copy Markdown
Member

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

@tamirdtamirdNov 27, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@alexcrichtonalexcrichtonNov 27, 2017

Copy link
Copy Markdown
Member

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.

@tamirdtamirdNov 27, 2017

Copy link
Copy Markdown
ContributorAuthor

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.

@alexcrichtonalexcrichtonNov 28, 2017

Copy link
Copy Markdown
Member

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.

@tamirdtamirdNov 28, 2017

Copy link
Copy Markdown
ContributorAuthor

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.

Comment threadsrc/librustc_llvm/ffi.rs Outdated

@alexcrichtonalexcrichtonNov 27, 2017

Copy link
Copy Markdown
Member

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?

@tamirdtamirdNov 27, 2017

Copy link
Copy Markdown
ContributorAuthor

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.

@alexcrichtonalexcrichtonNov 27, 2017

Copy link
Copy Markdown
Member

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

@kennytmkennytm 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

alexcrichton commented Nov 29, 2017

Copy link
Copy Markdown
Member

@bors: r+

@bors

bors commented Nov 29, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 94d02b8 has been approved by alexcrichton

@bors

bors commented Nov 29, 2017

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Nov 29, 2017

Copy link
Copy Markdown
Collaborator

💔 Test failed - status-travis

@tamird

tamird commented Nov 29, 2017

Copy link
Copy Markdown
ContributorAuthor

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.

@scottmcmscottmcm mentioned this pull request Nov 29, 2017
Closed
@kennytm

kennytm commented Nov 29, 2017

Copy link
Copy Markdown
Member

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

@kennytmkennytm 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
Ariel Ben-Yehuda
Rollup merge ofrust-lang#46280- tamird:remove-old-refs, r=alexcrichton
b54f43a
rustc_llvm: remove stale references
...that were removed in 77c3bfa.
r? @alexcrichton
@arielb1arielb1 mentioned this pull request Nov 30, 2017
Closed
@arielb1arielb1 mentioned this pull request Nov 30, 2017
Closed
@arielb1arielb1 mentioned this pull request Nov 30, 2017
Closed
arielb1 pushed a commit to arielb1/rust that referenced this pull request Nov 30, 2017
Ariel Ben-Yehuda
Rollup merge ofrust-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
Ariel Ben-Yehuda
Rollup merge ofrust-lang#46280- tamird:remove-old-refs, r=alexcrichton
2b52ae0
rustc_llvm: remove stale references
...that were removed in 77c3bfa.
r? @alexcrichton
@arielb1arielb1 mentioned this pull request Nov 30, 2017
Closed
arielb1 pushed a commit to arielb1/rust that referenced this pull request Nov 30, 2017
Ariel Ben-Yehuda
Rollup merge ofrust-lang#46280- tamird:remove-old-refs, r=alexcrichton
80c0824
rustc_llvm: remove stale references
...that were removed in 77c3bfa.
r? @alexcrichton
@arielb1arielb1 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 ofrust-lang#46280- tamird:remove-old-refs, r=alexcrichton
263eb4d
rustc_llvm: remove stale references
...that were removed in 77c3bfa.
r? @alexcrichton
@kennytmkennytm mentioned this pull request Dec 1, 2017
Merged
@shepmaster

shepmaster commented Dec 1, 2017

Copy link
Copy Markdown
Member

@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:masterDec 2, 2017
@tamird
tamird deleted the remove-old-refs branch December 2, 2017 00:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichtonalexcrichtonalexcrichton left review comments

Assignees

@alexcrichtonalexcrichton

Labels

S-waiting-on-borsStatus: 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