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

Improve UEFI stdio #117174

Merged
merged 3 commits into from
Feb 22, 2024
Merged

Improve UEFI stdio #117174

merged 3 commits into from
Feb 22, 2024

Conversation

Ayush1325
Copy link
Contributor

Fixed some things suggested in last PR: #116207

cc @dvdhrm
cc @nicholasbishop

@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2023

r? @joshtriplett

(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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 25, 2023
workingjubilee
workingjubilee reviewed Oct 30, 2023
View reviewed changes
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

r? workingjubilee

dvdhrm
dvdhrm suggested changes Oct 30, 2023
View reviewed changes
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

simple_text_input_read seems a little on the verbose side, but between that and it being named read while used in an implementation of Read::read, yeah, I'll take it.

@Ayush1325
Copy link
Contributor Author

@dvdhrm @workingjubilee Do you think read should ignore non-printable characters instead of returning an error?

@workingjubilee
Copy link
Member

I think Err in that case is correct: we lost the input, effectively. There's lots of ways to suppress that kind of error if the programmer wants.

dvdhrm
dvdhrm reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@dvdhrm dvdhrm left a comment

Choose a reason for hiding this comment

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

Thanks a lot for improving this! Some comments inline.

I really think we should suppress unknown scancodes. This is what TTYs on linux do, so I think we should follow.

library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

@rustbot author
There's at least one change we need to settle before we can ship this.

@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 22, 2023
@Ayush1325
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Nov 30, 2023
workingjubilee
workingjubilee reviewed Dec 3, 2023
View reviewed changes
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
library/std/src/sys/uefi/stdio.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jan 13, 2024

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

@Ayush1325
Improve UEFI stdio
07000dc 
- Do not drop any character while reading
- eabdf == Unsupported status
- loop untill read character or error encountered

Signed-off-by: Ayush Singh <[email protected]>
@Ayush1325
Use heap for stdout and stderr
346472c 
Signed-off-by: Ayush Singh <[email protected]>
@Ayush1325
Fixes from PR
1fbb00b 
- is_ebadf always returns false
- Allow reading partial characters to buffer
- Allow full UTF-16 in stdin

Signed-off-by: Ayush Singh <[email protected]>
@Amanieu
Copy link
Member

Amanieu commented Feb 21, 2024

ping from triage: @workingjubilee have all your review concerns been addressed?

@workingjubilee
Copy link
Member

Ah! Yes, it seems so!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2024

📌 Commit 1fbb00b has been approved by workingjubilee

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 Feb 22, 2024
@bors
Copy link
Contributor

bors commented Feb 22, 2024

⌛ Testing commit 1fbb00b with merge 026b3b8...

@bors
Copy link
Contributor

bors commented Feb 22, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 026b3b8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 22, 2024
@bors bors merged commit 026b3b8 into rust-lang:master Feb 22, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 22, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (026b3b8): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.7% [-0.7%, -0.7%] 1
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.5% [2.1%, 2.9%] 3
Improvements ✅
(primary)		 -2.1% [-2.1%, -2.1%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -2.1% [-2.1%, -2.1%] 1

Cycles

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

Binary size

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

Bootstrap: 649.414s -> 648.961s (-0.07%)
Artifact size: 310.96 MiB -> 310.94 MiB (-0.00%)

@Ayush1325 Ayush1325 deleted the uefi-stdio-improve branch February 22, 2024 10:11
@Ayush1325 Ayush1325 mentioned this pull request Jul 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdhrm dvdhrm dvdhrm requested changes

@workingjubilee workingjubilee Awaiting requested review from workingjubilee

Assignees

@workingjubilee workingjubilee

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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.
8 participants
@Ayush1325 @rustbot @workingjubilee @bors @Amanieu @rust-timer @dvdhrm @joshtriplett