Skip to content

rustbuild changes to build LLVM #32599

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

rustbuild changes to build LLVM #32599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/bootstrap/build/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use std::io::prelude::*;
use std::path::PathBuf;
use std::process;

use build::util::build_path;
use num_cpus;
use rustc_serialize::Decodable;
use toml::{Parser, Decoder, Value};
Expand Down Expand Up @@ -305,7 +306,7 @@ impl Config {
.collect();
}
"CFG_MUSL_ROOT" if value.len() > 0 => {
self.musl_root = Some(PathBuf::from(value));
self.musl_root = Some(build_path(value));
}
"CFG_DEFAULT_AR" if value.len() > 0 => {
self.rustc_default_ar = Some(value.to_string());
Expand All @@ -322,31 +323,31 @@ impl Config {
"CFG_LLVM_ROOT" if value.len() > 0 => {
let target = self.target_config.entry(self.build.clone())
.or_insert(Target::default());
let root = PathBuf::from(value);
let root = build_path(value);
target.llvm_config = Some(root.join("bin/llvm-config"));
}
"CFG_JEMALLOC_ROOT" if value.len() > 0 => {
let target = self.target_config.entry(self.build.clone())
.or_insert(Target::default());
target.jemalloc = Some(PathBuf::from(value));
target.jemalloc = Some(build_path(value));
}
"CFG_ARM_LINUX_ANDROIDEABI_NDK" if value.len() > 0 => {
let target = "arm-linux-androideabi".to_string();
let target = self.target_config.entry(target)
.or_insert(Target::default());
target.ndk = Some(PathBuf::from(value));
target.ndk = Some(build_path(value));
}
"CFG_I686_LINUX_ANDROID_NDK" if value.len() > 0 => {
let target = "i686-linux-androideabi".to_string();
let target = self.target_config.entry(target)
.or_insert(Target::default());
target.ndk = Some(PathBuf::from(value));
target.ndk = Some(build_path(value));
}
"CFG_AARCH64_LINUX_ANDROID_NDK" if value.len() > 0 => {
let target = "aarch64-linux-androideabi".to_string();
let target = self.target_config.entry(target)
.or_insert(Target::default());
target.ndk = Some(PathBuf::from(value));
target.ndk = Some(build_path(value));
}
_ => {}
}
Expand Down
13 changes: 9 additions & 4 deletions src/bootstrap/build/native.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,15 +146,20 @@ pub fn compiler_rt(build: &Build, target: &str) {
}
let _ = fs::remove_dir_all(&dst);
t!(fs::create_dir_all(&dst));
let build_llvm_config = build.llvm_out(&build.config.build)
.join("bin")
.join(exe("llvm-config", &build.config.build));

let internal_config = build.llvm_out(&build.config.build)
.join("bin")
.join("llvm-config");
let external_config = build.config.target_config.get(target).and_then(|config| {
config.llvm_config.clone()
});
let llvm_config = external_config.unwrap_or(internal_config);
let mut cfg = cmake::Config::new(build.src.join("src/compiler-rt"));
cfg.target(target)
.host(&build.config.build)
.out_dir(&dst)
.profile(mode)
.define("LLVM_CONFIG_PATH", build_llvm_config)
.define("LLVM_CONFIG_PATH", llvm_config)
.define("COMPILER_RT_DEFAULT_TARGET_TRIPLE", target)
.define("COMPILER_RT_BUILD_SANITIZERS", "OFF")
.define("COMPILER_RT_BUILD_EMUTLS", "OFF")
Expand Down
19 changes: 19 additions & 0 deletions src/bootstrap/build/sanity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::collections::HashSet;
use std::env;
use std::ffi::{OsStr, OsString};
use std::fs;
use std::path::PathBuf;
use std::process::Command;

use build_helper::output;
Expand Down Expand Up @@ -78,6 +79,24 @@ pub fn check(build: &mut Build) {
panic!("the iOS target is only supported on OSX");
}

if cfg!(windows) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment to this explaining that this is just ensuring that we can run binaries like llvm-config and the compiler by ensuring that the dynamically linked libraries are in PATH?

Copy link
Member

Choose a reason for hiding this comment

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

This also seems relevant for all platforms (not just Windows), so perhaps this could create a variable like ldpath to handle the Unix/OSX cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't needed on Linux atleast, since it links using full paths and not just the filename,

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this needed for running llvm-config at all? Or running the compiler if it's dynamically linked? I may be misunderstanding the purpose for this though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not need for llvm-config on Windows, since it's in the directory with the DLLs. It is needed for executables which are not in the LLVM's binary directory which link dynamically to LLVM (like rustc).

Copy link
Member

Choose a reason for hiding this comment

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

Right, but that's not a Windows specific limitation, right? If rustc ever dynamically links to LLVM (on any platform) we need the library directory to be in the dynamic library path?

It looks like the difference is just that the dynamic library directory on Windows happens to be the same as the binaries directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is Windows specific, as you link with filenames on Windows and not paths.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I think I'm not being clear enough. So if you're liking with an external LLVM that is itself dynamically linked, then the only hope we have of working correctly is to ensure that the directory with that LLVM dynamic library is in the runtime linker lookup path. On Windows this just happens to be PATH but on Linux/OSX this id LD_LIBRARY_PATH and DYLD_LIBRARY_PATH.

Which is to say, if we link to a dynamically linked LLVM on any platform, we need to run this logic to ensure that the relevant directory is in the relevant environment variable.

if let Some(config) = build.config.target_config.get(target) {
if let Some(ref llvm_config) = config.llvm_config {
let llvm_mode = output(Command::new(&llvm_config).arg("--shared-mode"));
if llvm_mode == "shared" {
let bin = output(Command::new(&llvm_config).arg("--bindir"));
let bin_canonical = PathBuf::from(bin.trim()).canonicalize().unwrap();
let bin_in_path = env::split_paths(&path).find(|p| {
p.canonicalize().ok().map_or(false, |c| c.eq(&bin_canonical))
});
if bin_in_path.is_none() {
panic!("Unable to find LLVM's binary folder {:?} in PATH", bin);
}
}
}
}
}

