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

Consistently use io error handlers in Unix shims #3930

Closed
RalfJung opened this issue Oct 1, 2024 · 2 comments · Fixed by #3990
Closed

Consistently use io error handlers in Unix shims #3930

RalfJung opened this issue Oct 1, 2024 · 2 comments · Fixed by #3990
Assignees
Labels
A-shims Area: This affects the external function shims A-unix Area: affects our shared Unix target support C-cleanup Category: cleaning up our code E-good-first-issue A good way to start contributing, mentoring is available

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 1, 2024

Unix shims return -1 and set the thread-local "last error" variable to indicate an error occurred. With #3923 and #3929, Miri has some nice helpers to help with that, but they are not being used everywhere yet. So this is about refactoring the existing code to use the new helpers.

To help, start by grepping for -1 and for this.eval_libc("E in the src/shims/unix folder. You will find a few patterns:

                    let efault = this.eval_libc("EFAULT");
                    this.set_last_error(efault)?;
                    this.write_int(-1, dest)?;

should become this.set_last_error_and_return(LibcError("EFAULT"), dest)?;.

            this.set_last_error(ErrorKind::PermissionDenied)?;
            return Ok(Scalar::from_i32(-1));

should become return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);.

Feel free to ask if you encounter other patterns you are unsure about. It's also okay for the first PR to only cover parts of this and leave the harder cases for a second round.

@RalfJung RalfJung added A-shims Area: This affects the external function shims A-unix Area: affects our shared Unix target support C-cleanup Category: cleaning up our code E-good-first-issue A good way to start contributing, mentoring is available labels Oct 1, 2024
@noahmbright
Copy link
Contributor

@rustbot claim

I'll make a PR after a first pass. I can add a second commit or just fixup depending on how that goes. Thanks for the instructions!

bors added a commit that referenced this issue Oct 21, 2024
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 added a commit that referenced this issue Oct 21, 2024
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 added a commit that referenced this issue Oct 21, 2024
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 added a commit that referenced this issue Oct 21, 2024
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.
@RalfJung
Copy link
Member Author

RalfJung commented Oct 21, 2024

After #3941, grepping for this.eval_libc("E still shows 36 hits. Most of these can probably be using set_last_error(LibcError(...)) instead, so that might be a good next step.

If this happens to be near a "return -1", it can use the helpers for that directly.

RalfJung pushed a commit to RalfJung/rust that referenced this issue Oct 27, 2024
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 rust-lang#3779, so I can update when how to handle that is decided.

Part of rust-lang/miri#3930.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims A-unix Area: affects our shared Unix target support C-cleanup Category: cleaning up our code E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants