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.

Sign up for GitHub

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

Jump to bottom

crashes: more tests #133294

Merged
merged 1 commit into from
Dec 13, 2024
Merged

crashes: more tests #133294

merged 1 commit into from
Dec 13, 2024
+113 −0

Conversation

matthiaskrgr
Copy link
Member

@matthiaskrgr matthiaskrgr commented Nov 21, 2024 •
edited
Loading

r? @jieyouxu

try-job: aarch64-apple
try-job: x86_64-msvc

@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 Nov 21, 2024
jieyouxu
jieyouxu requested changes Nov 21, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have some questions.

tests/crashes/132765.rs Outdated Show resolved Hide resolved
tests/crashes/132766.rs Show resolved Hide resolved
tests/crashes/133065.rs Outdated
@@ -0,0 +1,15 @@
//@ known-bug: #133065
//@compile-flags: -Zvalidate-mir -Zinline-mir -Zinline-mir-threshold=300
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: hmm, this is also one of those cases where it's better to identify which mir opts are involved, like #133104. Probably split this off into another PR.

Copy link
Member Author

@matthiaskrgr matthiaskrgr Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I understand this only enables mir inlining.
does Zinline-mir also enable other opts than mir inlining?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @saethlin do you know? I'm not super familiar with the MIR-specific ones.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too convinced that this is too useful in general.
If we for example have a broken mir opt in the pipeline, and the mir opt gets banished to -Zunsave-mir-opts, this still fixes the bug from a user perspective as the mir opt is no longer run by default.

Also some mir opts rely on each other which is why I don't fuzz single mir opts, as there are probably bunch of cases where "one opt will break if that other opt is not run before it."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a crashes test we can also remove it easily if it becomes a problem, so yeah it's a whatever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does Zinline-mir also enable other opts than mir inlining?

-Zinline-mir is the same as -Zmir-enable-passes=+Inline, and it takes work to make sure those flags don't diverge in behavior.

there are probably bunch of cases where "one opt will break if that other opt is not run before it."

There aren't supposed to be any such cases.

If we for example have a broken mir opt in the pipeline, and the mir opt gets banished to -Zunsave-mir-opts, this still fixes the bug from a user perspective as the mir opt is no longer run by default.

The crashes tests aren't about tracking the quality of the user experience, they are about tracking the state of open issues. If an ICEing MIR opt is put behind a flag, the bug is still in the code.

tests/crashes/133066.rs Show resolved Hide resolved
tests/crashes/133199.rs Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2024
@jieyouxu
Copy link
Member

Thanks, r=me if the discussions indicate we can go ahead and add the relevant crashes tests.

jieyouxu
jieyouxu approved these changes Nov 21, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jieyouxu jieyouxu closed this Dec 2, 2024
@jieyouxu jieyouxu reopened this Dec 2, 2024
@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 2, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu
Copy link
Member

jieyouxu commented Dec 2, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 2, 2024
@matthiaskrgr
Copy link
Member Author

I've removed the ones that passed for now, will revisit these later
@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Dec 5, 2024

📌 Commit 98bae8a has been approved by jieyouxu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 6, 2024
@matthiaskrgr
Rollup merge of rust-lang#133294 - matthiaskrgr:crashes21nov, r=jieyouxu
e5c30ed 
crashes: more tests

r? `@jieyouxu`
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 6, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2024
@bors
Auto merge of rust-lang#133948 - matthiaskrgr:rollup-ifyq71o, r=matth…
2af3896 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#130777 (rust_for_linux: -Zreg-struct-return commandline flag for X86 (rust-lang#116973))
 - rust-lang#133211 (Extend Miri to correctly pass mutable pointers through FFI)
 - rust-lang#133294 (crashes: more tests)
 - rust-lang#133790 (Improve documentation for Vec::extend_from_within)
 - rust-lang#133930 (rustbook: update to use new mdbook-trpl package from The Book)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member Author

@bors r-

#133948 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 6, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Dec 6, 2024

Maybe arch-specific ABI?

jieyouxu
jieyouxu approved these changes Dec 8, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jieyouxu
Copy link
Member

jieyouxu commented Dec 8, 2024

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Dec 8, 2024

📌 Commit cd979c6 has been approved by jieyouxu

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 Dec 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2024
@bors
Auto merge of rust-lang#133294 - matthiaskrgr:crashes21nov, r=jieyouxu
a41be64 
crashes: more tests

r? `@jieyouxu`

try-job: aarch64-apple
@bors
Copy link
Contributor

bors commented Dec 8, 2024

⌛ Testing commit cd979c6 with merge a41be64...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Dec 8, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 8, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Dec 9, 2024

#132981 might be arch-specific?

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2024
@matthiaskrgr
Copy link
Member Author

matthiaskrgr commented Dec 12, 2024 •
edited
Loading

@bors try

ignore the test on windows entirely..

@bors
Copy link
Contributor

bors commented Dec 12, 2024

⌛ Trying commit 33e6be0 with merge 1f04caa...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 12, 2024
@bors
Auto merge of rust-lang#133294 - matthiaskrgr:crashes21nov, r=<try>
1f04caa 
crashes: more tests

r? `@jieyouxu`

try-job: aarch64-apple
try-job: x86_64-msvc
@bors
Copy link
Contributor

bors commented Dec 12, 2024

☀️ Try build successful - checks-actions
Build commit: 1f04caa (1f04caae623a7116d715216b4ec1b995d1b92235)

@jieyouxu
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 13, 2024

📌 Commit 33e6be0 has been approved by jieyouxu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2024
@bors
Copy link
Contributor

bors commented Dec 13, 2024

⌛ Testing commit 33e6be0 with merge 3da8bfb...

@bors
Copy link
Contributor

bors commented Dec 13, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 3da8bfb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2024
@bors bors merged commit 3da8bfb into rust-lang:master Dec 13, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 13, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3da8bfb): 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 (primary 3.6%, secondary 3.0%)

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)		 3.6% [2.2%, 5.7%] 3
Regressions ❌
(secondary)		 3.0% [3.0%, 3.0%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 3.6% [2.2%, 5.7%] 3

Cycles

Results (secondary -2.7%)

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)		 - - 0
Improvements ✅
(secondary)		 -2.7% [-2.8%, -2.6%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 771.968s -> 769.59s (-0.31%)
Artifact size: 331.01 MiB -> 331.01 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saethlin saethlin saethlin left review comments

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
7 participants
@matthiaskrgr @jieyouxu @rust-log-analyzer @bors @rust-timer @saethlin @rustbot