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

Implement BeginPanic handling in const eval #16938

Merged
merged 6 commits into from Apr 21, 2024
Merged

Implement BeginPanic handling in const eval #16938

merged 6 commits into from Apr 21, 2024

Conversation

Nilstrieb
Copy link
Member

for #16935, needs some figuring out of how to write these tests correctly

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2024
@Nilstrieb Nilstrieb mentioned this pull request Mar 24, 2024
Merged
@Veykril
Copy link
Member

Veykril commented Mar 25, 2024

I pushed a fix to the minicore fixture which changes the failure of one of the tests

@Veykril
Copy link
Member

Veykril commented Mar 26, 2024

There were some more issues in the minicore definition (it was written with signatures in mind only so some functions were just lacking bodies etc), now both tests are running into execution limits (probably executing loop {}s but I am not sure why, I don't know hwo the current formatting infra works wrt to const eval)

Nilstrieb and others added 3 commits April 18, 2024 12:20
@Nilstrieb @Veykril
Handle panicking like rustc CTFE does
805f569 
Instead of using `core::fmt::format` to format panic messages, which may in turn
panic too and cause recursive panics and other messy things, redirect
`panic_fmt` to `const_panic_fmt` like CTFE, which in turn goes to
`panic_display` and does the things normally. See the tests for the full
call stack.
@Veykril
Fix missing function body in minicore
563bb6b
@Veykril
Fixup some issues with minicore
5df690e
@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

Found another issue, now we actually run into trying to interpret BeginPanic which I don't think is implemented? I don't know

@Veykril Veykril 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 Apr 18, 2024
@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

Found another issue, now we actually run into trying to interpret BeginPanic which I don't think is implemented? I don't know
Panic with message:
""
@HKalbasi is that the case? Do we need to implement the BeginPanic interpretation now or is that error deliberate as to that we should never hit it? I am not quite sure on this...

@HKalbasi
Copy link
Member

Previously, BeginPanic hit only in panics with custom payload. Now that we hook one layer below PanicFmt, I think we should try to extract the panic message if the underlying panic payload type is &str or String.

@Veykril Veykril marked this pull request as ready for review April 19, 2024 11:18
@Veykril Veykril force-pushed the dont-panic-tests branch 2 times, most recently from bb1acb4 to 4426142 Compare April 19, 2024 11:30
@Veykril Veykril changed the title [WIP] Extra tests for panics in const eval Implement BeginPanic handling in const eval Apr 19, 2024
@Veykril
Copy link
Member

Veykril commented Apr 19, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

📌 Commit 4426142 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

⌛ Testing commit 4426142 with merge bd5c08a...

bors added a commit that referenced this pull request Apr 19, 2024
@bors
Auto merge of #16938 - Nilstrieb:dont-panic-tests, r=Veykril
bd5c08a 
Implement `BeginPanic` handling in const eval

for #16935, needs some figuring out of how to write these tests correctly
@bors
Copy link
Collaborator

bors commented Apr 19, 2024

💔 Test failed - checks-actions

@Veykril Veykril force-pushed the dont-panic-tests branch 2 times, most recently from 598a053 to 7f3b84b Compare April 19, 2024 13:10
@Veykril
Copy link
Member

Veykril commented Apr 19, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

📌 Commit 7f3b84b has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

⌛ Testing commit 7f3b84b with merge 81a62d5...

bors added a commit that referenced this pull request Apr 19, 2024
@bors
Auto merge of #16938 - Nilstrieb:dont-panic-tests, r=Veykril
81a62d5 
Implement `BeginPanic` handling in const eval

for #16935, needs some figuring out of how to write these tests correctly
@bors
Copy link
Collaborator

bors commented Apr 19, 2024

💔 Test failed - checks-actions

@Veykril
Copy link
Member

Veykril commented Apr 21, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 21, 2024

📌 Commit 3b9a2af has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 21, 2024

⌛ Testing commit 3b9a2af with merge 4c08e2d...

@bors
Copy link
Collaborator

bors commented Apr 21, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 4c08e2d to master...

@bors bors merged commit 4c08e2d into rust-lang:master Apr 21, 2024
11 checks passed
@Nilstrieb Nilstrieb deleted the dont-panic-tests branch April 21, 2024 16:41
@Nilstrieb
Copy link
Member Author

thanks for continuing this, i completely forgot about it 💀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Nilstrieb @Veykril @HKalbasi @bors @rustbot