Driver cleanups #133198

nnethercote · 2024-11-19T06:54:45Z

Minor improvements I found while looking at this code.

The `Ok(())` was doing nothing.

Instead of passing in and returning an `AstFragment`. It's a little simpler that way, avoiding the need to return a tuple.

nnethercote · 2024-11-19T06:54:54Z

@bors rollup

rust-log-analyzer · 2024-11-19T07:18:01Z

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#16 exporting to docker image format
#16 sending tarball 28.1s done
#16 DONE 34.2s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
---- [ui] tests/ui-fulldeps/stable-mir/check_def_ty.rs stdout ----

error: test run failed!
status: exit status: 101
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/stable-mir/check_def_ty" && RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/stable-mir/check_def_ty/a"
--- stderr -------------------------------
warning: the feature `core_intrinsics` is internal to the compiler or standard library
##[warning] --> defs_ty_input.rs:3:20
  |
---

warning: constant `CONST_U32` is never used
##[warning] --> defs_ty_input.rs:5:15
  |
5 |         const CONST_U32: u32 = 0u32;

warning: function `extern_fn` is never used
##[warning]  --> defs_ty_input.rs:14:16
   |
   |
14 |             fn extern_fn(x: i32) -> i32;

warning: static `EXT_STATIC` is never used
##[warning]  --> defs_ty_input.rs:15:20
   |
---
--- stderr -------------------------------
warning: variable does not need to be mutable
##[warning]  --> alloc_input.rs:16:56
   |
