Preserve argument indexes when inlining MIR

[davidlattimore]

We store argument indexes on VarDebugInfo. Unlike the previous method of relying on the variable index to know whether a variable is an argument, this survives MIR inlining.

We also no longer check if var.source_info.scope is the outermost scope. When a function gets inlined, the arguments to the inner function will no longer be in the outermost scope. What we care about though is whether they were in the outermost scope prior to inlining, which we know by whether we assigned an argument index.

Fixes #83217

I considered using Option<NonZeroU16> instead of Option<u16> to store the index. I didn't because TypeFoldable isn't implemented for NonZeroU16 and because it looks like due to padding, it currently wouldn't make any difference. But I indexed from 1 anyway because (a) it'll make it easier if later it becomes worthwhile to use a NonZeroU16 and because the arguments were previously indexed from 1, so it made for a smaller change.

This is my first PR on rust-lang/rust, so apologies if I've gotten anything not quite right.

[rustbot]

r? @wesleywiser

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

[wesleywiser]

Great work @davidlattimore! 💯

Sorry for the delay in reviewing.

@bors r+

[bors]

📌 Commit d45dae666996f51afa6355168265666d7ed8a47d has been approved by wesleywiser

It is now in the queue for this repository.

[bors]

⌛ Testing commit d45dae666996f51afa6355168265666d7ed8a47d with merge d660e8aee40cb06996960ecf2720939ede35a557...

[bors]

💔 Test failed - checks-actions

[davidlattimore]

Looks like it's breaking compilation of a particular async function in Tokio. Specifically some code in tokio/src/sync/watch.rs. I managed to minimise the code that breaks to the following:

use std::future::Future;

pub fn poll_recv() {
    let _: Box<dyn Future<Output = ()>> = Box::new(recv_unit());
}

async fn recv_unit() {
    std::future::ready(()).await;
}

This fails with:

conflicting debug info for argument
  call void @llvm.dbg.declare(metadata ptr %8, metadata !113, metadata !DIExpression()), !dbg !120
!118 = !DILocalVariable(arg: 2, scope: !6, file: !7, line: 7, type: !71)
!113 = !DILocalVariable(name: "_task_context", arg: 2, scope: !6, file: !7, line: 7, type: !71)

Line 7 here is async fn recv_unit. Interestingly Boxing the return value from recv_unit is necessary in order for the problem to happen.

If I compile the same code with stable (1.68.2), I still see two local variables:

!118 = !DILocalVariable(arg: 2, scope: !6, file: !7, line: 7, type: !71)
!113 = !DILocalVariable(name: "_task_context", scope: !6, file: !7, line: 7, type: !71, align: 8)

But here, the one with the name doesn't have an arg index set.

The extra DILocalVariable seems to be coming from debug_introduce_locals. The logic for computing fallback_var in debug_introduce_local seems to repeat some of what's in compute_per_local_var_debug_info. If I just delete the fallback_var stuff, the tests all pass and the duplicate local variable goes away. I need spend some time to understand how this fallback_var stuff came to exist and determine whether it's actually necessary. But I'll do that tomorrow, since it's late now.

[davidlattimore]

I think I understand what's happening now.

• When the async function is processed by make_async_expr, the argument _task_context is added as argument 2.
• The function is then lowered to MIR and we assign the argument index 2.
• The StateTransform MIR pass then sees that this argument survives past a suspension point, so puts it into a tuple.
• Now we have a local called _task_context that we consider to be argument 2, but which is actually assigned via a projection from a tuple that is the actual argument to the function.
• In compute_per_local_var_debug_info, we then see that this variable is argument 2 and emit debug info as such. This function only emits debug info for arguments with no projection.
• debug_introduce_local emits debug info for unnamed arguments - i.e. arguments that have projection. There is an argument 2 that has projection - the tuple that StateTransform introduced.
• Now we've emitted debug info for two different argument number 2s, one named and one unamed, both in the same scope. This makes LLVM unhappy.

We've got a few options for how to fix this:

1. Delete the code that emits debug information for unnamed arguments - the fallback_var code in debug_introduce_local. This is a sizable chunk of code that already has a comment on it saying FIXME(eddyb) is this really needed? It's indeed of questionable value having debug info for unnamed arguments. AFAICT gdb does nothing with this info. lldb does use it, but it seems of limited value. See below for difference in behaviour with lldb.
2. Change StateTransform to set VarDebugInfo.argument_index = None when it demotes what was previously an argument to be just a projection of an argument.
3. Change MIR generation so that a VarDebugInfo for all arguments, not just those with names, then emit debug info for both named and unnamed arguments in compute_per_local_var_debug_info. i.e. still delete the unnamed argument code from debug_introduce_local, but reproduce its functionality elsewhere.

Option 2 is simple (3 extra lines), but I don't really like it because it seems easy for something else to do the same thing (demote an argument to be not an argument).

Option 3 I haven't tried writing yet, but it's plausible that creating VarDebugInfo instances without a name (or with an empty name) could have unintended side effects.

Option 1 currently is my preferred option. Who doesn't like deleting code? For now, I've added a commit that goes with this option. I put it before the original commit.

Affect on lldb of not emitting debug information for unnamed arguments

Suppose we have the following code

fn foo((aaa, bbb): (i32, i32), ccc: i32) -> i32 {
    aaa + bbb + ccc
}

If we set a breakpoint inside foo in lldb, then run frame variable, we currently see:

((i32, i32))  = (__0 = 10, __1 = 2)
(int) ccc = 30
(int) aaa = 10
(int) bbb = 2

Without debug info for unnamed arguments, the first line goes away. This doesn't seem like a big loss to me - the information you want is still there in the variables with the names that user gave them - aaa and bbb.

[davidlattimore]

Looks like we do need to emit debug info for unnamed arguments, otherwise gdb doesn't include the argument types in the function signature. So just deleting that code isn't an option. I guess I'll go with one of the other options.

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[davidlattimore]

Option 3 didn't really work out, so I went with option 2. A fourth option occurred to me in the process. We could change compute_per_local_var_debug_info to only use VarDebugInfo.argument_index when not processing the outer scope and for the outer scope, do it the way it was before. i.e. only use VarDebugInfo.argument_index for functions that got inlined. For now I've stuck with option 2 because it seems slightly simpler, but let me know if you prefer option 4.

[wesleywiser]

Would you mind adding the minimal repro you found for the failure as another test in this PR? It would be good to make sure that is caught before perf.rlo runs.

[davidlattimore]

Would you mind adding the minimal repro you found for the failure as another test in this PR? It would be good to make sure that is caught before perf.rlo runs.

Good idea, done. I confirmed that the added test fails without my change to compiler/rustc_mir_transform/src/generator.rs

[wesleywiser]

Great work @davidlattimore! Thanks so much 😄

@bors r+

[bors]

📌 Commit a629267 has been approved by wesleywiser

It is now in the queue for this repository.

[bors]

⌛ Testing commit a629267 with merge d8fc819...

[bors]

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing d8fc819 to master...

[rust-timer]

Finished benchmarking commit (d8fc819): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.8%	[0.6%, 1.2%]	3
Regressions ❌ (secondary)	1.3%	[1.3%, 1.3%]	1
Improvements ✅ (primary)	-3.6%	[-3.6%, -3.6%]	1
Improvements ✅ (secondary)	-2.4%	[-3.0%, -1.8%]	2
All ❌✅ (primary)	-0.3%	[-3.6%, 1.2%]	4

Cycles

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.6%, 0.5%]	2