-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert 78373 ("dont leak return value after panic in drop") #81257
Revert 78373 ("dont leak return value after panic in drop") #81257
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
(shoot, was working on an out-of-date repo clone when I did revert. Rebasing now.) |
3f6eac0
to
c9c2fa8
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
c9c2fa8
to
d37cc61
Compare
@@ -78,6 +78,7 @@ | |||
100:12-100:20: @0[5]: _5 = (*((*_1).0: &bool))"> let mut countdown = 0;</span></span> | |||
<span class="line"><span class="code even" style="--layer: 1" title="99:29-99:30: @0[1]: _3 = const 0_i32 | |||
99:13-99:26: @0[2]: FakeRead(ForLet, _3) | |||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge has broken here. Coverage tests probably need blessing since there have been other changes to these tests since #78373
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, sorry about that, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(how did it pass CI with that in place? Do we not check this output as part of CI?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Ah I bet I need to set profiler = true
in my config.toml to get the relevant information here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this isn't run on the PR builders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if I need to do something else besides set profiler = true
? I don't see the test failure in my local build (running x.py test src/test/run-make-fulldeps
), and thus I expect blessing here won't help until that's resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my entire config.toml
changelog-seen = 2
[llvm]
assertions = true
[build]
build = "x86_64-pc-windows-msvc"
profiler = true
[install]
[rust]
debug = true
[target.x86_64-unknown-linux-gnu]
[dist]
The test does take over a minute to run though. I can try on linux if that would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. The profiler coverage tooling and file generation shouldn't be Windows specific, right? So weird that I don't see this.
I'll try to make my own setup more closely match yours, I guess. (Also, @wesleywiser said they'd help; I'm going to get in touch with them soon.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pnkfelix I checked out your PR locally and built with ./x.py test src/test/run-make-fulldeps/coverage
. The three coverage tests were ignored because I didn't have profiler = true
set in my config.toml
. After setting that, I reran the command, everything built and I got one failing test: run-make-fulldeps/coverage-spanview
. I don't think I have anything special configuration other than profiler = true
set.
I went ahead and blessed the test and pushed here in case you just want to cherry-pick that.
@matthewjasper am I right in inferring that you are otherwise okay with this PR, assuming the blessing contributed by @wesleywiser worked? |
Yes, r=me once coverage tests pass. |
55980b8
to
37dcdff
Compare
@bors r=matthewjasper |
📌 Commit 37dcdff319c831bbe9568816f5a0e85d1f28ca8a has been approved by |
(I still haven't managed to figure out why my local machine does not appear to invoke the coverage tests, but at this point I'm going to use Wesley's contribution and cross my fingers.) |
@bors p=10 (this is a candidate for beta-backport, and so I want to prioritize it over the other PR's I see in the queue that appear to be more cosmetic in nature.) |
hmm and I guess I also need to up the priority because it already needs a rebase after 3 hours in the queue |
37dcdff
to
4d59bb4
Compare
⌛ Testing commit 7c7f10b with merge dafe03c8f7621bca5fd1743e933dee235d2a5cad... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Pushed a fix for the tidy errors. @bors r=matthewjasper p=10 rollup=never |
📌 Commit dce5e9e has been approved by |
☀️ Test successful - checks-actions |
…ution-via-revert-of-pr-78373, r=matthewjasper Revert 78373 ("dont leak return value after panic in drop") Short term resolution for issue rust-lang#80949. Reopen rust-lang#47949 after this lands. (We plan to fine-tune PR rust-lang#78373 to not run into this problem.)
[beta] backports This backports: * CI: only copy python.exe to python3.exe if the latter does not exist rust-lang#81762 * Make hitting the recursion limit in projection non-fatal rust-lang#81055 * Remove incorrect `delay_span_bug` rust-lang#81532 * introduce future-compatibility warning for forbidden lint groups rust-lang#81556 * Update cargo rust-lang#81755 * rustdoc: Fix visibility of trait and impl items rust-lang#81288 * Work around missing -dev packages in solaris docker image. rust-lang#81229 * Update LayoutError/LayoutErr stability attributes rust-lang#81767 * Revert 78373 ("dont leak return value after panic in drop") rust-lang#81257 * Rename `panic_fmt` lint to `non_fmt_panic` rust-lang#81729
Short term resolution for issue #80949.
Reopen #47949 after this lands.
(We plan to fine-tune PR #78373 to not run into this problem.)