Add #[track_caller] to track callers when initializing poisoned Once

[reez12g]

This PR is for this Issue.
#87707

With this fix, we expect to be able to track the caller when poisoned Once is initialized.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

Please see the contribution instructions for more information.

[yaahc]

Hey, thank you for taking on this issue :D. Let's start with a perf test.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit f4576017596cf669578e6075b186727feaca9d22 with merge d383a74d639169718e7de27d4754aebb019b5d4a...

[bors]

☀️ Try build successful - checks-actions
Build commit: d383a74d639169718e7de27d4754aebb019b5d4a (d383a74d639169718e7de27d4754aebb019b5d4a)

[rust-timer]

Queued d383a74d639169718e7de27d4754aebb019b5d4a with parent 68369a0, future comparison URL.

[rust-timer]

Finished benchmarking commit (d383a74d639169718e7de27d4754aebb019b5d4a): comparison url.

Summary: This benchmark run shows 4 relevant regressions 😿 to instruction counts.

• Average relevant regression: 0.8%
• Largest regression in instruction counts: 0.8% on incr-unchanged builds of externs debug

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[reez12g]

@yaahc
Thanks for running the perf test!
Looking at the results, it looks like there is regression, so I would like to discuss how to deal with it.

1. ignore the regression
2. resolve the regression (but I haven't figured out how to do that yet)
3. close this Issue (since the method that gave #[track_caller] is a cold one)

Which is better?

[yaahc]

Which is better?

I wasn't sure at first since I'm not super familiar with the rustc-perf test suite so I asked some other team members on the zulip and it seems that the regressions on the externs crate stress test are small enough that we can ignore them.

For next steps, Can you add a UI test to make sure we've added #[track_caller] to all the functions we need to in order to get the correct panic location in usercode? I suspect that in the process of setting up the UI test we will discover that Mark was right and that the location where the Once was accessed won't be nearly as useful as the location where it was poisoned. Though lets see what kind of error messages we get first before we consider more significant changes to track poisoning locations.

[nnethercote]

@rustbot label: +perf-regression-triaged

Yeah, externs is a stress test and a tiny regression is not an issue.

[reez12g]

OK, thanks.
I'll add a UI test.

[Mark-Simulacrum]

It's worth noting that perf is probably a pretty bad way to measure the effects of a patch like this. The compiler doesn't use std::sync::Once pretty much at all, so this is really at best measuring the extra codegen time if any of our benchmarks in the compiler suite are using it. Constructing proper benchmarks for something like Once is probably pretty difficult, so in the absence of them I would likely not block on perf concerns.

We definitely do want a test, though: I suspect the current impl is actually broken; it looks like the track_caller is not put on the call_once method, so this is just moving the caller tracking one level of indirection up, but still pointing to a spot in std.

[reez12g]

it looks like the track_caller is not put on the call_once method

Oh, I'm sure you're right.
I'll amend it, thanks.

[yaahc]

it looks like the track_caller is not put on the call_once method

Oh, I'm sure you're right. I'll amend it, thanks.

Just checked your force push diff, I think you'll want to keep the #[track_caller] annotation on both the inner and outer functions to get the location to propagate all the way in from user code.

[reez12g]

Thanks for checking.
You're right, I fixed it.

[yaahc]

Thanks for checking. You're right, I fixed it.

Awesome, looks perfect. Now we just need to add that UI test¹² then we should be good to go.

Footnotes

1. https://github.com/rust-lang/rust/pull/94236#issuecomment-1048369133 ↩

2. https://github.com/rust-lang/rust/issues/87707#issuecomment-1049341128 ↩

[reez12g]

I have started writing tests, but am a little stuck.

src/test/ui/issues/issue-87707.rs

// test for #87707
// edition:2018

use std::sync::Once;
use std::panic;

fn main() {
    let o = Once::new();
    let _ = panic::catch_unwind(|| {
        o.call_once(|| panic!("Here Once instance is poisoned."));
    });
    o.call_once(|| {});
}

That is, I am having trouble getting the standard error output to do what I expect.
It outputs in Playground, but when I run the test at hand, it does not output as shown below.

$ ./x.py test src/test/ui/issues/issue-87707.rs --bless --stage 1

~~~snip~~~

running 1 test
F
failures:

---- [ui] ui/issues/issue-87707.rs stdout ----

error: ui test compiled successfully!
status: exit status: 0
command: "/Users/reez12g/development/rust/build/aarch64-apple-darwin/stage1/bin/rustc" "/Users/reez12g/development/rust/src/test/ui/issues/issue-87707.rs" "-Zthreads=1" "--target=aarch64-apple-darwin" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/Users/reez12g/development/rust/build/aarch64-apple-darwin/test/ui/issues/issue-87707" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/Users/reez12g/development/rust/build/aarch64-apple-darwin/native/rust-test-helpers" "--edition=2018" "-L" "/Users/reez12g/development/rust/build/aarch64-apple-darwin/test/ui/issues/issue-87707/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------

------------------------------------------



failures:
    [ui] ui/issues/issue-87707.rs

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 12657 filtered out; finished in 1.42s

Some tests failed in compiletest suite=ui mode=ui host=aarch64-apple-darwin target=aarch64-apple-darwin
Build completed unsuccessfully in 0:00:08

Could you please tell me what I am doing wrong?
(Please point out if it is more appropriate to ask in zulip (#t-lib?))

[yaahc]

Could you please tell me what I am doing wrong?
(Please point out if it is more appropriate to ask in zulip (#t-lib?))

I think its best to ask in the issue but its also okay to ask in both places to try to get a response sooner. My only concern is that I don't want the discussion to get lost because it's split across platforms. To answer your question though, I'm happy to help debug the issue. Can you go ahead and push your copy of your test so I can take a look at it?

[reez12g]

I think its best to ask in the issue

I will do so next time.
I greatly appreciate it.

d71a4ab16496db4c4c8e3093299e261be8ac0ec9
↑ I pushed the current code.
My expectation is that this test code will output some kind of standard error, but the current situation is that it doesn't output anything when I run it at hand.

[yaahc]

d71a4ab ↑ I pushed the current code. My expectation is that this test code will output some kind of standard error, but the current situation is that it doesn't output anything when I run it at hand.

awesome ty, taking a look right now.

[yaahc]

Looks like you were just missing some header commands¹ to tell the testsuite to check the output of the test.

I went ahead and pushed the fix since I'd already done it locally when testing. Once that passes CI I'll go ahead and approve the PR. Thank you for the help! ^_^

Footnotes

1. https://rustc-dev-guide.rust-lang.org/tests/ui.html#controlling-passfail-expectations ↩

[yaahc]

@bors r+

[bors]

📌 Commit 9aaf9f172a28ccf479e9fbe36a45a1360d44f882 has been approved by yaahc

[Mark-Simulacrum]

Squashed commits down into one. I suspect the header commands may not be enough if the RUST_BACKTRACE env is set, though -- that will fail the llvm-12 builder in CI if so.

@bors r=yaahc rollup=iffy

[bors]

📌 Commit bca67fe has been approved by yaahc

[bors]

⌛ Testing commit bca67fe with merge 1b14fd3...

[reez12g]

Thanks for the review & the help!
I appreciate it.