Eagerly compute output_filenames #117584

bjorn3 · 2023-11-04T18:00:44Z

It can be computed before creating TyCtxt. Previously the query would also write the dep info file, which meant that the output filenames couldn't be accessed before macro expansion is done. The dep info file writing is now done as a separate non-query function. The old query was always executed again anyways due to depending on the HIR.

Also encode the output_filenames in rlink files to ensure #![crate_name] affects the linking stage when doing separate compiling and linking using -Zno-link/-Zlink-only.

rustbot · 2023-11-04T18:00:52Z

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

cjgillot · 2023-11-04T18:39:25Z

compiler/rustc_interface/src/passes.rs

+    // Make sure name resolution and macro expansion is run for
+    // the side-effect of providing a complete set of all
+    // accessed files and env vars.
+    let _ = tcx.resolver_for_lowering(());


This should use ensure().

If resolver_for_lowering weren't eval_always and it is green, using ensure() would mean that the side effects of resolver_for_lowering (adding the list of used files and env vars to the Session) are not replayed as far as I understand it. Now resolver_for_lowering is eval_always currently, so it shouldn't matter for now I guess. There shouldn't be any perf difference though as resolver_for_lowering returns a reference.

Should we maybe add a comment in the query definition explaining this, i.e. why this needs to be eval_always?

cjgillot · 2023-11-04T18:45:30Z

compiler/rustc_session/src/config.rs

