-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
rustc_llvm: remove stale references #46280
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -505,17 +505,13 @@ pub mod debuginfo { | |
|
||
pub enum ModuleBuffer {} | ||
|
||
// Link to our native llvm bindings (things that we need to use the C++ api | ||
// for) and because llvm is written in C++ we need to link against libstdc++ | ||
// | ||
// You'll probably notice that there is an omission of all LLVM libraries | ||
// from this location. This is because the set of LLVM libraries that we | ||
// link to is mostly defined by LLVM, and the `llvm-config` tool is used to | ||
// 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. | ||
#[link(name = "rustllvm", kind = "static")] // not quite true but good enough | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I am quite sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I've included your explanation in the code. |
||
// 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. | ||
#[link(name = "rustllvm", kind = "static")] | ||
extern "C" { | ||
// Create and destroy contexts. | ||
pub fn LLVMContextCreate() -> ContextRef; | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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