-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Add std
support for armv7a-vex-v5
#145973
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb These commits modify the If this was unintentional then you should revert the changes before this PR is merged. These commits modify compiler targets. The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging. |
I think the limitations here seem fine, and from a quick skim nothing jumps out to me as problematic. Nominating to make sure the team doesn't have any concerns. @rustbot label +I-libs-nominated |
This comment has been minimized.
This comment has been minimized.
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
@rustbot ready |
Thanks for the updates here, I'll take a more thorough look through after the libs meeting (next Wednesday). |
☔ The latest upstream changes (presumably #146160) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/cargo cc @ehuss |
This comment has been minimized.
This comment has been minimized.
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
while let Some((out_byte, new_buf)) = buf.split_first_mut() { | ||
buf = new_buf; | ||
|
||
let byte = unsafe { vex_sdk::vexSerialReadChar(STDIO_CHANNEL) }; | ||
if byte < 0 { | ||
break; | ||
} | ||
|
||
*out_byte = byte as u8; | ||
count += 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified as for out_byte in buf
rather than using the split API and reassigning.
File { size: u64 }, | ||
} | ||
|
||
pub struct ReadDir(!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could reexport from unsupported
, and just move your comment from fn readdir
to the reexport for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't do that for reasons similar to the FilePermissions
situation:
ReadDir
must implIterator<Item = io::Result<DirEntry>>
.DirEntry
must returnFileAttr
/FileType
, which do have proper implementations in this case (though not for directories).
I tried this a while back and ran into this issue; I'm pretty sure the current code re-exports everything it possibly can from unsupported
at the moment. Maybe I could have DirEntry
just store !
rather than an actual path and just return that here? Not sure.
pub fn copy(from: &Path, to: &Path) -> io::Result<u64> { | ||
use crate::fs::File; | ||
|
||
let mut reader = File::open(from)?; | ||
let mut writer = File::create(to)?; | ||
|
||
io::copy(&mut reader, &mut writer) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an implementation in common
that does this, but also verifies it's a file and sets permissions. Would that work here?
rust/library/std/src/sys/fs/common.rs
Line 13 in 4056082
pub fn copy(from: &Path, to: &Path) -> io::Result<u64> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VEXos will return unsupported
for writer.set_permissions(perm)?;
(we can't support changing file permissions after a file has been opened), meaning the copy
function in common
will always fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Should the file vs. dir check be included, or will the reads handle failing if from
is a directory?
fn map_fresult(fresult: vex_sdk::FRESULT) -> io::Result<()> { | ||
// VEX uses a derivative of FatFs (Xilinx's xilffs library) for filesystem operations. | ||
match fresult { | ||
vex_sdk::FRESULT::FR_OK => Ok(()), | ||
vex_sdk::FRESULT::FR_DISK_ERR => Err(io::Error::new( | ||
io::ErrorKind::Uncategorized, | ||
"internal function reported an unrecoverable hard error", | ||
)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use io::const_error!
for errors known ahead of time if possible, saves the allocation. Also applies quite a few other places in this module.
if *create_new { | ||
if vex_sdk::vexFileStatus(path.as_ptr()) != 0 { | ||
return Err(io::Error::new( | ||
io::ErrorKind::AlreadyExists, | ||
"File exists", | ||
)); | ||
} | ||
} else if !*create { | ||
if vex_sdk::vexFileStatus(path.as_ptr()) == 0 { | ||
return Err(io::Error::new( | ||
io::ErrorKind::NotFound, | ||
"No such file or directory", | ||
)); | ||
} | ||
} | ||
|
||
vex_sdk::vexFileOpenWrite(path.as_ptr()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The status checks aren't required for the correctness of vexFileOpenWrite
are they? It's a convenience, but we probably shouldn't be doing this check: it's an extra syscall that the user may not need, plus a bit of TOCTU. If VEX doesn't have a more detailed error from the file APIs (or errno), it would be better to return a generic error and let the user do this check themselves (via fs::metadata
) if they need more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checks are required for correctness here. The create
/create_new
flags are kind of "smoke and mirrors" in this implementation. Since we don't have anything like an oflag
system or global errno, ensuring that files already exist or are created with create
/create_new
is verified by libstd rather than the OS itself. If those status checks weren't there, we could violate the contract requiring create_new
to disallow existing files of the same name or !create
requiring a file to already be there.
As for the overhead of vexFileStatus
- I think that it should be negligable here, since it would just be a simple lookup rather than a full syscall since most of the filesystem is implemented in "userspace" (though I'm cautious to use that word here, since the distinction is blurry on this target).
}; | ||
|
||
#[derive(Debug)] | ||
struct FileDesc(*mut vex_sdk::FIL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a real pointer to something that the OS will dereference? Or is it more like a Unix file descriptor (int
) / Windows handle where the OS does some validity checking and lookup? Would be worth a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a real pointer - there is no unix-like file descriptor table exposed to us. I believe it points to a FatFs file object structure under the hood. I'll add a comment, though the SDK intentionally keeps this type opaque (FIL
is just c_void
), so actually dereferencing it from libstd or treating it as anything other than opaque would be a bad idea since there's no stability guarantees regarding size/layout of FIL
in firmware.
#[derive(Clone, PartialEq, Eq, Debug)] | ||
pub struct FilePermissions {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly curious, why is an empty struct used here rather than unsupported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the sys api, File::perm
must infallibly return an instance of FilePermissions
. This is also why I can't re-export this type from unsupported
, because it stores !
due to the fact that FileAttr
is verified to be never-initialized as well (and therefore it can get away with just returning !
rather than the real type).
let len = buf.len() as _; | ||
let buf_ptr = buf.as_mut_ptr(); | ||
let read = unsafe { vex_sdk::vexFileRead(buf_ptr.cast(), 1, len, self.fd.0) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let len = buf.len() as _; | |
let buf_ptr = buf.as_mut_ptr(); | |
let read = unsafe { vex_sdk::vexFileRead(buf_ptr.cast(), 1, len, self.fd.0) }; | |
let len = buf.len() as u32; | |
let buf_ptr = buf.as_mut_ptr(); | |
let read = unsafe { vex_sdk::vexFileRead(buf_ptr.cast::<c_char>(), 1, len, self.fd.0) }; |
Just explicit for a bit more readability
let len = buf.len(); | ||
let buf_ptr = buf.as_ptr(); | ||
let written = | ||
unsafe { vex_sdk::vexFileWrite(buf_ptr.cast_mut().cast(), 1, len as _, self.fd.0) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
let len = buf.len(); | |
let buf_ptr = buf.as_ptr(); | |
let written = | |
unsafe { vex_sdk::vexFileWrite(buf_ptr.cast_mut().cast(), 1, len as _, self.fd.0) }; | |
let len = buf.len() as u32; | |
let buf_ptr = buf.as_ptr(); | |
let written = | |
unsafe { vex_sdk::vexFileWrite(buf_ptr.cast_mut().cast::<c_char>(), 1, len, self.fd.0) }; |
// `vexFileSeek` does not support seeking with negative offset, meaning | ||
// we have to calculate the offset from the end of the file ourselves. | ||
map_fresult(vex_sdk::vexFileSeek( | ||
self.fd.0, | ||
try_convert_offset(self.file_attr()?.size() as i64 + offset)?, | ||
SEEK_SET, | ||
))? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to seek to the end, tell, then seek to an offset from there? I'm speculating about the inner workings here but if vexFileSize
reads the size from disk, it may not be the same as the size as seen from the open (potentially unflushed) buffer.
This PR adds standard library support for the VEX V5 Brain (
armv7a-vex-v5
target). It is more-or-less an updated version of the library-side work done in #131530.This was a joint effort between me, @lewisfm, @max-niederman, @Gavin-Niederman and several other members of the
vexide
project.Background
VEXos is a fairly unconventional operating system, with user code running in a restricted enviornment with regards to I/O capabilities and whatnot. As such, several OS-dependent APIs are unsupported or have partial support (such as
std::net
,std::process
, and most ofstd::thread
). A more comprehensive list of what does or doesn't work is outlined in the updated target documentation. Despite these limitations, we believe thatlibstd
support on this target still has value to users, especially given the popular use of this hardware for educational purposes. For some previous discussion on this matter, see this comment.SDK Linkage
VEXos doesn't really ship with an official
libc
or POSIX-style platform API (and though it does port newlib, these are stubbed on top of the underlying SDK). Instead, VEX provides their own SDK for calling platform APIs. Their official SDK is kept proprietary (with public headers), though open-source implementations exist. Following the precedent of thearmv6k-nintendo-3ds
team's work in #95897, we've opted not to directly linklibstd
to any SDK with the expectation that users will provide their own - either throughvex-sdk-build
(which will download the official SDK from VEX),vex-sdk-jumptable
(a compatible, open-source reimplementation), or potentially through linking the PROS kernel. Thevex-sdk
crate used in the VEXos PAL provideslibc
-style FFI bindings for any compatible system library, so any of these options should work fine. A functional demo project usingvex-sdk-build
can be found here.Future Work
This PR implements virtually everything we are currently able to implement given the current capabilities of the platform. The exception to this is file directory enumeration, though the implementation of that is sufficiently gross enough to drive us away from supporting this officially.
Additionally, I have a working branch implementing the
panic_unwind
runtime for this target, which is something that would be nice to see in the future, though given the volume of compiler changes i've deemed it out-of-scope for this PR.