@@ -580,7 +580,7 @@ pub enum ResolveDocLinks {
 /// *Do not* switch `BTreeMap` out for an unsorted container type! That would break
 /// dependency tracking for command-line arguments. Also only hash keys, since tracking
 /// should only depend on the output types, not the paths they're written to.
-#[derive(Clone, Debug, Hash, HashStable_Generic)]
+#[derive(Clone, Debug, Hash, HashStable_Generic, Encodable, Decodable)]


Could this cause us to leak host info into build artifacts?

The rlink file already contains absolute file paths. rustc -Zlink-only foo.rlink is supposed to be called immediately after all dependencies of a rustc -Zlink-only invocation are finished. This has to be done on the same machine and can only be done once because all temporary files that are shared between the two rustc invocations have unpredictable names and are removed by rustc -Zlink-only.

bors · 2023-11-17T06:45:07Z

☔ The latest upstream changes (presumably #118001) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-11-17T19:35:29Z

☔ The latest upstream changes (presumably #117993) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-11-18T19:28:27Z

☔ The latest upstream changes (presumably #118002) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2023-11-20T21:32:31Z

compiler/rustc_interface/src/queries.rs

-    #[deprecated = "pre_configure may be made private in the future. If you need it please open an issue with your use case."]
-    pub fn pre_configure(&self) -> Result<QueryResult<'_, (ast::Crate, ast::AttrVec)>> {
-        self.pre_configure.compute(|| {
+    pub fn global_ctxt(&'tcx self) -> Result<QueryResult<'_, &'tcx GlobalCtxt<'tcx>>> {


nnethercote · 2023-11-20T21:33:56Z

The commit message of the third commit is truncated.

bjorn3 · 2023-11-21T11:00:22Z

I think the line that got removed originally started with #![crate_name = "..."], which git commit or git rebase then interpreted as a comment and removed.

bors · 2023-11-22T14:50:04Z

☔ The latest upstream changes (presumably #118086) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-11-26T17:26:59Z

☔ The latest upstream changes (presumably #117301) made this pull request unmergeable. Please resolve the merge conflicts.

This ensures that linking will use the correct crate name even when `#![crate_name = "..."]` is used to specify the crate name.

Since the introduction of the crate attribute pre-expansion pass we don't need access to the TyCtxt to compute it.

…t_filenames"

It has side-effects and as such can't be cached.

b-naber · 2023-11-26T20:16:34Z

compiler/rustc_interface/src/passes.rs

+    // Make sure name resolution and macro expansion is run for
+    // the side-effect of providing a complete set of all
+    // accessed files and env vars.
+    let _ = tcx.resolver_for_lowering(());


Should we maybe add a comment in the query definition explaining this, i.e. why this needs to be eval_always?

b-naber · 2023-11-26T20:19:11Z

@bors r+

bors · 2023-11-26T20:19:14Z

📌 Commit d7e9a30 has been approved by b-naber

It is now in the queue for this repository.

bors · 2023-11-27T00:03:54Z

⌛ Testing commit d7e9a30 with merge ac9b308...

bors · 2023-11-27T02:01:41Z

☀️ Test successful - checks-actions
Approved by: b-naber
Pushing ac9b308 to master...

rust-timer · 2023-11-27T04:06:11Z

Finished benchmarking commit (ac9b308): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.2%, 0.2%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.1%	[1.8%, 8.8%]	3
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-3.1%	[-4.1%, -1.5%]	8
All ❌✅ (primary)	-1.6%	[-1.6%, -1.6%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.544s -> 675.005s (0.22%)
Artifact size: 313.34 MiB -> 313.35 MiB (0.00%)

…r=bjorn3 Fix crate name validation Reject macro calls inside attribute `#![crate_name]` like in `#![crate_name = concat!("na", "me")]`. Prior to rust-lang#117584, the result of the expansion (here: `"name"`) would actually be properly picked up by the compiler and used as the crate name. However since rust-lang#117584 / on master, we extract the "value" (i.e., the *literal* string literal) of the `#![crate_name]` much earlier in the pipeline way before macro expansion and **skip**/**ignore** any `#![crate_name]`s "assigned to" a macro call. See also rust-lang#122001. T-lang has ruled to reject `#![crate_name = MACRO!(...)]` outright very similar to other built-in attributes whose value we need early like `#![crate_type]`. See accepted FCP: rust-lang#122001 (comment). Note that the check as implemented in this PR is even more "aggressive" compared to the one of `#![crate_type]` by running as early as possible in order to reject `#![crate_name = MACRO!(...)]` even in "non-normal" executions of `rustc`, namely on *print requests* (e.g., `--print=crate-name` and `--print=file-names`). If I were to move the validation step a bit further back close to the `#![crate_type]` one, `--print=crate-name` (etc.) would *not* exit fatally with an error in this kind of situation but happily report an incorrect crate name (i.e., the "crate name" as if `#![crate_name]` didn't exist / deduced from other sources like `--crate-name` or the file name) which would match the behavior on master. Again, see also rust-lang#122001. I'm mentioning this explicitly because I'm not sure if it was that clear in the FCP'ed issue. I argue that my current approach is the most reasonable one. I know (from reading the code and from past experiments) that various print requests are still quite broken (mostly lack of validation). To the best of my knowledge, there's no print request whose output references/contains a crate *type*, so there's no "inherent need" to move `#![crate_type]`'s validation to happen earlier. --- Fixes rust-lang#122001. https://github.com/rust-lang/rust/labels/relnotes: Compatibility. Breaking change.

Rollup merge of rust-lang#127581 - fmease:fix-crate_name-validation, r=bjorn3 Fix crate name validation Reject macro calls inside attribute `#![crate_name]` like in `#![crate_name = concat!("na", "me")]`. Prior to rust-lang#117584, the result of the expansion (here: `"name"`) would actually be properly picked up by the compiler and used as the crate name. However since rust-lang#117584 / on master, we extract the "value" (i.e., the *literal* string literal) of the `#![crate_name]` much earlier in the pipeline way before macro expansion and **skip**/**ignore** any `#![crate_name]`s "assigned to" a macro call. See also rust-lang#122001. T-lang has ruled to reject `#![crate_name = MACRO!(...)]` outright very similar to other built-in attributes whose value we need early like `#![crate_type]`. See accepted FCP: rust-lang#122001 (comment). Note that the check as implemented in this PR is even more "aggressive" compared to the one of `#![crate_type]` by running as early as possible in order to reject `#![crate_name = MACRO!(...)]` even in "non-normal" executions of `rustc`, namely on *print requests* (e.g., `--print=crate-name` and `--print=file-names`). If I were to move the validation step a bit further back close to the `#![crate_type]` one, `--print=crate-name` (etc.) would *not* exit fatally with an error in this kind of situation but happily report an incorrect crate name (i.e., the "crate name" as if `#![crate_name]` didn't exist / deduced from other sources like `--crate-name` or the file name) which would match the behavior on master. Again, see also rust-lang#122001. I'm mentioning this explicitly because I'm not sure if it was that clear in the FCP'ed issue. I argue that my current approach is the most reasonable one. I know (from reading the code and from past experiments) that various print requests are still quite broken (mostly lack of validation). To the best of my knowledge, there's no print request whose output references/contains a crate *type*, so there's no "inherent need" to move `#![crate_type]`'s validation to happen earlier. --- Fixes rust-lang#122001. https://github.com/rust-lang/rust/labels/relnotes: Compatibility. Breaking change.

rustbot assigned b-naber Nov 4, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 4, 2023

cjgillot reviewed Nov 4, 2023

View reviewed changes

bjorn3 force-pushed the eager_output_filenames branch from 2aa5e3c to 2b849e6 Compare November 17, 2023 08:40

bjorn3 force-pushed the eager_output_filenames branch from 2b849e6 to bea98b1 Compare November 18, 2023 11:36

bjorn3 mentioned this pull request Nov 20, 2023

Queries cleanups #118086

Merged

nnethercote reviewed Nov 20, 2023

View reviewed changes

bjorn3 force-pushed the eager_output_filenames branch from bea98b1 to 0709954 Compare November 21, 2023 10:59

bjorn3 force-pushed the eager_output_filenames branch from 0709954 to 4b809b6 Compare November 22, 2023 16:19

bjorn3 added 7 commits November 26, 2023 17:43

Inline and remove pre_configure

457dbbf

Accept crate name instead of attributes in build_output_filenames

eacbe65

Serialize OutputFilenames into rmeta file

98a6eaa

This ensures that linking will use the correct crate name even when `#![crate_name = "..."]` is used to specify the crate name.

Feed the output filenames into the TyCtxt

4acaa02

Since the introduction of the crate attribute pre-expansion pass we don't need access to the TyCtxt to compute it.

Ensure macro expansion runs before writing the dep info

7ede8e2

Mostly revert "Accept crate name instead of attributes in build_outpu…

365a580

…t_filenames"

Turn write_dep_info into a regular function

d7e9a30

It has side-effects and as such can't be cached.

bjorn3 force-pushed the eager_output_filenames branch from 4b809b6 to d7e9a30 Compare November 26, 2023 18:03

b-naber approved these changes Nov 26, 2023

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 26, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 27, 2023

bors merged commit ac9b308 into rust-lang:master Nov 27, 2023

rustbot added this to the 1.76.0 milestone Nov 27, 2023

bors mentioned this pull request Nov 27, 2023

resolve: Feed the def_kind query immediately on DefId creation #118188

Merged

bjorn3 deleted the eager_output_filenames branch November 28, 2023 16:19

ehuss mentioned this pull request Mar 5, 2024

#![crate_name = EXPR] semantically allows EXPR to be a macro call but otherwise mostly ignores it #122001

Closed

fmease mentioned this pull request Jul 18, 2024

Fix crate name validation #127581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eagerly compute output_filenames #117584

Eagerly compute output_filenames #117584

bjorn3 commented Nov 4, 2023

rustbot commented Nov 4, 2023

cjgillot Nov 4, 2023

bjorn3 Nov 4, 2023

b-naber Nov 26, 2023

cjgillot Nov 4, 2023

bjorn3 Nov 4, 2023

bors commented Nov 17, 2023

bors commented Nov 17, 2023

bors commented Nov 18, 2023

nnethercote Nov 20, 2023

nnethercote commented Nov 20, 2023

bjorn3 commented Nov 21, 2023

bors commented Nov 22, 2023

bors commented Nov 26, 2023

b-naber Nov 26, 2023

b-naber commented Nov 26, 2023

bors commented Nov 26, 2023

bors commented Nov 27, 2023

bors commented Nov 27, 2023

rust-timer commented Nov 27, 2023

Eagerly compute output_filenames #117584

Eagerly compute output_filenames #117584

Conversation

bjorn3 commented Nov 4, 2023

rustbot commented Nov 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 17, 2023

bors commented Nov 17, 2023

bors commented Nov 18, 2023

Choose a reason for hiding this comment

nnethercote commented Nov 20, 2023

bjorn3 commented Nov 21, 2023

bors commented Nov 22, 2023

bors commented Nov 26, 2023

Choose a reason for hiding this comment

b-naber commented Nov 26, 2023

bors commented Nov 26, 2023

bors commented Nov 27, 2023

bors commented Nov 27, 2023

rust-timer commented Nov 27, 2023

Overall result: ❌✅ regressions and improvements - no action needed