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: correct reading of flags for llvm #94466

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Mar 1, 2022

First, this reverts the CFLAGS/CXXFLAGS of #93918. Those flags are
already read by cc and populated into Build earlier on in the
process. We shouldn't be overriding that based on CFLAGS, since cc
also respects overrides like CFLAGS_{TARGET} and HOST_CFLAGS, which
we want to take into account.

Second, this adds the same capability to specify target-specific
versions of LDFLAGS as we have through cc for the C* flags:
https://github.com/alexcrichton/cc-rs#external-configuration-via-environment-variables

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 1, 2022
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 1, 2022

Hold on before merging this — I just realized there's actually another bug in the pre-existing logic that is what caused me to submit the (partially erroneous) fix is #93918. Specifically, we set both CFLAGS and CXXFLAGS based on builder.cflags:

let mut cxxflags: OsString = builder.cflags(target, GitRepo::Llvm).join(" ").into();

This is wrong — when computing the base arguments for CXXFLAGS, we need to use builder.cxx not builder.cc, which means this bit needs to be changed to take into account whether it's trying to grab CFLAGS or CXXFLAGS:

rust/src/bootstrap/lib.rs

Lines 944 to 952 in 8d6f527

fn cflags(&self, target: TargetSelection, which: GitRepo) -> Vec<String> {
// Filter out -O and /O (the optimization flags) that we picked up from
// cc-rs because the build scripts will determine that for themselves.
let mut base = self.cc[&target]
.args()
.iter()
.map(|s| s.to_string_lossy().into_owned())
.filter(|s| !s.starts_with("-O") && !s.starts_with("/O"))
.collect::<Vec<String>>();

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 1, 2022

I've updated the PR to handle CXXFLAGS separately from CFLAGS throughout src/bootstrap. This also required teaching compiletest about --cxxflags.

@rust-log-analyzer

This comment has been minimized.

@jonhoo jonhoo force-pushed the bootstrap-proper-env-flags branch from 48ca19d to 18a7573 Compare March 1, 2022 17:52
@rust-log-analyzer

This comment has been minimized.

@jonhoo jonhoo force-pushed the bootstrap-proper-env-flags branch from 18a7573 to 9c8cff9 Compare March 1, 2022 18:11
@Mark-Simulacrum
Copy link
Member

r=me with squashed changes

First, this reverts the `CFLAGS`/`CXXFLAGS` of rust-lang#93918. Those flags are
already read by `cc` and populated into `Build` earlier on in the
process. We shouldn't be overriding that based on `CFLAGS`, since `cc`
also respects overrides like `CFLAGS_{TARGET}` and `HOST_CFLAGS`, which
we want to take into account.

Second, this adds the same capability to specify target-specific
versions of `LDFLAGS` as we have through `cc` for the `C*` flags:
https://github.com/alexcrichton/cc-rs#external-configuration-via-environment-variables

Note that this also necessitated an update to compiletest to treat
CXXFLAGS separately from CFLAGS.
@jonhoo jonhoo force-pushed the bootstrap-proper-env-flags branch from 9c8cff9 to 9c05f0b Compare March 3, 2022 17:42
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 3, 2022

squashed and rebased onto master

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2022

📌 Commit 9c05f0b has been approved by Mark-Simulacrum

@bors bors 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 Mar 3, 2022
@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Mar 3, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 4, 2022
…r=Mark-Simulacrum

bootstrap: correct reading of flags for llvm

First, this reverts the `CFLAGS`/`CXXFLAGS` of rust-lang#93918. Those flags are
already read by `cc` and populated into `Build` earlier on in the
process. We shouldn't be overriding that based on `CFLAGS`, since `cc`
also respects overrides like `CFLAGS_{TARGET}` and `HOST_CFLAGS`, which
we want to take into account.

Second, this adds the same capability to specify target-specific
versions of `LDFLAGS` as we have through `cc` for the `C*` flags:
https://github.com/alexcrichton/cc-rs#external-configuration-via-environment-variables
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2022
Rollup of 10 pull requests

Successful merges:

 - rust-lang#88805 (Clarification of default socket flags)
 - rust-lang#93418 (rustdoc & doc: no `shortcut` for `rel="icon"`)
 - rust-lang#93913 (Remove the everybody loops pass)
 - rust-lang#93965 (Make regular stdio lock() return 'static handles)
 - rust-lang#94339 (ARM: Only allow using d16-d31 with asm! when supported by the target)
 - rust-lang#94404 (Make Ord and PartialOrd opt-out in `newtype_index`)
 - rust-lang#94466 (bootstrap: correct reading of flags for llvm)
 - rust-lang#94572 (Use `HandleOrNull` and `HandleOrInvalid` in the Windows FFI bindings.)
 - rust-lang#94575 (CTFE SwitchInt: update comment)
 - rust-lang#94582 (Fix a bug in `x.py fmt` that prevents some files being formatted.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 958bd02 into rust-lang:master Mar 4, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 4, 2022
@jonhoo jonhoo deleted the bootstrap-proper-env-flags branch March 4, 2022 17:44
@Mark-Simulacrum
Copy link
Member

See #94719 (comment) -- approving for beta backport.

@Mark-Simulacrum Mark-Simulacrum added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Mar 9, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.61.0, 1.60.0 Mar 14, 2022
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 14, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2022
…ulacrum

[beta] backports

* Update LLVM submodule rust-lang#94764
* Statically compile libstdc++ everywhere if asked rust-lang#94719
* Downgrade #[test] on macro call to warning rust-lang#94624
* Delay bug in expr adjustment when check_expr is called multiple times rust-lang#94596
* bootstrap: correct reading of flags for llvm rust-lang#94466
* Check method input expressions once rust-lang#94438
* remove feature gate in control_flow examples rust-lang#94283

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants