Auto merge of #13900 - gmorenz:target_applies_to_host_rustflags, r=we…

…ihanglo

Pass rustflags to artifacts built with implicit targets when using target-applies-to-host

## What this PR does

This fixes #10744, a long-standing bug with `target-applies-to-host=false` where `RUSTFLAGS` are not being passed to artifact-units when built with `cargo build` (as opposed to `cargo build --target <host>`).

It doesn't fix a similar problem with `linker` and `links`. If the architecture in this PR is accepted, I expect I will be able to fix `linker` and `links` in the same way in a subsequent PR.

Below is a hopefully useful table of what flags are passed when, with new behavior bolded (without these changes the flag is not passed). I've only changed values in the top right cell, I've included the whole table for completeness and to hopefully make clear what exactly this feature is doing (which took me awhile to understand).

The table was generated with a host of x86_64-unknown-linux-gnu. "Flag" refers to values in RUSTFLAGS. "Linker" refers to the value of `--config target.<host>.linker` . The table was built with [this repo](https://github.com/gmorenz/target_application_to_host_matrix) and then hand modify to bold changed values.

| | target_applies_to_host=true | target_applies_to_host=false |
|-|-|-|
| no --target flag | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker passed to bin<br/><br/>Flag passed to proc macro<br/>Flag passed to shared dep from proc macro<br/>Linker passed to proc macro<br/><br/>Built with 5 invocations<br/>Without rustflags, built with 5 invocations<br/> | **Flag passed to bin**<br/>**Flag passed to shared dep from bin**<br/>Linker not passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker not passed to proc macro<br/><br/>**Built with 6 invocations**<br/>Without rustflags, built with 5 invocations<br/> |
| --target x86_64-unknown-linux-gnu | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker not passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> |
| --target i686-unknown-linux-gnu | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker not passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> | Flag passed to bin<br/>Flag passed to shared dep from bin<br/>Linker not passed to bin<br/><br/>Flag not passed to proc macro<br/>Flag not passed to shared dep from proc macro<br/>Linker not passed to proc macro<br/><br/>Built with 6 invocations<br/>Without rustflags, built with 6 invocations<br/> |

 ## How it is implemented

There are two stages in the `UnitGraph`'s life. It gets built, with correct `CompileKind`: `CompileKind::Host` for host units, `CompileKind::Target(HostTriple)` for artifact units. Then it gets rebuilt with artifact units that are intended for the host  having their kind changed to `CompileKind::Host`.

This rebuilding step serves to de-duplicate host and artifact units. A step that we want to maintain where possible. Because de-duplicating host and artifact dependencies is only possible if their `rustflags` don't differ we must calculate `rustflags` for units before this step, and this step necessarily throws away the information necessary to calculate them.

The possible rustflags have already been determined before creating units. "Calculating rustflags for units" just means determining which set of rustflags go with a particular unit, and storing that somewhere. The obvious place to do that is in the unit itself, so that's what this PR does, which has the convenient side affect of also handing the de-duplication logic for us. If the rustflags are the same, two units can be merged, if they differ, they cannot.

In the above table that's why it takes 5 invocations to build the repo without rustflags, but 6 with them. The shared_dependency merges when it is built without rustflags, and not when it is built with rustflags.

 ## Related PRs, comments and issues

fixes #10744

#9453 - Tracking issue

#9753 - Stabilization PR

#9634 - I believe this was another attempt (going down another implementation route) to fix the same issue

Comments from Alex Crichton noting that this must be fixed [1](#10395 (comment)) [2](#9753 (comment))

[My own comment describing exactly how I ran into this](#9753 (comment))

[Documentation for the feature](https://doc.rust-lang.org/cargo/reference/unstable.html#target-applies-to-host)

src/cargo/core/compiler/build_context/mod.rs

@@ -134,32 +134,6 @@ impl<'a, 'gctx> BuildContext<'a, 'gctx> {
         self.build_config.jobs
     }
 
-    /// Extra compiler flags to pass to `rustc` for a given unit.
-    ///
-    /// Although it depends on the caller, in the current Cargo implementation,
-    /// these flags take precedence over those from [`BuildContext::extra_args_for`].
-    ///
-    /// As of now, these flags come from environment variables and configurations.
-    /// See [`TargetInfo.rustflags`] for more on how Cargo collects them.
-    ///
-    /// [`TargetInfo.rustflags`]: TargetInfo::rustflags
-    pub fn rustflags_args(&self, unit: &Unit) -> &[String] {
-        &self.target_data.info(unit.kind).rustflags
-    }
-
-    /// Extra compiler flags to pass to `rustdoc` for a given unit.
-    ///
-    /// Although it depends on the caller, in the current Cargo implementation,
-    /// these flags take precedence over those from [`BuildContext::extra_args_for`].
-    ///
-    /// As of now, these flags come from environment variables and configurations.
-    /// See [`TargetInfo.rustdocflags`] for more on how Cargo collects them.
-    ///
-    /// [`TargetInfo.rustdocflags`]: TargetInfo::rustdocflags
-    pub fn rustdocflags_args(&self, unit: &Unit) -> &[String] {
-        &self.target_data.info(unit.kind).rustdocflags
-    }
-
     /// Extra compiler args for either `rustc` or `rustdoc`.
     ///
     /// As of now, these flags come from the trailing args of either

src/cargo/core/compiler/build_context/target_info.rs

@@ -23,6 +23,7 @@ use std::cell::RefCell;
 use std::collections::hash_map::{Entry, HashMap};
 use std::path::{Path, PathBuf};
 use std::str::{self, FromStr};
+use std::sync::Arc;
 
 /// Information about the platform target gleaned from querying rustc.
 ///
@@ -52,9 +53,9 @@ pub struct TargetInfo {
     /// target libraries.
     pub sysroot_target_libdir: PathBuf,
     /// Extra flags to pass to `rustc`, see [`extra_args`].
-    pub rustflags: Vec<String>,
+    pub rustflags: Arc<[String]>,
     /// Extra flags to pass to `rustdoc`, see [`extra_args`].
-    pub rustdocflags: Vec<String>,
+    pub rustdocflags: Arc<[String]>,
     /// Whether or not rustc (stably) supports the `--check-cfg` flag.
     ///
     /// Can be removed once the minimum supported rustc version of Cargo is
@@ -312,15 +313,16 @@ impl TargetInfo {
                 crate_types: RefCell::new(map),
                 sysroot,
                 sysroot_target_libdir,
-                rustflags,
+                rustflags: rustflags.into(),
                 rustdocflags: extra_args(
                     gctx,
                     requested_kinds,
                     &rustc.host,
                     Some(&cfg),
                     kind,
                     Flags::Rustdoc,
-                )?,
+                )?
+                .into(),
                 cfg,
                 support_split_debuginfo,
                 support_check_cfg,
@@ -867,7 +869,10 @@ pub struct RustcTargetData<'gctx> {
 
     /// Build information for the "host", which is information about when
     /// `rustc` is invoked without a `--target` flag. This is used for
-    /// procedural macros, build scripts, etc.
+    /// selecting a linker, and applying link overrides.
+    ///
+    /// The configuration read into this depends on whether or not
+    /// `target-applies-to-host=true`.
     host_config: TargetConfig,
     /// Information about the host platform.
     host_info: TargetInfo,
@@ -889,7 +894,10 @@ impl<'gctx> RustcTargetData<'gctx> {
         let mut target_config = HashMap::new();
         let mut target_info = HashMap::new();
         let target_applies_to_host = gctx.target_applies_to_host()?;
+        let host_target = CompileTarget::new(&rustc.host)?;
         let host_info = TargetInfo::new(gctx, requested_kinds, &rustc, CompileKind::Host)?;
+
+        // This config is used for link overrides and choosing a linker.
         let host_config = if target_applies_to_host {
             gctx.target_cfg_triple(&rustc.host)?
         } else {
@@ -902,9 +910,21 @@ impl<'gctx> RustcTargetData<'gctx> {
         // needs access to the target config data, create a copy so that it
         // can be found. See `rebuild_unit_graph_shared` for why this is done.
         if requested_kinds.iter().any(CompileKind::is_host) {
-            let ct = CompileTarget::new(&rustc.host)?;
-            target_info.insert(ct, host_info.clone());
-            target_config.insert(ct, gctx.target_cfg_triple(&rustc.host)?);
+            target_config.insert(host_target, gctx.target_cfg_triple(&rustc.host)?);
+
+            // If target_applies_to_host is true, the host_info is the target info,
+            // otherwise we need to build target info for the target.
+            if target_applies_to_host {
+                target_info.insert(host_target, host_info.clone());
+            } else {
+                let host_target_info = TargetInfo::new(
+                    gctx,
+                    requested_kinds,
+                    &rustc,
+                    CompileKind::Target(host_target),
+                )?;
+                target_info.insert(host_target, host_target_info);
+            }
         };
 
         let mut res = RustcTargetData {

src/cargo/core/compiler/build_runner/mod.rs

@@ -269,7 +269,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
                         }
                     }
                 }
-                args.extend(self.bcx.rustdocflags_args(unit).iter().map(Into::into));
+                args.extend(unit.rustdocflags.iter().map(Into::into));
 
                 use super::MessageFormat;
                 let format = match self.bcx.build_config.message_format {

src/cargo/core/compiler/custom_build.rs

@@ -352,10 +352,7 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
             cmd.env("RUSTC_WORKSPACE_WRAPPER", wrapper);
         }
     }
-    cmd.env(
-        "CARGO_ENCODED_RUSTFLAGS",
-        bcx.rustflags_args(unit).join("\x1f"),
-    );
+    cmd.env("CARGO_ENCODED_RUSTFLAGS", unit.rustflags.join("\x1f"));
     cmd.env_remove("RUSTFLAGS");
 
     if build_runner.bcx.ws.gctx().extra_verbose() {

src/cargo/core/compiler/fingerprint/mod.rs

@@ -1415,9 +1415,9 @@ fn calculate_normal(
     // hashed to take up less space on disk as we just need to know when things
     // change.
     let extra_flags = if unit.mode.is_doc() || unit.mode.is_doc_scrape() {
-        build_runner.bcx.rustdocflags_args(unit)
+        &unit.rustdocflags
     } else {
-        build_runner.bcx.rustflags_args(unit)
+        &unit.rustflags
     }
     .to_vec();
 
@@ -1512,7 +1512,7 @@ fn calculate_run_custom_build(
 An I/O error happened. Please make sure you can access the file.
 
 By default, if your project contains a build script, cargo scans all files in
-it to determine whether a rebuild is needed. If you don't expect to access the 
+it to determine whether a rebuild is needed. If you don't expect to access the
 file, specify `rerun-if-changed` in your build script.
 See https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed for more information.";
             pkg_fingerprint(build_runner.bcx, &unit.pkg).map_err(|err| {
@@ -1542,7 +1542,7 @@ See https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-change
             .collect::<CargoResult<Vec<_>>>()?
     };
 
-    let rustflags = build_runner.bcx.rustflags_args(unit).to_vec();
+    let rustflags = unit.rustflags.to_vec();
 
     Ok(Fingerprint {
         local: Mutex::new(local),

src/cargo/core/compiler/mod.rs

@@ -683,7 +683,7 @@ fn prepare_rustc(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> CargoResult
     base.inherit_jobserver(&build_runner.jobserver);
     build_deps_args(&mut base, build_runner, unit)?;
     add_cap_lints(build_runner.bcx, unit, &mut base);
-    base.args(build_runner.bcx.rustflags_args(unit));
+    base.args(&unit.rustflags);
     if build_runner.bcx.gctx.cli_unstable().binary_dep_depinfo {
         base.arg("-Z").arg("binary-dep-depinfo");
     }
@@ -780,7 +780,7 @@ fn prepare_rustdoc(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> CargoResu
 
     rustdoc::add_output_format(build_runner, unit, &mut rustdoc)?;
 
-    rustdoc.args(bcx.rustdocflags_args(unit));
+    rustdoc.args(&unit.rustdocflags);
 
     if !crate_version_flag_already_present(&rustdoc) {
         append_crate_version_flag(unit, &mut rustdoc);

src/cargo/core/compiler/standard_lib.rs

@@ -178,6 +178,7 @@ pub fn generate_std_roots(
     package_set: &PackageSet<'_>,
     interner: &UnitInterner,
     profiles: &Profiles,
+    target_data: &RustcTargetData<'_>,
 ) -> CargoResult<HashMap<CompileKind, Vec<Unit>>> {
     // Generate the root Units for the standard library.
     let std_ids = crates
@@ -216,6 +217,8 @@ pub fn generate_std_roots(
                 *kind,
                 mode,
                 features.clone(),
+                target_data.info(*kind).rustflags.clone(),
+                target_data.info(*kind).rustdocflags.clone(),
                 /*is_std*/ true,
                 /*dep_hash*/ 0,
                 IsArtifact::No,

src/cargo/core/compiler/unit.rs

@@ -14,6 +14,7 @@ use std::fmt;
 use std::hash::{Hash, Hasher};
 use std::ops::Deref;
 use std::rc::Rc;
+use std::sync::Arc;
 
 /// All information needed to define a unit.
 ///
@@ -59,6 +60,28 @@ pub struct UnitInner {
     /// The `cfg` features to enable for this unit.
     /// This must be sorted.
     pub features: Vec<InternedString>,
+    /// Extra compiler flags to pass to `rustc` for a given unit.
+    ///
+    /// Although it depends on the caller, in the current Cargo implementation,
+    /// these flags take precedence over those from [`BuildContext::extra_args_for`].
+    ///
+    /// As of now, these flags come from environment variables and configurations.
+    /// See [`TargetInfo.rustflags`] for more on how Cargo collects them.
+    ///
+    /// [`BuildContext::extra_args_for`]: crate::core::compiler::build_context::BuildContext::extra_args_for
+    /// [`TargetInfo.rustflags`]: crate::core::compiler::build_context::TargetInfo::rustflags
+    pub rustflags: Arc<[String]>,
+    /// Extra compiler flags to pass to `rustdoc` for a given unit.
+    ///
+    /// Although it depends on the caller, in the current Cargo implementation,
+    /// these flags take precedence over those from [`BuildContext::extra_args_for`].
+    ///
+    /// As of now, these flags come from environment variables and configurations.
+    /// See [`TargetInfo.rustdocflags`] for more on how Cargo collects them.
+    ///
+    /// [`BuildContext::extra_args_for`]: crate::core::compiler::build_context::BuildContext::extra_args_for
+    /// [`TargetInfo.rustdocflags`]: crate::core::compiler::build_context::TargetInfo::rustdocflags
+    pub rustdocflags: Arc<[String]>,
     // if `true`, the dependency is an artifact dependency, requiring special handling when
     // calculating output directories, linkage and environment variables provided to builds.
     pub artifact: IsArtifact,
@@ -151,6 +174,7 @@ impl fmt::Debug for Unit {
             .field("kind", &self.kind)
             .field("mode", &self.mode)
             .field("features", &self.features)
+            .field("rustflags", &self.rustflags)
             .field("artifact", &self.artifact.is_true())
             .field(
                 "artifact_target_for_features",
@@ -198,6 +222,8 @@ impl UnitInterner {
         kind: CompileKind,
         mode: CompileMode,
         features: Vec<InternedString>,
+        rustflags: Arc<[String]>,
+        rustdocflags: Arc<[String]>,
         is_std: bool,
         dep_hash: u64,
         artifact: IsArtifact,
@@ -231,6 +257,8 @@ impl UnitInterner {
             kind,
             mode,
             features,
+            rustflags,
+            rustdocflags,
             is_std,
             dep_hash,
             artifact,

src/cargo/core/compiler/unit_dependencies.rs

@@ -859,6 +859,8 @@ fn new_unit_dep_with_profile(
         kind,
         mode,
         features,
+        state.target_data.info(kind).rustflags.clone(),
+        state.target_data.info(kind).rustdocflags.clone(),
         state.is_std,
         /*dep_hash*/ 0,
         artifact.map_or(IsArtifact::No, |_| IsArtifact::Yes),

src/cargo/ops/cargo_compile/mod.rs

@@ -360,6 +360,7 @@ pub fn create_bcx<'a, 'gctx>(
     let generator = UnitGenerator {
         ws,
         packages: &to_builds,
+        target_data: &target_data,
         filter,
         requested_kinds: &build_config.requested_kinds,
         explicit_host_kind,
@@ -399,6 +400,7 @@ pub fn create_bcx<'a, 'gctx>(
             &pkg_set,
             interner,
             &profiles,
+            &target_data,
         )?
     } else {
         Default::default()
@@ -694,6 +696,8 @@ fn traverse_and_share(
             to_host.unwrap(),
             unit.mode,
             unit.features.clone(),
+            unit.rustflags.clone(),
+            unit.rustdocflags.clone(),
             unit.is_std,
             unit.dep_hash,
             unit.artifact,
@@ -719,6 +723,8 @@ fn traverse_and_share(
         canonical_kind,
         unit.mode,
         unit.features.clone(),
+        unit.rustflags.clone(),
+        unit.rustdocflags.clone(),
         unit.is_std,
         new_dep_hash,
         unit.artifact,
@@ -880,6 +886,8 @@ fn override_rustc_crate_types(
             unit.kind,
             unit.mode,
             unit.features.clone(),
+            unit.rustflags.clone(),
+            unit.rustdocflags.clone(),
             unit.is_std,
             unit.dep_hash,
             unit.artifact,

src/cargo/ops/cargo_compile/unit_generator.rs

@@ -4,8 +4,8 @@ use std::fmt::Write;
 
 use crate::core::compiler::rustdoc::RustdocScrapeExamples;
 use crate::core::compiler::unit_dependencies::IsArtifact;
-use crate::core::compiler::UnitInterner;
 use crate::core::compiler::{CompileKind, CompileMode, Unit};
+use crate::core::compiler::{RustcTargetData, UnitInterner};
 use crate::core::dependency::DepKind;
 use crate::core::profiles::{Profiles, UnitFor};
 use crate::core::resolver::features::{self, FeaturesFor};
@@ -47,6 +47,7 @@ struct Proposal<'a> {
 pub(super) struct UnitGenerator<'a, 'gctx> {
     pub ws: &'a Workspace<'gctx>,
     pub packages: &'a [&'a Package],
+    pub target_data: &'a RustcTargetData<'gctx>,
     pub filter: &'a CompileFilter,
     pub requested_kinds: &'a [CompileKind],
     pub explicit_host_kind: CompileKind,
@@ -162,13 +163,16 @@ impl<'a> UnitGenerator<'a, '_> {
                     unit_for,
                     kind,
                 );
+                let kind = kind.for_target(target);
                 self.interner.intern(
                     pkg,
                     target,
                     profile,
-                    kind.for_target(target),
+                    kind,
                     target_mode,
                     features.clone(),
+                    self.target_data.info(kind).rustflags.clone(),
+                    self.target_data.info(kind).rustdocflags.clone(),
                     /*is_std*/ false,
                     /*dep_hash*/ 0,
                     IsArtifact::No,