16 |         pub unsafe extern "C" fn variadic_fn(n: usize, mut args: ...) -> usize {
   |                                                        |
   |                                                        help: remove this `mut`
   |
   = note: `#[warn(unused_mut)]` on by default
---

warning: static `C_STR` is never used
##[warning] --> alloc_input.rs:8:12
  |
8 |     static C_STR: &std::ffi::CStr = c"cstr";
  |
  = note: `#[warn(dead_code)]` on by default

warning: function `check_type_id` is never used
---

warning: unused variable: `vtable_sz`
##[warning] --> binop_input.rs:5:17
  |
5 |             let vtable_sz = unsafe { vtable_size(0 as *const ()) };
  |                 ^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_vtable_sz`
  = note: `#[warn(unused_variables)]` on by default

thread 'rustc' panicked at compiler/rustc_interface/src/queries.rs:37:25:
already borrowed: BorrowMutError
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: 2 warnings emitted
------------------------------------------


---- [ui] tests/ui-fulldeps/obtain-borrowck.rs stdout ----
Saved the actual run.stdout to "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/obtain-borrowck/obtain-borrowck.run.stdout"

- Bodies retrieved for:
- ::X::provided
- ::foo
- ::foo
- ::main
- ::main::{constant#0}
- ::{impl#0}::new
- ::{impl#1}::provided
- ::{impl#1}::required


The actual run.stdout differed from the expected run.stdout.
The actual run.stdout differed from the expected run.stdout.
Saved the actual run.stderr to "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/obtain-borrowck/obtain-borrowck.run.stderr"
thread 'rustc' panicked at compiler/rustc_interface/src/queries.rs:37:25:
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace




The actual run.stderr differed from the expected run.stderr.

error: 2 errors occurred comparing run output.
status: exit status: 101
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/obtain-borrowck" && RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/obtain-borrowck/a" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--edition=2021" "/checkout/tests/ui-fulldeps/auxiliary/obtain-borrowck-input.rs"
--- stderr -------------------------------
thread 'rustc' panicked at compiler/rustc_interface/src/queries.rs:37:25:
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

jieyouxu · 2024-11-19T07:31:40Z

I suspect the queries.global_ctxt()? was indeed for abiding with dynamic borrowck rules

bjorn3 · 2024-11-19T08:22:20Z

This PR looks fine to me. Just FYI: I've got a whole bunch of driver refactorings myself that may conflict with whatever cleanups you want to do to the driver. Part of them are in #132410 and the rest are in a branch somewhere waiting for this PR to be merged.

jieyouxu · 2024-11-19T08:37:07Z

@bjorn3 what would you like to do, coalesce this PR into #132410?

bjorn3 · 2024-11-19T08:41:29Z

I'm fine with this PR being merged separately. It contains some changes that are not in my PR and it may take a while before my PR gets reviewed.

jieyouxu

Thanks, this also LGTM otherwise too, feel free to r=me after PR CI is green.

petrochenkov · 2024-11-19T14:26:12Z

compiler/rustc_driver_impl/src/lib.rs

@@ -424,13 +424,14 @@ fn run_compiler(
            }

            // Make sure name resolution and macro expansion is run.
-            queries.global_ctxt()?.enter(|tcx| tcx.resolver_for_lowering());
+            let mut gcx = queries.global_ctxt()?;


This change was attempted before and reverted - #107740.

Huh, we should add a comment to point this out linking to #107740 and why.

FWIW #132410 changes after_analysis to pass in TyCtxt directly, avoiding the double locking issue.

petrochenkov · 2024-11-19T14:28:34Z

compiler/rustc_expand/src/expand.rs

        let orig_expansion_data = self.cx.current_expansion.clone();
        let orig_force_mode = self.cx.force_mode;

        // Collect all macro invocations and replace them with placeholders.
-        let (mut fragment_with_placeholders, mut invocations) =
-            self.collect_invocations(input_fragment, &[]);
+        let mut invocations = self.collect_invocations(&mut fragment, &[]);


The naming (input_fragment, fragment_with_placeholders, expanded_fragment) was important to me when I (re)wrote this code.
Otherwise it's not clear what is actually happening with the fragment during the mutations.

I guess we should also leave a comment explaining the difference between these fragment phases?

jieyouxu · 2024-11-19T14:37:27Z

(Unassigning myself as petrochenkov knows about this part better.)

nnethercote added 3 commits November 19, 2024 10:08

Merge two queries entry calls into one.

3dca0c1

The `Ok(())` was doing nothing.

Factor out repeated queries.global_ctxt()? calls.

7b81cdd

Use &mut AstFragment in MacroExpander::collect_invocations.

e6ddccd

Instead of passing in and returning an `AstFragment`. It's a little simpler that way, avoiding the need to return a tuple.

rustbot assigned jieyouxu Nov 19, 2024

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 19, 2024

jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2024

jieyouxu approved these changes Nov 19, 2024

View reviewed changes

petrochenkov reviewed Nov 19, 2024

View reviewed changes

petrochenkov self-assigned this Nov 19, 2024

jieyouxu removed their assignment Nov 19, 2024

nnethercote closed this Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Driver cleanups #133198

Driver cleanups #133198

nnethercote commented Nov 19, 2024

nnethercote commented Nov 19, 2024

rust-log-analyzer commented Nov 19, 2024

jieyouxu commented Nov 19, 2024 •

edited

Loading

bjorn3 commented Nov 19, 2024

jieyouxu commented Nov 19, 2024

bjorn3 commented Nov 19, 2024

jieyouxu left a comment

petrochenkov Nov 19, 2024

jieyouxu Nov 19, 2024

bjorn3 Nov 19, 2024

petrochenkov Nov 19, 2024

jieyouxu Nov 19, 2024

jieyouxu commented Nov 19, 2024

Driver cleanups #133198

Driver cleanups #133198

Conversation

nnethercote commented Nov 19, 2024

nnethercote commented Nov 19, 2024

rust-log-analyzer commented Nov 19, 2024

jieyouxu commented Nov 19, 2024 • edited Loading

bjorn3 commented Nov 19, 2024

jieyouxu commented Nov 19, 2024

bjorn3 commented Nov 19, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

petrochenkov Nov 19, 2024

Choose a reason for hiding this comment

jieyouxu Nov 19, 2024

Choose a reason for hiding this comment

bjorn3 Nov 19, 2024

Choose a reason for hiding this comment

petrochenkov Nov 19, 2024

Choose a reason for hiding this comment

jieyouxu Nov 19, 2024

Choose a reason for hiding this comment

jieyouxu commented Nov 19, 2024

jieyouxu commented Nov 19, 2024 •

edited

Loading