Skip to content
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.

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

Bootstrap assumes llvm-dwp is sibling of llvm-config instead of inside bindir #81949

Closed
dtolnay opened this issue Feb 10, 2021 · 2 comments · Fixed by #81955
Closed

Bootstrap assumes llvm-dwp is sibling of llvm-config instead of inside bindir #81949

dtolnay opened this issue Feb 10, 2021 · 2 comments · Fixed by #81955
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@dtolnay
Copy link
Member

dtolnay commented Feb 10, 2021

The incorrect way: assuming llvm-dwp exists in the parent directory of the path provided for llvm-config. In general these two paths cannot be assumed to have any relationship to one another.

rust/src/bootstrap/compile.rs

Lines 1075 to 1079 in 87bacf2

let src_exe = exe("llvm-dwp", target_compiler.host);
let dst_exe = exe("rust-llvm-dwp", target_compiler.host);
let llvm_config_bin = builder.ensure(native::Llvm { target: target_compiler.host });
let llvm_bin_dir = llvm_config_bin.parent().unwrap();
builder.copy(&llvm_bin_dir.join(&src_exe), &libdir_bin.join(&dst_exe));


The correct way: run llvm-config --bindir using the provided llvm-config, and pick llvm-dwp inside of the directory that it returns. Bootstrap does this correctly elsewhere, such as for FileCheck:

rust/src/bootstrap/lib.rs

Lines 669 to 671 in 87bacf2

} else if let Some(s) = target_config.and_then(|c| c.llvm_config.as_ref()) {
let llvm_bindir = output(Command::new(s).arg("--bindir"));
let filecheck = Path::new(llvm_bindir.trim()).join(exe("FileCheck", target));

@dtolnay dtolnay added the C-bug Category: This is a bug. label Feb 10, 2021
@Mark-Simulacrum
Copy link
Member

Good catch! I wish we had tools to lint for this kind of thing in bootstrap, or at least some kind of testing; we consistently get this wrong quite often unfortunately.

I'd be happy to see a PR to fix this. Otherwise it'll likely take some time for me to get to it.

@Mark-Simulacrum Mark-Simulacrum added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Feb 10, 2021
@cuviper
Copy link
Member

cuviper commented Feb 10, 2021

I think this is also something that dist should ignore when building with external LLVM, although I suspect rust's support for split DWARF won't handle that well as-is.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 11, 2021
bootstrap: Locate llvm-dwp based on llvm-config bindir

Fixes rust-lang#81949.

Tested by successfully building 1.50.0 pre-release, which is where I originally hit the issue (https://internals.rust-lang.org/t/rust-1-50-0-pre-release-testing/14012/4?u=dtolnay). Tested both with and without prebuilt LLVM. The check for dry_run is necessary in the non-prebuilt case because the llvm-config built by bootstrap won't exist yet.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 11, 2021
bootstrap: Locate llvm-dwp based on llvm-config bindir

Fixes rust-lang#81949.

Tested by successfully building 1.50.0 pre-release, which is where I originally hit the issue (https://internals.rust-lang.org/t/rust-1-50-0-pre-release-testing/14012/4?u=dtolnay). Tested both with and without prebuilt LLVM. The check for dry_run is necessary in the non-prebuilt case because the llvm-config built by bootstrap won't exist yet.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 11, 2021
bootstrap: Locate llvm-dwp based on llvm-config bindir

Fixes rust-lang#81949.

Tested by successfully building 1.50.0 pre-release, which is where I originally hit the issue (https://internals.rust-lang.org/t/rust-1-50-0-pre-release-testing/14012/4?u=dtolnay). Tested both with and without prebuilt LLVM. The check for dry_run is necessary in the non-prebuilt case because the llvm-config built by bootstrap won't exist yet.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 12, 2021
bootstrap: Locate llvm-dwp based on llvm-config bindir

Fixes rust-lang#81949.

Tested by successfully building 1.50.0 pre-release, which is where I originally hit the issue (https://internals.rust-lang.org/t/rust-1-50-0-pre-release-testing/14012/4?u=dtolnay). Tested both with and without prebuilt LLVM. The check for dry_run is necessary in the non-prebuilt case because the llvm-config built by bootstrap won't exist yet.
@bors bors closed this as completed in 67403da Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants