rustc_llvm: remove stale references #46280
Conversation
| // generates an llvmdeps.rs file next to this one which will be | ||
| // automatically updated whenever LLVM is updated to include an up-to-date | ||
| // set of the libraries we need to link to LLVM for. | ||
| #[link(name = "rustllvm", kind = "static")] // not quite true but good enough |
There was a problem hiding this comment.
This line shouldn't be removed as I believe it's required for msvc
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Can you help me understand how hoedown, profiler-rt, binaryen_wrapper, and others work? AFAICT, none of them have #[link(...)] attributes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks! I've included your explanation in the code.
| // figure out the exact set of libraries. To do this, the build system | ||
| // generates an llvmdeps.rs file next to this one which will be | ||
| // automatically updated whenever LLVM is updated to include an up-to-date | ||
| // set of the libraries we need to link to LLVM for. |
There was a problem hiding this comment.
Isn't this comment still largely applicable with some editing?
There was a problem hiding this comment.
Sort of, but there's no longer anything here to anchor it to, and the relevant code (in build.rs) is rather self explanatory.
There was a problem hiding this comment.
Ok yeah on rereading let's leave out
kennytm
added
the
S-waiting-on-review
...that were removed in 77c3bfa.
The documentation states: "The name output should be the name of the library." and this is already done in more recently-added callers.
tamird
force-pushed
the
remove-old-refs
branch
from
12dc132
to
94d02b8
Compare
|
@bors: r+ |
|
📌 Commit 94d02b8 has been approved by |
|
⌛ Testing commit 94d02b8 with merge 24cbcaedb1f02525771187f5d944f23daa365e56... |
|
💔 Test failed - status-travis |
|
Something to do with travis using an older xcode? Doesn't seem related to this PR. |
|
@bors retry — travis-ci/travis-ci#8821 |
kennytm
added
S-waiting-on-bors
rustc_llvm: remove stale references ...that were removed in 77c3bfa. r? @alexcrichton
rustc_llvm: remove stale references ...that were removed in 77c3bfa. r? @alexcrichton
rustc_llvm: remove stale references ...that were removed in 77c3bfa. r? @alexcrichton
rustc_llvm: remove stale references ...that were removed in 77c3bfa. r? @alexcrichton
rustc_llvm: remove stale references ...that were removed in 77c3bfa. r? @alexcrichton
|
@bors retry |
...that were removed in 77c3bfa.
r? @alexcrichton