[theemathas]

View all comments

An LLM was used to locate relevant code, suggest ideas, and review the code. However, I manually wrote all code, and I take all responsibility for all code written and all decisions made.

Tracking issue (async_fn_track_caller): #110011

Related tracking issue (closure_track_caller): #87417

The first commit adds extensive tests for behavior touched in this PR. Commits 2-7 are the actual implementation, and also modification of the tests to reflect this new behavior.

This PR makes coroutines desugared from #[track_caller] async fn track the caller of the function, not the poller/awaiter of the coroutine. This is done as per T-lang's decision at #110011 (comment). The implementation modifies AST lowering to add an upvar to the coroutine, which stores the relevant caller location.

In both codegen and in consteval/miri, we read the relevant upvar from the coroutine at the point in time where we need access to the &Location reference (either when we call the caller_location intrinsic, when we panic, or when we call another #[track_caller] function). We only dereference this reference as needed, when we actually need the Location information.

This PR changes the behavior of async fn, but not of async closures, due to difficulties I've noted at #t-compiler/help > Help with `async_fn_track_caller` @ 💬

This PR's implementation assumes that the desired behavior in #110011 (comment) is the behavior that I think makes the most sense.

This PR corrects the behavior of #163406 when the async_fn_track_caller feature is enabled, but does not give a warning when the feature is disabled (resulting in the pre-existing broken behavior).

The tests pass on cranelift when I run ./x test ui --test-codegen-backend cranelift -- async-await/track-caller/panic-track-caller-no-unwind.rs. (Cranelift doesn't support unwinding.)

r? compiler

[theemathas]

I don't understand what this is doing, but I'm doing the same thing as this existing code:

rust/compiler/rustc_ast_lowering/src/item.rs

Lines 1148 to 1155 in c1070d6

	let (effective_ident, impl_kind) = if is_in_trait_impl {
	let trait_item_def_id = self
	.get_partial_res(i.id)
	.and_then(|r| r.expect_full_res().opt_def_id())
	.ok_or_else(|| {
	self.dcx()
	.span_delayed_bug(span, "could not resolve trait item being implemented")
	});

View changes since the review

[theemathas]

What is this, and do I need to modify it?

View changes since the review

[rustbot]

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[theemathas]

I split up the implementation into multiple commits, hopefully making reviewing easier. (And also hopefully making it less confusing for me.)

[RalfJung]

I don't know the MIR being run here, it is generated by the async lowering. You are asking me to spend a lot of time reverse engineering the exact steps that the auto-generated MIR is taking here. Is that worth it? Do you for some reason not want that code to have UB?
My guess would be that the MIR actually loads that field at its reference type and therefore it's UB for that field to be null / dangling. You can dig into this more yourself with something like the following that will show a trace of each MIR statement being executed:

MIRI_LOG=info ./x run miri -- src/tools/miri/tests/fail/async_fn_track_caller/null-location-ref.rs

seem to indicate that there's something special about the first poll of an async fn

It looks like the caller location isn't read every single time the async fn is polled. It's apparently only read on the first poll and the third poll. I don't know much about the sync lowering so I can't tell you why that is the case.

I don't understand the point of these tests, to be honest. Why should we have such fragile tests that probe implementation details of the generator lowerings?

[theemathas]

@RalfJung

Why should we have such fragile tests that probe implementation details of the generator lowerings?

I might have misunderstood your intentions. I thought that you wanted me to have the miri implementation of track_caller match the run time implementation so that Miri's UB always matches whatever the run time implementation assumes. Making sure that this is the case presumably requires fragile tests that reflect what things are assumed by the implementation. Am I misunderstanding what I was supposed to do?

[theemathas]

A potential alternative is to, in miri, require that all caller locations inside #[track_caller] async fn coroutines point to miri-allocated caller-location-specific allocations. This would detect more UB than what the runtime needs, but would make explaining the UB easier, and would still allow most if not all sane uses of such coroutines. I'm not entirely sure though about when exactly this check should be done, if we go with this approach.

[theemathas]

I just figured out why the tests behaved so weirdly. It's because I did a dumb and made a typed copy of the &Location references at 9e15722 (this PR). This typed copy runs on exactly the first poll of the coroutine. I'll get around to fixing this later, after which I expect the behaviour of stuff to make a lot more sense.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[oli-obk]

also use get_root_variable here, and compare var_id to captured_caller_location before shadowing var_id with its local id

View changes since the review

[theemathas]

I'm very confused by what you're talking about. This is pre-existing code before my PR, so I'm not exactly sure what this code is doing. And what "shadowing" are you talking about?

[oli-obk]

this snipped does exactly what you do below, except inlined. get_root_variable's body is literally this piece of code. So I just wanna deduplicate the check once, but you can't just use var_id below in your code, as it gets shadowed before your code is invoked, so you'd need to move your code up

[oli-obk]

I don't get this, isn't your PR changing exactly that (poll becoming irrelevant for determining the caller location)?

View changes since the review

[theemathas]

This is talking about the self argument of the poll call, since that's the argument storing the coroutine, which is where the caller location info is stored.

[oli-obk]

Ah I see, the comment makes sense now, that's kinda annoying that we don't inline track caller async fns anymore. Tho I don't seen any immediately good way to fix that beyond going all the way and making track caller actually be desugared much earlier

[oli-obk]

Is the intent with having the direct/captured difference that we only generate the operand for loading the captured field if/where it is actually used? Because I would have thought we'd just not have CallerLocation, but just generate the OperandRef here directly.

I'd assume that we always need it anyway, as you'll either forward it to things you call or are panicking somehow somewhere

View changes since the review

[theemathas]

I'm not sure what you mean by "you'll either forward it to things you call or are panicking somehow somewhere". We need to forward it to things we call only if we're calling a #[track_caller] function. And panicking is a rare one-time operation.

If we load the captured caller location eagerly, we'd potentially need to do so every time poll() is called. I'm not convinced that's a good idea.

[oli-obk]

you're not actually generating any code here, you're just building a projection to read from, and then use that operand at every use site. Or can this logic actually introduce temporaries?

[oli-obk]

even then, this seems like sth LLVM would totally reorder properly, but I guess it's nice not to have to rely on opts

[theemathas]

Can a projection go through two references?

I'm not convinced that LLVM can reorder this. I think our aliasing assumptions aren't enough to prevent the captured caller location from being modified in the middle of the poll call.

[theemathas]

Oh, I see what you mean.

I'll see if doing this is feasible.

[theemathas]

Note: T-lang came up with some new ideas, so I'm currently not quite sure yet how to proceed. I'll have to think about this more.

[theemathas]

I changed the mechanism to capture the caller location: Instead of creating an unused variable, I instead add a special-cased capture for it. This avoids unnecessarily reading the caller location on the first poll, which was previously causing strange miri behavior.

I've realized though that, in Miri, I need to retag the captured caller location each time we use it, and add a protector when passing it as an argument. I'll get around to that at some point.

[theemathas]

I'm going to wait until everything's finalized with T-lang before I continue working on this.

[RalfJung]

@RalfJung

Why should we have such fragile tests that probe implementation details of the generator lowerings?

I might have misunderstood your intentions. I thought that you wanted me to have the miri implementation of track_caller match the run time implementation so that Miri's UB always matches whatever the run time implementation assumes. Making sure that this is the case presumably requires fragile tests that reflect what things are assumed by the implementation. Am I misunderstanding what I was supposed to do?

Hm, fair. If there are places where codegen does something special for track_caller, such as adding noalias readonly attributes, we should have tests for that, to ensure that Miri does retagging.

But everything that's implemented entirely in the MIR corotuine lowering pass, we don't systematically have tests for in Miri -- we just run this like we run any other MIR code. With track_caller being partially implemented in the lowering and partially in the backend/Miri, I am not sure where to draw the line. The main thing that happens in codegen is that this pointer to the caller location gets dereferenced, right? So we should have a Miri test where that pointer is non-null and well-aligned but points to garbage memory (e.g. uninitialized, or otherwise not a valid Location), to ensure we run the usual tests. I don't think we need separate tests for null / dangling pointers -- if we can check the memory the pointer points to, we are definitely running the checks that ensure that such memory even exists.

[RalfJung]

I've realized though that, in Miri, I need to retag the captured caller location each time we use it, and add a protector when passing it as an argument. I'll get around to that at some point.

Why retag on every use? I would think that protected-retag when passed as argument should suffice.

[rust-bors]

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