// Make sure musl-root is valid if specified
if target.contains("musl") && (target.contains("x86_64") || target.contains("i686")) {
match build.config.musl_root {
Expand Down
8 changes: 6 additions & 2 deletions src/bootstrap/build/step.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ fn top_level(build: &Build) -> Vec<Step> {
src: Source::Llvm { _dummy: () },
target: &build.config.build,
};
targets.push(t.doc(stage));
if build.config.docs {
targets.push(t.doc(stage));
}
for host in build.config.host.iter() {
if !build.flags.host.contains(host) {
continue
Expand Down Expand Up @@ -320,7 +322,9 @@ impl<'a> Step<'a> {

Source::Dist { stage } => {
let mut base = Vec::new();
base.push(self.dist_docs(stage));
if build.config.docs {
base.push(self.dist_docs(stage));
}

for host in build.config.host.iter() {
let host = self.target(host);
Expand Down
23 changes: 23 additions & 0 deletions src/bootstrap/build/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,29 @@ use std::process::Command;
use bootstrap::{dylib_path, dylib_path_var};
use filetime::FileTime;

#[cfg(not(windows))]
pub fn build_path(path: &str) -> PathBuf {
PathBuf::from(path)
}

#[cfg(windows)]
pub fn build_path(path: &str) -> PathBuf {
Copy link
Member

Choose a reason for hiding this comment

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

Hm this seems very suspicious about how this would be called, how are MSYS paths leaking into the configuration here? Are the makefiles encoding MSYS paths?

Also, this can probably be done in Rust like sanitize_sh rather than forking out to cygpath, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The configure script produces MSYS path, so we need to convert them when importing config.mk.

I'm not sure what the logic is to convert to Windows paths, so I'd rather avoid writing that.

Copy link
Member

Choose a reason for hiding this comment

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

The logic should be pretty simple, right? /a/foo translates to A:\foo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's only the Windows paths in msys form. There are also things like /home and even /h/path where /h as a directory and not a drive.

Copy link
Member

Choose a reason for hiding this comment

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

Right so this can cause problems, but this happens on every startup of the build system, so it would be best to spawn fewer processes and just catch up on edge cases over time. The vast majority of these paths will just be /c/foo or /other that we need to fix (which are all trivial to fix)

Copy link
Member

Choose a reason for hiding this comment

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

Or actually, even better, we should change the configure script to not emit MSYS paths, but rather use cygpath in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to touch that script though.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can stomach all of these process spawns on every invocation of the build system, so this needs to be handled in some method.

use std::io::{stderr, Write};
use build_helper::output;

if path.chars().next() == Some('/') {
let output = output(&mut Command::new("cygpath").arg("-w").arg(path));
let win_path = output.trim_right();
writeln!(&mut stderr(),
"note: Converted Unix path '{}' to Windows path '{}'",
path,
win_path).ok();
PathBuf::from(win_path)
} else {
PathBuf::from(path)
}
}

pub fn staticlib(name: &str, target: &str) -> String {
if target.contains("windows-msvc") {
format!("{}.lib", name)
Expand Down
16 changes: 12 additions & 4 deletions src/librustc_llvm/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ fn main() {
println!("cargo:rustc-cfg=cargobuild");

let target = env::var("TARGET").unwrap();
let llvm_config = env::var_os("LLVM_CONFIG").map(PathBuf::from)
.unwrap_or_else(|| {
let mut llvm_config = env::var_os("LLVM_CONFIG").map(PathBuf::from)
.unwrap_or_else(|| {
match env::var_os("CARGO_TARGET_DIR").map(PathBuf::from) {
Some(dir) => {
let to_test = dir.parent().unwrap().parent().unwrap()
Expand All @@ -35,7 +35,7 @@ fn main() {
}
PathBuf::from("llvm-config")
});

llvm_config.set_extension(env::consts::EXE_EXTENSION);
println!("cargo:rerun-if-changed={}", llvm_config.display());

// Test whether we're cross-compiling LLVM. This is a pretty rare case
Expand Down Expand Up @@ -111,6 +111,7 @@ fn main() {
// Link in all LLVM libraries, if we're uwring the "wrong" llvm-config then
// we don't pick up system libs because unfortunately they're for the host
// of llvm-config, not the target that we're attempting to link.
let llvm_static = output(Command::new(&llvm_config).arg("--shared-mode")) == "static";
let mut cmd = Command::new(&llvm_config);
cmd.arg("--libs");
if !is_crossed {
Expand All @@ -136,7 +137,7 @@ fn main() {
continue
}

let kind = if name.starts_with("LLVM") {"static"} else {"dylib"};
let kind = if name.starts_with("LLVM") && llvm_static {"static"} else {"dylib"};
println!("cargo:rustc-link-lib={}={}", kind, name);
}

Expand All @@ -161,6 +162,13 @@ fn main() {
}
}

if !llvm_static && cfg!(windows) {
// Use --bindir as the search path. DLLs will be placed here.
// llvm-config is bugged and doesn't doesn't indicate this on Windows
let bin = output(Command::new(&llvm_config).arg("--bindir"));
println!("cargo:rustc-link-search=native={}", bin.trim());
}

// C++ runtime library
if !target.contains("msvc") {
if let Some(s) = env::var_os("LLVM_STATIC_STDCPP") {
Expand Down