Meziu / rust-horizon

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
3 stars 1 forks source link

STD Cleanup and Test #6

Closed Meziu closed 2 years ago

Meziu commented 2 years ago

We have worked with most of the std while updating ctru-rs's modules. However, we now have to merge our changes into the rustlang tree, and this will require cleanup and a lot of testing. We must make sure to never have to make PR's to std ever again, since the effort for the contributor and the reviewer is great.

Since very basic/already tested multiple times, I'd skip testing of fundamental datatypes and of these modules:

TESTED AND NOW WORKING MODULES:

Modules we should test, though we have already worked with:

Other than testing, cleaning up lints and other problems is needed.

@AzureMarker @ian-h-chamberlain Please, a combined effort is the best and fastest way.

AzureMarker commented 2 years ago

I'm interested to see what kind of futures executor I can get running on the 3DS. Then again, there isn't much in the futures module that could be broken.

AzureMarker commented 2 years ago

Maybe there's a way to run the runtime std tests on-device?

ian-h-chamberlain commented 2 years ago

At one point I actually was able to build libc-test for the 3DS but ran into some issues when running it. I didn't spend much time to debug the issue, but based on that I believe it should be possible to build and run tests on the device (not as part of CI or anything, but perhaps with Citra there's something possible...)

I can start taking a look at cleaning up lints and warnings, I noticed that std does not seem to build using the default RUSTFLAGS in x.py, so I think there's some cleanup to do there, which should help make submitting upstream easier in the future.

AzureMarker commented 2 years ago

I noticed that std does not seem to build using the default RUSTFLAGS in x.py

x.py build -i library/std --stage 1 doesn't work for you?

Meziu commented 2 years ago

Maybe there's a way to run the runtime std tests on-device?

https://github.com/Meziu/ctru-rs/issues/25 Maybe we could do something with this, or Citra. Though I suggest on-hardware tests (as I also can't run Citra).

Meziu commented 2 years ago

I'm interested to see what kind of futures executor I can get running on the 3DS. Then again, there isn't much in the futures module that could be broken.

This was me before noticing HashMap are broken. Yes. A collection type (it's actually an issue with randoms).

ian-h-chamberlain commented 2 years ago

x.py build -i library/std --stage 1 doesn't work for you?

This was a while ago, so it might have been a combination of libc changes and my own setup, but IIRC there were some warnings which compiled (but warned) with -Zbuild-std but failed with x.py build library/std --target armv6k-nintendo-3ds because of -D warnings or something like that. I'll double check, right now I've been trying various x.py check, x.py clippy just to see what comes up. A bunch of test stuff doesn't pass the check due to the getrandom issue @Meziu mentioned, which looks like something we might be able to easily fix by adding some target_os = "horizon" in the getrandom crate.

Meziu commented 2 years ago

IIRC there were some warnings which compiled (but warned) with -Zbuild-std but failed with x.py build library/std --target armv6k-nintendo-3ds

I personally never built the std separately, and always built the stage 1 or 2 compiler.

A bunch of test stuff doesn't pass the check due to the getrandom issue @Meziu mentioned, which looks like something we might be able to easily fix by adding some target_os = "horizon" in the getrandom crate.

It's just a small change in sys/unix/rand.rs to use rand() instead of non-available syscalls.

AzureMarker commented 2 years ago

I haven't looked into it, but there's some randomness code in libctru used by ctru-rs which we might be able to use in the linker fix crate to supply a libc call?

Meziu commented 2 years ago

I haven't looked into it, but there's some randomness code in libctru used by ctru-rs which we might be able to use in the linker fix crate to supply a libc call?

Is there? I haven't found anything other than rand

ian-h-chamberlain commented 2 years ago

This API is the only thing I've found, but it includes this note at the top of the module:

This is normally not accessible to userland apps

So I'm not sure if we could use that.

BTW, I've found that searching with Google and site:libctru.devkitpro.org is a bit more effective than the builtin search on the generated documention

Edit: there are a few other APIs, sslcGenerateRandomData which is probably more for cryptographically secure randomness and I guess uses PS_GenerateRandomBytes internally. I guess the hosted docs haven't been updated to include these, since I can only find them in the headers on my system and not online, except for the wiki.

Meziu commented 2 years ago

We could implement a call to sslcGenerateRandomData into getrandom from Newlib, but that would require another libc declaration. We can make all the changes and once testing the std is over, we'll open a new PR to libc.

Meziu commented 2 years ago

Turns out time::Instant doesn't work because the impl uses libc::CLOCK_MONOTONIC, which is either wrong or nonexistent for our platform. I searched for a WHILE but didn't find anything about it. Resorted to https://github.com/Meziu/libc/commit/87449104dffa502276f5aac4a7fbc829c5f14cb9 in case you also can't find anything.

Meziu commented 2 years ago

image

Founded worries... Still, I wonder if using only the REALTIME clock is a good idea or we should go with something else.

ian-h-chamberlain commented 2 years ago

Looking at the implementation of osGetTime (which is used by gettimeofday, which in turn is used by our clock_gettime impl for CLOCK_REALTIME), they appear to do some adjustment for clock drift in an attempt to get monotonic values, but I'm not sure if that implies the value from svcGetSystemTick() is not normally monotonic? There is also an epoch adjustment from 1900 to 1970 here for gettimeofday, but I don't think that would be strictly needed for a monotonic clock_gettime...

So maybe we could try our own impl that basically looks like this?

let now = svcGetSystemTick();
// Not sure if the math is quite right but you get the idea...
(*tp).tv_sec = now / SYSCLOCK_ARM11;
(*tp).tv_nsec = 1000 * (now - SYSCLOCK_ARM11*(*tp).tv_sec) / CPU_TICKS_PER_USEC;

I have to imagine that svcGetSystemTick() is monotonic, since I think it's supposed to be the number of clock cycles since boot, based on this docstring

Meziu commented 2 years ago

Fantastic. If it's ticks since boot, it's by definition monotonic. Though now I wonder if we may have link problems (this is the first normally available function we are actively going to reimplement).

ian-h-chamberlain commented 2 years ago

first normally available function we are actively going to reimplement

I'm not sure I understand, wouldn't this just go into a different match arm of our existing clock_gettime implementation? Or does this need to replace some other newlib function, or a syscall or something?

Meziu commented 2 years ago

first normally available function we are actively going to reimplement

I'm not sure I understand, wouldn't this just go into a different match arm of our existing clock_gettime implementation? Or does this need to replace some other newlib function, or a syscall or something?

Yeah, no other syscall. We just need a CLOCK_MONOTONIC match

Meziu commented 2 years ago

Fixed HashMaps in https://github.com/Meziu/rust-horizon/commit/bbaf4bfbdb91c768e86a4bbd8222474279fe9709

AzureMarker commented 2 years ago

Based on https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/std.3A.3Athread.20support.20for.20armv6k-nintendo-3ds/near/271654826, I think we should consider making a PR with what we have (or split it up into more chunks) without the thread changes in #10, and then make PRs for that and other changes later.

Meziu commented 2 years ago

We can totally do that. But first we need a couple of things. 1) We have to make a PR to libc (this includes renaming the newly added pthread functions) and make sure we won’t need much of libc in the future. 2) Check if our repo passes CI checks. GitHub doesn’t let us run them, but I wonder if we can try them anyways via CLI.

@AzureMarker @ian-h-chamberlain

ian-h-chamberlain commented 2 years ago

Putting this out there... A lot of the stuff we'd like to test may work "well enough" if we can use the same mechanism as ctru::test_runner, and provide a custom test runner to run std's existing tests against, on the device. (probably an unmergeable PR due to the linking to ctru required, but perhaps useful nonetheless).

I don't imagine it would be easy, especially since std uses x.py test instead of the usual cargo test, but it may be worth pursuing in relation to this issue.

Meziu commented 2 years ago

Yeah we could try running std tests, that would simplify things a lot, but I have no idea where to start implementing it. These kinds of tests are usually done for the host target.

Meziu commented 2 years ago

Priorities on the first STD PR:

ian-h-chamberlain commented 2 years ago

Try running std tests. (This isn’t about making the testing stable or anything, but being able to try out the whole std systems in one go would be great)

I tried this a bit yesterday, but didn't get super far along. I have a patch to getrandom, which was required for building, and now am seeing a bunch of link errors as expected for stuff like the stubs in ctru::test_runner. Hoping to make some further progress on it later this week, to at least get an executable built.

Meziu commented 2 years ago

Try running std tests. (This isn’t about making the testing stable or anything, but being able to try out the whole std systems in one go would be great)

