-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Fix target_os
for mipsel-sony-psx
#131168
base: master
Are you sure you want to change the base?
Conversation
r? @davidtwco rustbot has assigned @davidtwco. Use |
These commits modify compiler targets. |
Since PS1 doesn't support
Also the differences from the PSP target were intentional given what I saw the rust psp sdk does. I don't have time to get into it now, but I'll follow up with my rationale later today or tomorrow. |
No problem, you're much more involved in the details here than I am! I will say though, if we do not do this change, then the target should perhaps be renamed to I'll leave this PR open, both to allow the discussion to continue, and because I intend to document the final rationale in the code, but to prevent it from being merged, lets do: |
This comment has been minimized.
This comment has been minimized.
feels like a bootloader with some firmware, tbh: https://psx.arthus.net/sdk/Psy-Q/DOCS/Devrefs/os.pdf guess you could call that an exokernel? |
4d7cb84
to
619dcc0
Compare
Some changes occurred in tests/ui/check-cfg cc @Urgau |
This comment has been minimized.
This comment has been minimized.
Previously set to `target_os = "none"` and `target_env = "psx"`, but although the Playstation 1 is _close_ to a bare metal target in some regards, it's still very much an operating system, so we should set `target_os = "psx"`. This also matches the `mipsel-sony-psp` target, which sets `target_os = "psp"`.
619dcc0
to
111f2e8
Compare
So the line between operating system and bare metal is blurry here. Basically it does have a thing called a BIOS but it's more of a loader than useful for anything that an OS does. Specifically:
The one point for Imho since it doesn't use syscalls for most of the functions it provides and running BIOS/user code doesn't have appreciable differences in terms of what that code is allowed to do (especially for the code it relocates to RAM)
I'd really like to avoid that if possible, that kind of user-facing instability defeats the purpose of having the target in tree :/ |
fwiw I don't think we necessarily define an OS as "something you do syscalls to", as odd as that may sound. e.g. the hermit target is a unikernel (so the "OS" and application are all compiled as one thing!) but we put the name in
no strong opinion on whether that applies to this target, tho', just noting for reference. |
yeah unikernels are a good point @workingjubilee. I realized after my last comment that another counter-example is linux which is obviously an OS, but provides functions w/o syscalls through vdso. Either way defining what counts as an OS is fuzzy but if this change makes it easier to do the target parsing stuff for cc-rs then that's good enough for me @madsmtm. |
@rustbot ready |
Thanks a lot for explaining your reasoning! I've tried to add a quick comment summarizing it, though not too happy with how it turned out, if you want to review the wording here that'd be nice. Overall, I still have a preference for merging this PR, but ultimately, I'll leave it to the T-compiler reviewer to decide whether they want
I think we're gonna go a different approach, so doesn't matter too much for |
If you don't need this for anything you're doing (either using the target or not) I have a slight preference for keeping it the way it was since this case is fuzzy. |
I feel like we might want to get the relevant Nintendo and PlayStation people in a room and hash out how we want to handle talking about console compilation and runtime environments in the compiler at some point, but it doesn't seem best decided in this PR. |
I don't have a strong opinion on this because it's the Playstation 1 target, but I was chatting to @wesleywiser about this at EuroRust and we think that whether or not we set But like I said, it's the Playstation 1 target, so I have the weakest possible opinion on this, and if @ayrtonm is happy with this patch as the target maintainer, then I'm happy to r+ it. |
Previously set to
target_os = "none"
andtarget_env = "psx"
in the PR introducing the target, but although the Playstation 1 is close to a bare metal target in some regards, it's still very much an operating system, so we should instead settarget_os = "psx"
.This also matches the
mipsel-sony-psp
target, which setstarget_os = "psp"
.CC target maintainer @ayrtonm.
If there's any code out there that uses
cfg(target_env = "psx")
, they can usecfg(any(target_os = "psx", target_env = "psx"))
until they bump their MSRV to a version where this is fully fixed.