Replace set_last_error with set_last_error_and_return #3941

noahmbright · 2024-10-04T18:35:57Z

Took care of the simple patterns. Other patterns involved setting an error and then using write_int or setting metadata and returning -1. Unsure if those are in the scope of this change

Looks like this has conflicts with #3779, so I can update when how to handle that is decided.

Part of #3930.

oli-obk

Please revert the formatting changes unless they are from ./miri fmt (you may need to configure your vscode correctly with the settings from CONTRIBUTING.md)

Also use LibcError("foobar") everywhere while you're doing this change.

src/shims/unix/fs.rs

src/shims/unix/fd.rs

oli-obk · 2024-10-05T06:48:57Z

Ignore #3779, i'll rebase at some point

src/shims/unix/fd.rs

src/shims/unix/fs.rs

RalfJung · 2024-10-12T12:05:38Z

Not sure why the comments above have all been marked as resolved -- in the PR they are definitely still unresolved.

@rustbot author

noahmbright · 2024-10-12T12:28:49Z

Not sure why the comments above have all been marked as resolved -- in the PR they are definitely still unresolved.

Yes, sorry for the wait. How do you want to handle the InterpResult<'tcx, i64> vs . InterpResult<'tcx, Scalar> issue in my comment above? Once I fix that I'll push the update.

RalfJung · 2024-10-12T12:35:47Z

Which comment?

RalfJung · 2024-10-12T12:38:27Z

It's possible that you have an unsubmitted review, then only you can see those comments. They are marked with "pending". There should be a button somewhere to submit the review and make the comments visible to everyone.

src/shims/unix/fs.rs

src/shims/unix/fd.rs

noahmbright · 2024-10-18T20:41:50Z

Again sorry about the wait and the careless mistakes, but I added two commits, one a small formatting fix, and in the other I added set_last_error_and_return_i64 and fixed the type issues I introduced. Diff is smaller now too so hopefully this will be easier to re-review.

RalfJung · 2024-10-19T07:01:01Z

There are unfortunately conflicts with the master branch, could you take care of them with a rebase?
Then write @rustbot ready to initiate the next round of review.

noahmbright · 2024-10-19T12:51:42Z

There are unfortunately conflicts with the master branch, could you take care of them with a rebase? Then write @rustbot ready to initiate the next round of review.

Of course. All the conflicts are something like this:

<<<<<<< unix_shims
            return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);
=======
            this.set_last_error(ErrorKind::PermissionDenied)?;
            return interp_ok(Scalar::from_i32(-1));
>>>>>>> master

I can wrap interp_ok(this.set_last_error_and_return_*(e)) where these all occur, and we'll have an interp_ok(InterpResult<'tcx, Scalar>). Does that sound alright for the resolution?

RalfJung · 2024-10-19T13:07:08Z

return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied); looks like the right resolution for that example.

noahmbright · 2024-10-19T19:12:34Z

return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied); looks like the right resolution for that example.

I agree, after seeing the new changes while rebasing. @rustbot ready

RalfJung

Thanks, this all looks correct but there is one spot where we can make it a bit more compact. :)

Please also squash the commits (with git rebase --keep-base).

src/shims/unix/linux/epoll.rs

RalfJung · 2024-10-20T21:00:58Z

@rustbot author

Add `set_last_error_and_return_i64`, change calls from `this.libc_eval` to `LibcError`, and replace pairs of `set_last_error` and returning values of the right type with the new helper functions

noahmbright · 2024-10-21T09:34:18Z

Thanks, this all looks correct but there is one spot where we can make it a bit more compact. :)

Good catch. Thank you for the close reviews, this should have been much simpler. If I'm still welcome, I'll make sure my next PR goes more smoothly

RalfJung · 2024-10-21T10:12:31Z

Looks good, thanks for the PR!
@bors r+

Thank you for the close reviews, this should have been much simpler. If I'm still welcome, I'll make sure my next PR goes more smoothly

Sure thing, hickups happen. :) Just be careful and ask us rather than making guesses if you're not sure about something. :D When submitting a PR, it is a very good idea to list which things in the PR you are least sure about -- that greatly helps focus the review effort.

bors · 2024-10-21T10:12:35Z

📌 Commit ca5c26d has been approved by RalfJung

It is now in the queue for this repository.

Replace set_last_error with set_last_error_and_return Took care of the simple patterns. Other patterns involved setting an error and then using `write_int` or setting metadata and returning -1. Unsure if those are in the scope of this change Looks like this has conflicts with #3779, so I can update when how to handle that is decided. Part of #3930.

bors · 2024-10-21T10:13:46Z

⌛ Testing commit ca5c26d with merge 0a916c5...

bors · 2024-10-21T10:40:40Z

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

RalfJung · 2024-10-21T11:18:46Z

That's odd... @rust-lang/infra bors has a problem if we leave a "changes required" review, and then the changes are done? CI is green but the PR has not been pushed to master.

RalfJung

Let's see if this helps github.

RalfJung · 2024-10-21T11:19:43Z

@bors retry

bors · 2024-10-21T11:20:51Z

⌛ Testing commit ca5c26d with merge f026d57...

Replace set_last_error with set_last_error_and_return Took care of the simple patterns. Other patterns involved setting an error and then using `write_int` or setting metadata and returning -1. Unsure if those are in the scope of this change Looks like this has conflicts with #3779, so I can update when how to handle that is decided. Part of #3930.

Kobzol · 2024-10-21T11:20:52Z

Wtf, I never saw this error before.. Anyway, what do you think about switching miri from bors to merge queues? xD

RalfJung · 2024-10-21T11:20:57Z

@bors r+

bors · 2024-10-21T11:20:59Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2024-10-21T11:21:00Z

📌 Commit ca5c26d has been approved by RalfJung

It is now in the queue for this repository.

RalfJung · 2024-10-21T11:21:11Z

Wtf, I never saw this error before.. Anyway, what do you think about switching miri from bors to merge queues? xD

Yeah someone just needs to put in the work for that and I don't currently have the time.^^
Cc #3882

Replace set_last_error with set_last_error_and_return Took care of the simple patterns. Other patterns involved setting an error and then using `write_int` or setting metadata and returning -1. Unsure if those are in the scope of this change Looks like this has conflicts with #3779, so I can update when how to handle that is decided. Part of #3930.

bors · 2024-10-21T11:22:07Z

⌛ Testing commit ca5c26d with merge 652e860...

bors · 2024-10-21T11:47:57Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 652e860 to master...

bors · 2024-10-21T11:48:56Z

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes and 1 approving review by reviewers with write access.

RalfJung · 2024-10-21T11:51:08Z

@oli-obk I think we need to you to send an "approving" review or so, otherwise this PR can't land...

Is that really how github reviews work? Nobody else can approve a PR after a team member left a "changes required" review? That's a complete disaster.

oli-obk · 2024-10-21T13:57:20Z

you should be able to dismiss my review in the github ui (i approved it now, but for the future)

@bors r+

bors · 2024-10-21T13:57:23Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2024-10-21T13:57:24Z

📌 Commit ca5c26d has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-10-21T13:58:31Z

⌛ Testing commit ca5c26d with merge 0ebdb0c...

RalfJung · 2024-10-21T14:06:26Z

Where is the "dismiss review" button?

RalfJung · 2024-10-21T14:12:09Z

Ah I think I found it. That's quite well-hidden...

bors · 2024-10-21T14:24:23Z

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

bors · 2024-10-21T14:24:23Z

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

bors · 2024-10-21T14:25:23Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

oli-obk requested changes Oct 5, 2024

View reviewed changes

src/shims/unix/fs.rs Show resolved Hide resolved

src/shims/unix/fd.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 5, 2024

View reviewed changes

src/shims/unix/fd.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 5, 2024

View reviewed changes

src/shims/unix/fs.rs Outdated Show resolved Hide resolved

src/shims/unix/fs.rs Outdated Show resolved Hide resolved

src/shims/unix/fs.rs Outdated Show resolved Hide resolved

tiif reviewed Oct 12, 2024

View reviewed changes

src/shims/unix/fs.rs Outdated Show resolved Hide resolved

rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Oct 12, 2024

noahmbright commented Oct 12, 2024

View reviewed changes

src/shims/unix/fs.rs Outdated Show resolved Hide resolved

src/shims/unix/fd.rs Outdated Show resolved Hide resolved

noahmbright force-pushed the unix_shims branch from a8311c3 to 4c002e3 Compare October 19, 2024 18:39

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Oct 19, 2024

RalfJung reviewed Oct 20, 2024

View reviewed changes

src/shims/unix/linux/epoll.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Oct 20, 2024

Replace set_last_error with set_last_error_and_return_i*

ca5c26d

Add `set_last_error_and_return_i64`, change calls from `this.libc_eval` to `LibcError`, and replace pairs of `set_last_error` and returning values of the right type with the new helper functions

noahmbright force-pushed the unix_shims branch from 4c002e3 to ca5c26d Compare October 21, 2024 09:31

RalfJung approved these changes Oct 21, 2024

View reviewed changes

RalfJung closed this Oct 21, 2024

RalfJung reopened this Oct 21, 2024

oli-obk approved these changes Oct 21, 2024

View reviewed changes

RalfJung mentioned this pull request Oct 21, 2024

Consistently use io error handlers in Unix shims #3930

Closed

bors merged commit 0ebdb0c into rust-lang:master Oct 21, 2024
15 checks passed

Replace set_last_error with set_last_error_and_return #3941

Replace set_last_error with set_last_error_and_return #3941

Conversation

noahmbright commented Oct 4, 2024 • edited by RalfJung Loading

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk commented Oct 5, 2024 • edited Loading

RalfJung commented Oct 12, 2024

noahmbright commented Oct 12, 2024

RalfJung commented Oct 12, 2024

RalfJung commented Oct 12, 2024

noahmbright commented Oct 18, 2024

RalfJung commented Oct 19, 2024 • edited Loading

noahmbright commented Oct 19, 2024

RalfJung commented Oct 19, 2024

noahmbright commented Oct 19, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Oct 20, 2024

noahmbright commented Oct 21, 2024

RalfJung commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

RalfJung commented Oct 21, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Oct 21, 2024

bors commented Oct 21, 2024

Kobzol commented Oct 21, 2024

RalfJung commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

RalfJung commented Oct 21, 2024 • edited Loading

bors commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

RalfJung commented Oct 21, 2024

oli-obk commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

RalfJung commented Oct 21, 2024

RalfJung commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

noahmbright commented Oct 4, 2024 •

edited by RalfJung

Loading

oli-obk commented Oct 5, 2024 •

edited

Loading

RalfJung commented Oct 19, 2024 •

edited

Loading

RalfJung commented Oct 21, 2024 •

edited

Loading