I tried this a bit yesterday, but didn't get super far along. I have a patch to getrandom, which was required for building, and now am seeing a bunch of link errors as expected for stuff like the stubs in ctru::test_runner. Hoping to make some further progress on it later this week, to at least get an executable built.

Honestly compiling an executable would be more than enough. Getting to run them even once would give us immense info on the status of our progress.

Meziu commented 2 years ago

What’s left doing:

@AzureMarker @ian-h-chamberlain

AzureMarker commented 2 years ago

I think both of those are nice-to-haves, especially the path one. We could start opening a PR to libc and eventually std in the meantime. We can open follow-up PRs as needed.

Meziu commented 2 years ago

I think both of those are nice-to-haves, especially the path one. We could start opening a PR to libc and eventually std in the meantime. We can open follow-up PRs as needed.

We need to squash some commits both in libc and std (though std commits also needs to be separated in two PR’s). I don’t have much time these days, if you could do that (in a new branch) I’ll open the PR to libc.

AzureMarker commented 2 years ago

I made two branches for libc. I can merge them into one, but I think the first might go in easier than the second (to unblock the getrandom PR quicker): horizon-getrandom-and-fixes - This contains fixes and the getrandom call by @ian-h-chamberlain, which unblocks https://github.com/rust-random/getrandom/pull/248. feature/horizon-pthread - This contains the changes needed for std threads support.

Feel free to push these to your repo and open the PR from there if you want. Or, I could handle the PR if you think you won't have time.

Meziu commented 2 years ago

Thank you! They look good already. You can open the PR’s yourself if you want it to go through faster, I don’t have my pc right now.

AzureMarker commented 2 years ago

Opened https://github.com/rust-lang/libc/pull/2714 and https://github.com/rust-lang/libc/pull/2715

AzureMarker commented 2 years ago

Well, that was pretty easy :). Both PRs are merged now and released in 0.2.120. We should update https://github.com/Meziu/libc to match the upstream (I definitely don't have permission to force-push master, so I'll leave that to @Meziu). We may want to make more changes later (I'm still eyeing tokio-console, which requires some more networking changes).

https://github.com/rust-random/getrandom/pull/248 is published as well now, which isn't needed for std but is well used in third party crates (ex. through rand).

ian-h-chamberlain commented 2 years ago

