Skip to content
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

Ensure mir_drops_elaborated_and_const_checked when requiring codegen. #110107

Merged
merged 3 commits into from
Apr 21, 2023

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Apr 9, 2023

mir_drops_elaborated_and_const_checked may emit errors while codegen has started, and the compiler would exit leaving object code files around.

Found by @cuviper in #109731

@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2023

r? @b-naber

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

@rustbot 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 Apr 9, 2023
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 9, 2023
@bors
Copy link
Contributor

bors commented Apr 9, 2023

⌛ Trying commit 34b763de3f046b64ed2fdb3562fbf7ca4be79349 with merge d58481d6971e2fcdda6e99ceab5b37c90c87536d...

@bors
Copy link
Contributor

bors commented Apr 9, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Apr 9, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d58481d6971e2fcdda6e99ceab5b37c90c87536d): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 may lead 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-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.8% [1.0%, 2.3%] 12
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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
Regressions ❌
(secondary)
2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) - - 0

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
Regressions ❌
(secondary)
1.8% [1.2%, 3.1%] 7
Improvements ✅
(primary)
-1.6% [-1.6%, -1.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.6% [-1.6%, -1.6%] 1

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 9, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Apr 10, 2023

Looks like we were already not running const prop lints on dead code? Or is it more complicated than that?

If you want to preserve the "not running on dead code" behaviour while avoiding creating object files, you can probably just add another abort-if-errors after the monomorphization collector.

@cjgillot
Copy link
Contributor Author

That's just it: we didn't run the lint on dead functions, and we should.

I think this PR is consistent with the goal stated in #108730, and can be adapted later once that PR is unblocked.

@b-naber
Copy link
Contributor

b-naber commented Apr 11, 2023

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned b-naber Apr 11, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Apr 17, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Apr 17, 2023

📌 Commit 34b763de3f046b64ed2fdb3562fbf7ca4be79349 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2023
@bors
Copy link
Contributor

bors commented Apr 17, 2023

⌛ Testing commit 34b763de3f046b64ed2fdb3562fbf7ca4be79349 with merge baa520c5a041ac4e29f403f375f9b075a1a07ef5...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 17, 2023

💔 Test failed - checks-actions

@bors
Copy link
Contributor

bors commented Apr 21, 2023

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

@oli-obk
Copy link
Contributor

oli-obk commented Apr 21, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2023

📌 Commit 7e214bf has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2023
@bors
Copy link
Contributor

bors commented Apr 21, 2023

⌛ Testing commit 7e214bf with merge fa4cc63...

@bors
Copy link
Contributor

bors commented Apr 21, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing fa4cc63 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 21, 2023
@bors bors merged commit fa4cc63 into rust-lang:master Apr 21, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 21, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fa4cc63): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [1.7%, 2.4%] 10
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.9% [-2.3%, -1.5%] 2
Improvements ✅
(secondary)
-2.7% [-3.4%, -2.2%] 3
All ❌✅ (primary) -1.9% [-2.3%, -1.5%] 2

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
Regressions ❌
(secondary)
1.9% [1.2%, 3.1%] 6
Improvements ✅
(primary)
-2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) -2.2% [-2.2%, -2.2%] 1

@cjgillot cjgillot deleted the const-prop-lint-junk branch April 22, 2023 09:07
@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Apr 25, 2023
@Mark-Simulacrum
Copy link
Member

#110107 (comment) seems like sufficient explanation for the regression, though parts of it may also be noise.

RalfJung pushed a commit to RalfJung/rust that referenced this pull request Apr 28, 2023
Remove a test that wasn't carrying its weight

See rust-lang#110107 (comment) for discussion.

TLDR: it keeps breaking out of unrelated reasons and the real thing to check (early alloc ids can be tracked without ICEing miri) is hard to check otherwise.
davidtwco added a commit to davidtwco/rust that referenced this pull request Jul 25, 2023
Unions cannot have unsized fields, and as such, layout computation for
unions asserts that each union field is sized (as this would normally
have halted compilation earlier).

However, if a generator ends up with an unsized local - a circumstance
in which an error will always have been emitted earlier, for example, if
attempting to dereference a `&str` - then the generator transform will
produce a union with an unsized field.

Since rust-lang#110107, later passes will be run, such as constant propagation,
and can attempt layout computation on the generator, which will result
in layout computation of `str` in the context of it being a field of a
union - and so the aforementioned assertion would cause an ICE.

It didn't seem appropriate to try and detect this case in the MIR body
and skip this specific pass; tainting the MIR body or delaying a bug
from the generator transform (or elsewhere) wouldn't prevent this either
(as neither would prevent the later pass from running); and tainting when
the deref of `&str` is reported, if that's possible, would unnecessarily
prevent potential other errors from being reported later in compilation,
and is very tailored to this specific case of getting a unsized type in
a generator.

Given that this circumstance can only happen when an error should have
already been reported, the correct fix appears to be just changing the
assert to a delayed bug. This will still assert if there is some
circumstance where this occurs and no error has been reported, but it
won't crash the compiler in this instance.

Signed-off-by: David Wood <david@davidtw.co>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 25, 2023
abi: unsized field in union - assert to delay bug

Fixes rust-lang#113279.

> Unions cannot have unsized fields, and as such, layout computation for
unions asserts that each union field is sized (as this would normally
have halted compilation earlier).
>
> However, if a generator ends up with an unsized local - a circumstance
in which an error will always have been emitted earlier, for example, if
attempting to dereference a `&str` - then the generator transform will
produce a union with an unsized field.
>
> Since rust-lang#110107, later passes will be run, such as constant propagation,
and can attempt layout computation on the generator, which will result
in layout computation of `str` in the context of it being a field of a
union - and so the aforementioned assertion would cause an ICE.
>
> It didn't seem appropriate to try and detect this case in the MIR body
and skip this specific pass; tainting the MIR body or delaying a bug
from the generator transform (or elsewhere) wouldn't prevent this either
(as neither would prevent the later pass from running); and tainting when
the deref of `&str` is reported, if that's possible, would unnecessarily
prevent potential other errors from being reported later in compilation,
and is very tailored to this specific case of getting a unsized type in
a generator.
>
> Given that this circumstance can only happen when an error should have
already been reported, the correct fix appears to be just changing the
assert to a delayed bug. This will still assert if there is some
circumstance where this occurs and no error has been reported, but it
won't crash the compiler in this instance.

While debugging this, I noticed a translation ICE in a delayed bug, so I fixed that too:

> During borrowck, the `MultiSpan` from a buffered diagnostic is cloned and
used to emit a delayed bug indicating a diagnostic was buffered - when
the buffered diagnostic is translated, then the cloned `MultiSpan` may
contain labels which can only render with the diagnostic's arguments, but
the delayed bug being emitted won't have those arguments. Adds a function
which clones `MultiSpan` without also cloning the contained labels, and
use this function when creating the buffered diagnostic delayed bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants