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

STD Cleanup and Test #6

Closed
Meziu opened this issue Jan 30, 2022 · 51 comments
Closed

STD Cleanup and Test #6

Meziu opened this issue Jan 30, 2022 · 51 comments

Comments

@Meziu
Copy link
Owner

Meziu commented Jan 30, 2022

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:

  • alloc
  • any
  • array
  • ascii
  • borrow
  • boxed
  • cell
  • char
  • clone
  • cmp
  • collections
  • convert
  • default
  • error
  • f32
  • f64
  • ffi
  • fmt
  • hint
  • io
  • iter
  • marker
  • mem
  • num
  • ops
  • option
  • os (it's empty for our target)
  • panic
  • prelude
  • ptr
  • rc
  • result
  • slice
  • str
  • string
  • vec

TESTED AND NOW WORKING MODULES:

  • hash
  • future
  • pin
  • time
  • env
  • process
  • sync
  • thread

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

  • fs
  • path

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
Copy link

AzureMarker commented Jan 30, 2022

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
Copy link

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

@ian-h-chamberlain
Copy link

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
Copy link

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
Copy link
Owner Author

Meziu commented Jan 30, 2022

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

rust3ds/ctru-rs#25 Maybe we could do something with this, or Citra. Though I suggest on-hardware tests (as I also can't run Citra).

@Meziu
Copy link
Owner Author

Meziu commented Jan 30, 2022

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
Copy link

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
Copy link
Owner Author

Meziu commented Jan 30, 2022

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
Copy link

AzureMarker commented Jan 30, 2022

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
Copy link
Owner Author

Meziu commented Jan 30, 2022

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
Copy link

ian-h-chamberlain commented Jan 30, 2022

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
Copy link
Owner Author

Meziu commented Jan 30, 2022

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
Copy link
Owner Author

Meziu commented Jan 31, 2022

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 Meziu/libc@8744910 in case you also can't find anything.

@Meziu
Copy link
Owner Author

Meziu commented Jan 31, 2022

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
Copy link

ian-h-chamberlain commented Jan 31, 2022

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
Copy link
Owner Author

Meziu commented Jan 31, 2022

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
Copy link

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
Copy link
Owner Author

Meziu commented Jan 31, 2022

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
Copy link
Owner Author

Meziu commented Feb 2, 2022

Fixed HashMaps in bbaf4bf

@Meziu Meziu mentioned this issue Feb 10, 2022
@AzureMarker
Copy link

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
Copy link
Owner Author

Meziu commented Feb 12, 2022

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
Copy link

ian-h-chamberlain commented Feb 12, 2022

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
Copy link
Owner Author

Meziu commented Feb 12, 2022

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
Copy link
Owner Author

Meziu commented Feb 15, 2022

Priorities on the first STD PR:

  • Check out errors in any libc/pthread related code (this includes zombies ate my 3ds)
  • Test functionality of remaining modules (mainly fs and sync)
  • Close off unimplementable functionality (mainly process and env related functionality
  • 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)

@ian-h-chamberlain
Copy link

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
Copy link
Owner Author

Meziu commented Feb 16, 2022

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
Copy link
Owner Author

Meziu commented Mar 1, 2022

What’s left doing:

@AzureMarker @ian-h-chamberlain

@AzureMarker
Copy link

AzureMarker commented Mar 6, 2022

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
Copy link
Owner Author

Meziu commented Mar 6, 2022

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
Copy link

AzureMarker commented Mar 6, 2022

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 rust-random/getrandom#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
Copy link
Owner Author

Meziu commented Mar 6, 2022

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
Copy link

Opened rust-lang/libc#2714 and rust-lang/libc#2715

@AzureMarker
Copy link

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).

rust-random/getrandom#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
Copy link

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 #16 (comment) - I need to test again and can probably do so whenever I figure out or give up on the std testing.

@AzureMarker
Copy link

AzureMarker commented Mar 16, 2022

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 rust-lang#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
Copy link
Owner Author

Meziu commented Mar 16, 2022

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
Copy link

ian-h-chamberlain commented Mar 16, 2022

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

@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.

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
Copy link
Owner Author

Meziu commented Mar 16, 2022

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
Copy link
Owner Author

Meziu commented Mar 16, 2022

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
Copy link

(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
Copy link
Owner Author

Meziu commented Mar 17, 2022

(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
Copy link

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
Copy link

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
Copy link
Owner Author

Meziu commented Mar 18, 2022

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
Copy link

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
Copy link

@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
Copy link

AzureMarker commented Mar 24, 2022

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
Copy link
Owner Author

Meziu commented Apr 8, 2022

@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
Copy link

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
Copy link

Opened the std PR! rust-lang#95897

@AzureMarker
Copy link

I think with that, we can close this issue? We will still need to open the std::thread PR, but that's straightforward.

@Meziu Meziu closed this as completed Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants