Rebuild on changes to the deployment target when compiling Apple targets

[madsmtm]

Rebuild, both in Cargo and the incremental cache, when the *_DEPLOYMENT_TARGET environment variables change.

Fixes #118204. See #129432 for more motivation behind this change.

I am a bit unsure if I have implemented this correctly, I'm particularly uncertain whether creating a new field in Options the correct approach for busting the incremental cache? It's only the codegen that needs to be re-run, everything before that shouldn't be influenced by this, so if there's a way to make that happen, we should do that instead.

Builds upon #130068.

r? thomcc

CC @BlackHoleFox

[jieyouxu]

I'd also like some input on how to add a test for these kinds of incremental compilation changes?

If the incremental test suite doesn't suit your needs, you could fallback to a run-make test if needed.

[madsmtm]

I'd also like some input on how to add a test for these kinds of incremental compilation changes?

If the incremental test suite doesn't suit your needs, you could fallback to a run-make test if needed.

Thanks for the tip, I took a look at the incremental test suite, but ended up creating a run-make test instead. Note that this change is going to conflict with #128507, since I updated the test from in there.

[jieyouxu]

Note that this change is going to conflict with #128507, since I updated the test from in there.

I'll discuss this with @Oneirical, if this PR is updating the macos deployment test we might as well drop the port in #128507.

[madsmtm]

@rustbot ready

[petrochenkov]

Rebuild, both in Cargo and the incremental cache

I think the correct way to do this would be to integrate env var reading into rustc's query system.
We can make an eval_always query that reads the variable, then it will automatically poison all its dependent queries if the env var changes between compilations.
This query would then be used for reading all environment variables, not just *_DEPLOYMENT_TARGET.

The query can also put the read variable into sess.psess.env_depinfo, then it will be automatically tracked for cargo as well.

It would also be better to split the Apple-specific parts, including tests, from rustc infrastructure changes, put them into a different PR and land it separately.

[petrochenkov]

There are some issues related to target specs though.

First, the query system is not available in rustc_target when targets are loaded.
So we may need to re-query the relevant env vars used from there later.

Second, with --check-cfg we load all the targets, including the Apple ones, and depend on the Apple env vars.
--check-cfg is enabled by default now, so we technically depend on the Apple-related env vars on all hosts and targets, in most scenarios, if we consider just constructing a Target(Option) value a dependency.

Which fields of the target specs actually depend on the env vars?
Maybe the logic related to those fields can be excluded from target specs and moved to some other place (link.rs or where else they are necessary)?
Then the dependencies will be introduced only when strictly necessary.

[petrochenkov]

Linker arguments depending on env vars can certainly be added in link.rs, instead of target specs, likely in fn add_order_independent_options.

The llvm_target field can be versionless in target specs, the version can be added only when strictly necessary, e.g. before passing to linker or maybe to LLVM backend as well.

(Maybe I missed some other fields, didn't look very carefully.)

[madsmtm]

It would also be better to split the Apple-specific parts, including tests, from rustc infrastructure changes, put them into a different PR and land it separately.

Good point, I've started with the tests in #130068, will get back to your other comments.

@rustbot blocked

[madsmtm]

Linker arguments depending on env vars can certainly be added in link.rs, instead of target specs, likely in fn add_order_independent_options.

Yes, that's also likely what I will end up doing anyhow to implement proper SDKROOT reading.

The llvm_target field can be versionless in target specs, the version can be added only when strictly necessary, e.g. before passing to linker or maybe to LLVM backend as well.

I... never actually really considered that, might be doable? And would be a lot cleaner, I really dislike that rustc_target is impure in this fashion.

(Maybe I missed some other fields, didn't look very carefully.)

I don't think you missed any, but I think the question should rather be "which fields could we possible want to depend on the deployment target in the future". That is, I would like to change rustc_target so that it doesn't depend on these, but it has to be forwards compatible, so that future people (likely myself) that wants to configure a target specially depending on the deployment target, is empowered to do so.

I'm going through all the current Target[Options], and considering whether they might want to change depending on deployment target:

Details

// Target
llvm_target -> yes, as discussed
metadata -> no
pointer_width -> no
arch -> possibly, but probably no
data_layout -> possibly

// TargetOptions
is_builtin -> no
endian -> no
c_int_width -> no
os -> no
env -> no
abi -> no
vendor -> no
linker -> no, though might want to depend on the SDK version
linker_flavor -> no
linker_flavor_json -> no
lld_flavor_json -> no
linker_is_gnu_json -> no
*link* -> yes, as discussed, but only desired to load when actually linking
asm_args -> yes
cpu -> yes, definitely
features -> yes, definitely
direct_access_external_data -> no
dynamic_linking -> possibly, watchOS might start supporting it
dll_tls_export -> no
only_cdylib -> no
executables -> no
relocation_model -> unlikely
code_model -> no
tls_model -> no
disable_redzone -> no
frame_pointer -> possibly?
function_sections -> no
dll_prefix -> no
dll_suffix -> no
exe_suffix -> no
staticlib_prefix -> no
staticlib_suffix -> no
families -> unlikely
abi_return_struct_as_int -> no
is_like_* -> no
default_dwarf_version -> yes
allows_weak_linkage -> no
has_rpath -> no
no_default_libraries -> no
position_independent_executables -> no
static_position_independent_executables -> no
plt_by_default -> no
relro_level -> no
archive_format -> no
allow_asm -> no
main_needs_argc_argv -> no
has_thread_local -> would've been useful in the past
obj_is_bitcode -> no
forces_embed_bitcode -> would've been useful in the past
bitcode_llvm_cmdline -> no
min_atomic_width -> no
max_atomic_width -> no
atomic_cas -> no
panic_strategy -> no
crt_static_allows_dylibs -> no
crt_static_default -> no
crt_static_respected -> no
stack_probes -> possibly
min_global_align -> no
default_codegen_units -> no
default_codegen_backend -> no
trap_unreachable -> no
requires_lto -> no
singlethread -> no
no_builtins -> no
default_hidden_visibility -> no
emit_debug_gdb_scripts -> no
requires_uwtable -> no
default_uwtable -> no
simd_types_indirect -> no
limit_rdylib_exports -> no
override_export_symbols -> no
merge_functions -> no
mcount -> no
llvm_mcount_intrinsic -> no
llvm_abiname -> no
relax_elf_relocations -> no
llvm_args -> possibly
use_ctors_section -> no
eh_frame_header -> no
has_thumb_interworking -> no
debuginfo_kind -> no
split_debuginfo -> no
supported_split_debuginfo -> no
supported_sanitizers -> no
c_enum_min_bits -> no
generate_arange_section -> no
supports_stack_protector -> no
entry_name -> no
entry_abi -> no
supports_xray -> no

In summary, most of these are really just codegen, though some of them would be interesting for the standard library to do version detection, and emit more performant implementations if the deployment target is high enough. And in those cases, we do want it to be integrated properly with the query system, such that only e.g. the MIR in the standard library that depend on those would need to be changed.

A query like deployment_target would also be useful for something like rust-lang/rfcs#3379, if we had cfg!(os_version_min(...)) we'd want incremental compilation to work with that.

All of this to say that I've convinced myself that turning it into a query is a good idea (I kinda regret most of #129341 now, the big reason I did it was to consistently pass the deployment target to llvm_target. But oh well, live and learn).

[madsmtm]

Just to make sure I understand correctly, I would add a query env_var in compiler/rustc_middle/src/query/mod.rs? And then in another PR I would change llvm_target to be versionless in target specs, and make the deployment target be loaded using the query instead? Should that also be a query like query deployment_target, or should that just be a regular method that calls env_var internally?

And since I'm asking anyways, though I'll do it separately for sure, as part of #129432 I know that I'm also gonna need the ability to mark certain system file paths as part of the incremental cache - should this be done generally as well (query file_changed), or is it niche enough that it should just be its own query sdkroot?

[madsmtm]

Linker arguments depending on env vars can certainly be added in link.rs, instead of target specs, likely in fn add_order_independent_options.

Actually, how would I go about doing that, if I wanted the deployment target to be a query? It seems like the TyCtx<'_> no longer exists when add_order_independent_options is called? Although there is ParseSess::env_depinfo, do you mean I should use that?

[petrochenkov]

Just to make sure I understand correctly, I would add a query env_var in compiler/rustc_middle/src/query/mod.rs?

Yes, seems about right.

And then in another PR I would change llvm_target to be versionless in target specs, and make the deployment target be loaded using the query instead?

Yep.

Should that also be a query like query deployment_target, or should that just be a regular method that calls env_var internally?

Just env_var query should be enough for a start.

I'm also gonna need the ability to mark certain system file paths as part of the incremental cache

This is better left for the last, I don't think we are currently doing anything like that in the compiler.

It seems like the TyCtx<'_> no longer exists when add_order_independent_options is called?

add_order_independent_options has the codegen_results.crate_info in arguments, tcx is available when it is constructed.

[petrochenkov]

#130068 has landed, unblocking.
@rustbot author

[madsmtm]

It seems like the TyCtx<'_> no longer exists when add_order_independent_options is called?

add_order_independent_options has the codegen_results.crate_info in arguments, tcx is available when it is constructed.

Ah, I see now, thanks!

I will try to find some time next week to implement what we've discussed.