Little update about what I've been doing: I've made a bit of progress getting std tests to compile, but not 100% there yet (some linker issue with compiler_builtins, I've asked for help on Zulip meanwhile). I can try to open a draft PR to show the changes, but it's pretty hacky and I've had to use a lot of local patches to get past compilation issues.

Side note: I also found out there is a built-in way to run std tests on a remote device, which seems like it would be useful for us, but the implementation seems to rely on forking new processes to run the tests, which makes sense but probably makes it unusable for us. Maybe it could be hacked around to work without forking.

Re https://github.com/Meziu/rust-horizon/pull/16#issuecomment-1067531804 - I need to test again and can probably do so whenever I figure out or give up on the std testing.

AzureMarker commented 2 years ago

I rebased our commits on the latest rust-lang/rust changes, and split it into two branches:

This rebase also cleaned up the Cargo.lock situation (upgraded to libc 0.2.120 in the first commit) and tidied up the history slightly (removed some reverted commits, squashed/fixed-up a commit).

I can open a PR to rust-lang/rust soon for feature/horizon-std if that sounds good with everyone, @Meziu & @ian-h-chamberlain. There might still be some cleanups we want to do before that (ex. there might be something missing in MetadataExt's docs?). Edit: looking at the diff, there's definitely some small cleanups we can do.

Once that merges we can rebase the threads branch and make another PR. In the meantime, @Meziu you could reset your rust-horizon fork's horizon-std branch to feature/horizon-threads if you'd like (or we just reset to rust-lang's master when everything merges).

Note: when testing I did run into https://github.com/rust-lang/rust/issues/94983, which I worked around by setting static-libstdcpp and consequently download-ci-llvm in my config.toml to false (there's a warning if you only set the first field)... which meant I had to build LLVM :/.

Meziu commented 2 years ago

Wait, does libc 0.2.120 Have all of our changes? Our merges happened after their bump.

Edit: I also wonder how the automatic building process works for std. Can we ship a prebuilt std only needing to link symbols afterwards (with our polyfills)? Can a Tier 3 target even ship a prebuilt std?

ian-h-chamberlain commented 2 years ago

@AzureMarker - sounds good, I can help with cleanup if there is enough worth splitting the work.

@Meziu it looks like https://github.com/rust-lang/libc/pull/2725 did not make it in to 0.2.120 but everything else did, based on the commit order. So perhaps we will want to bump again and update before a PR to std.

As far as I know, tier 3 platforms do not have prebuilt std (that would be tier 2 i think based on https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-2 )

Also, should we add a documentation page for 3ds as in https://doc.rust-lang.org/nightly/rustc/platform-support/TEMPLATE.html ?

Seems like the policy to start adding these happened right around the time of rustc support for 3ds was added so it wasn't required in review, but with std coming in I think it would make sense. I can write that up if you'd like and put the three of us on as maintainers?

Meziu commented 2 years ago

Seems like the policy to start adding these happened right around the time of rustc support for 3ds was added so it wasn't required in review, but with std coming in I think it would make sense. I can write that up if you'd like and put the three of us on as maintainers?

I didn’t know about those. Yeah, we three can be maintainers (maybe I should put up a GitHub group now that I think about it). About specific info, like build information, specify the need of system libraries, and that we automatically link them via cargo-3ds, without getting too deep into devkitPRO-specific features (though mentioning the need), like tools and such. The toolchain changes a lot, and I still prefer caution about what we post on Rust official repositories. Nintendo has no mercy😂.

Meziu commented 2 years ago

Once that merges we can rebase the threads branch and make another PR. In the meantime, @Meziu you could reset your rust-horizon fork's horizon-std branch to feature/horizon-threads if you'd like (or we just reset to rust-lang's master when everything merges).

Let’s just wait for Rust to pull everything and then sync with them. That tree is screwed, thanks for the cleanup.

AzureMarker commented 2 years ago

(maybe I should put up a GitHub group now that I think about it)

There is the https://github.com/Rust3DS group, but we'd have to talk to them and see if they're still interested or are able to transfer ownership (we should let them know of our progress in any case).

Meziu commented 2 years ago

(maybe I should put up a GitHub group now that I think about it)

There is the https://github.com/Rust3DS group, but we'd have to talk to them and see if they're still interested or are able to transfer ownership (we should let them know of our progress in any case).

I can contact @FenrirWolf later (doesn't look active on GitHub).

FenrirWolf commented 2 years ago

Hiya folks, I'm still alive. Just haven't been very active in the 3DS space. I'll definitely have to look over everything you guys have been doing though!

I have some control over the Rust3DS github group, but iirc @HybridEidolon is the one who has full ownership and permissions over it.

FenrirWolf commented 2 years ago

Yeah, I played around with things and I don't have very many permissions in the org. Don't think I can even add people to it myself. But hopefully we can get Eidolon's help with that.

Also do you guys coordinate on discord or something, or has it mainly been through github issues?

Meziu commented 2 years ago

Also do you guys coordinate on discord or something, or has it mainly been through github issues?

We have only communicated via GitHub for now.

AzureMarker commented 2 years ago

Also do you guys coordinate on discord or something, or has it mainly been through github issues?

We have only communicated via GitHub for now.

We could enable GitHub Discussions on some of the repositories to make non-issue communication easier (I don't have that permission).

AzureMarker commented 2 years ago

@Meziu it looks like rust-lang/libc#2725 did not make it in to 0.2.120 but everything else did, based on the commit order. So perhaps we will want to bump again and update before a PR to std.

This is now in 0.2.121. I'll bump it in the rustc fork.

AzureMarker commented 2 years ago

I enabled Discussions on my rustc fork at least (where we have some branches staged for the upstream PRs), if anyone's interested: https://github.com/AzureMarker/rust-horizon/discussions

@FenrirWolf if you have any issues with getting the toolchain working, feel free to open a thread there.

Meziu commented 2 years ago

@HybridEidolon we’ve noticed people still stumble (most definitely because of google search results) in your rust3ds GitHub org, which is quite outdated. Are you willing to pass the ownership to us or make us contributors so we can port our changes to that repo and re-organize it?

AzureMarker commented 2 years ago

Thanks for the organization invites @HybridEidolon!

@Meziu @ian-h-chamberlain we should discuss how we should transfer and organize repos in the org. Let's talk here: https://github.com/orgs/rust3ds/teams/group-members/discussions/3

AzureMarker commented 2 years ago

Opened the std PR! https://github.com/rust-lang/rust/pull/95897