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

Add a thumbv4t-none-eabi target #74419

Merged
merged 12 commits into from
Jul 19, 2020
Merged

Add a thumbv4t-none-eabi target #74419

merged 12 commits into from
Jul 19, 2020

Conversation

Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Jul 16, 2020

(cc @ketsuban, one of the few other Rust users who programs for GBA.)


EDIT: This is now a more general thumbv4t-none-eabi PR! See this comment


Now that the PSP officially has an official target within Rust, well as the lead of the gba crate I can't not add a GBA target as well.

I know that the target tier policy isn't ratified and official, but I'll use it as an outline (cc @joshtriplett):

  • Designated Developer: Lokathor
  • Naming consistent with any existing targets
  • Doesn't create Rust project legal issues.
  • No license issues
  • Uses the standard Apache/mit license.
  • Rust tooling users don't have to accept any new licensing requirements
  • Does not support hosting rust tooling.
  • Doesn't require linking in proprietary code to obtain a functional binary. However, you will need to do some post-build steps to turn the ELF file into a usable GBA ROM (either for an emulator or for the actual hardware).
  • This is a no_std environment, without even a standard global allocator, so this adds no new code to alloc or std.
  • The process of building for this target is documented in the gba crate (link). Well, the docs there are currently a little out of date, they're back on using cargo-xbuild, but the crate docs there will get updated once this target is available.
  • This places no new burden on any other targets
  • Does not break any existing targets.

I'm not fully confident in specifying the same linker script for all possible projects, so I'm currently just not giving a linker script at all, and users can continue to select their own linker script by using -C to provide a linker arg.

I added the file, and added it to the supported_targets! macro usage, and I think that's all there is to do.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2020
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Cool! Left a few remarks/questions.

@@ -676,6 +676,7 @@ supported_targets! {
("powerpc64-wrs-vxworks", powerpc64_wrs_vxworks),

("mipsel-sony-psp", mipsel_sony_psp),
("thumbv4t-nintendo-gba", thumbv4t_nintendo_gba),
Copy link
Contributor

Choose a reason for hiding this comment

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

The CPU name is a bit odd as the trailing T means thumb support, but I guess this makes sense. You could run ARM instructions on the GBA though, right?

We do support one other ARMv4 target, armv4t-unknown-linux-gnueabi, but I believe that defaults to ARM instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah both a32 and t32 are supported, and the system boots in a32 state, but we want as much of the code as possible to be compiled using t32, so we're defaulting to t32. With the instruction set RFC it'll be a lot easier to mix the two, but we'll still want the default to be t32 for any untagged functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh but I see what you mean with the trailing t since the name already starts with thumb.

i guess it could just be thumbv4-nintendo-gba

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I remember now!! it's called thumbv4t-nintendo-gba because the LLVM target string that LLVM uses as the "basis" for this target profile is thumbv4t-none-eabi, and so if they put "thumbv4t" then we should kinda just follow suit.

I remember that somehow I was able to run a command line thing and have LLVM or rustc print a list of all the "known" target strings, but I can't remember how to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean rustc --print target-list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorta like that, but I thought that I'd seen a similar command called on LLVM directly. I could be imagining that part.

src/librustc_target/spec/thumbv4t_nintendo_gba.rs Outdated Show resolved Hide resolved
src/librustc_target/spec/thumbv4t_nintendo_gba.rs Outdated Show resolved Hide resolved
src/librustc_target/spec/thumbv4t_nintendo_gba.rs Outdated Show resolved Hide resolved
src/librustc_target/spec/thumbv4t_nintendo_gba.rs Outdated Show resolved Hide resolved
src/librustc_target/spec/thumbv4t_nintendo_gba.rs Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor

I won't be online in the short term, can you take over r? @jonas-schievink (or delegate to someone else)?

@joshtriplett joshtriplett added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 17, 2020
@ketsuban
Copy link
Contributor

I'd like to standardise on using Nintendo's three-letter names for their own products, so this target would be -nintendo-agb. Code can make use of Rust's existing functionality for conditional compilation if it wants to support (say) both the GBA and the DS (which have similar guts due to backwards compatibility).

@Lokathor
Copy link
Contributor Author

This is one part where I have to disagree.

  • Basically everyone in the world outside of Nintendo just calls the device the GBA, rather than using the technical corporate designation of AGB.
  • As just one data point, GBATEK (the main source of GBA info for homebrew programmers, for those who don't know) uses the designation AGB 34 times and the designation GBA 1,224 times.
  • Similarly, the Awesome GBADev page (Which collects GBA programming resources across the several languages that can target the GBA), and essentially everyone who I've ever spoken to in their linked discord, also uses GBA to refer to the device family in all standard conversation.

So I think that we should be using the far more common term GBA, especially since the alternative is so similar that it can at first be thought of as being just a weird misspelling of "gba".

Particularly, regardless of the spelling selected here, users will be able to use conditional compilation to share MMIO definitions across more than one target if they set things up properly.

@leo60228
Copy link
Contributor

I'm planning on upstreaming a similar Switch homebrew target, and have been using aarch64-none-horizon-libnx, Horizon being the Switch's operating system. One reason for this is that there's a WIP Rust clone of the OS (Sunrise) targeting i386.

Note that the 3DS uses Horizon as well, but has nearly no backwards compatibility. My thought is that if Nintendo has an internal fork of Rust (I've heard that they might?) then they could use all(target_arch = "aarch64", target_os = "horizon") for things that apply to both homebrew and their SDK, while upstream could use target_env = "libnx" for stuff that only applies to homebrew.

@Lokathor
Copy link
Contributor Author

Lokathor commented Jul 17, 2020

@leo60228 I can't tell if that means you favor the common name for the env or the manufacturer name for the env, or some third option entirely.

EDIT: though I don't think Nintendo is going to be putting out any new GBA games either way ;P

@leo60228
Copy link
Contributor

I favor using, well, the OS name. However, unlike the Switch, the GBA doesn't have an OS, so...

@Lokathor
Copy link
Contributor Author

Well there's 4 strings:

  • target_arch is clearly "arm"
  • target_vendor is certainly "nintendo"
  • target_os is expected to be "none" for embedded systems like this, so that's fairly set.
  • target_env is "???"

If we want to have a way to separate the GBA, DS, and DSi targets (and they do share a lot of MMIO declarations) then we must use a different target_env value for each of them. The question just becomes what string convention to use.

From there, the name of the overall target is just gluing together parts of the various target_foo strings.

@Lokathor
Copy link
Contributor Author

Lokathor commented Jul 17, 2020

(Note: I've asked on Discord for some of the other homebrew folks from other consoles (DS, GameCube, Wii, N64) to come weigh in, since they also will assumedly want targets at some point, so perhaps we can find a consensus with enough eyeballs on the situation.)

@Daxorinator
Copy link

Hi, I'm one of the developers of Grand Star, a set of toolchains and libraries to target the Nintendo Wii.

I agree with Lokathor's proposed naming scheme of thumbv4t-nintendo-gba (full: thumbv4t-nintendo-gba-none), I think this would suit to effectively convey the architecture, vendor and the system name (in place of OS, specifically for consoles / other no-OS devices where we can't maintain the usual consistency), further examples include: powerpc-nintendo-wii (full: powerpc-nintendo-wii-newlib) and mipsel-sony-psp (full: mipsel-sony-psp-newlib), thumbv(?)t-nintendo-ds (full: thumbv(?)t-nintendo-ds-newlib), etc. etc.
[Sorry - all of these use newlib and I couldn't find the thumb version for the DS!]

I would have to strongly disagree with a proposed triples of: powerpc-nintendo-revolution, powerpc-nintendo-rvl, or powerpc-nintendo-agb` as I find these to all be highly confusing and ineffectively convey the actual target machine's name, instead showing off the codenames of the machines. Also: Not all consoles / embedded devices have a three letter codename.

I'm unsure on the currently used aarch64-none-horizon-libnx for Switch, I'd prefer if the Vendor was nintendo or whoever the manufacturer is in general. Horizon does make sense as that is the name of the OS however there is no mention of the target being for the Switch in this triple. As well as this I'm unsure why libnx is set as the environment name, I do understand that's the main library being used however it doesn't adhere to the usual "put a libc name here", if this naming scheme was used, Wii would have to use powerpc-nintendo-wii-luma or powerpc-nintendo-wii-libogc.

Further - whilst I would love to use lld and other tools that ship with Rust, as Lokathor has said many others in the community use devkitPPC or devkitARM, etc. all of which are based on GNU Binutils. Whilst it seems from our research with the Wii that using LLVM tools such as lld maybe be possible, it seems that it would be some pain getting that to work at this point in time.

Great job with this @Lokathor and @ketsuban on the GBA target and also the PSP Community for the PSP target.
I hope this PR makes it in and we get an official target for GBA as I believe this kind of work is really laying the foundations for the future of console and embedded development with Rust. Hopefully someday we'll see a Wii target too :D

@ketsuban
Copy link
Contributor

ketsuban commented Jul 17, 2020

I think that we should be using the far more common term GBA, especially since the alternative is so similar that it can at first be thought of as being just a weird misspelling of "gba".

This is the case for this particular target, but other codenames are more esoteric - "Nitro" (NTR) for the Nintendo DS, for example. (That said, the GBA SP and GBA Micro have different codenames to the original Game Boy Advance - AGS and OXY - but would almost certainly not need separate targets since they're architecturally extremely similar.)

Edit: Alright, I'm convinced on using gba for the target env rather than agb. We can use nds and dsi for those targets, since that's what they're both known as to the general public.

@leo60228
Copy link
Contributor

leo60228 commented Jul 17, 2020 via email

@Lokathor
Copy link
Contributor Author

Nintendo certainly uses LLVM, and at least one 3rd party has ported Rust to Switch before. However, I am not aware of Nintendo using Rust directly. In fact last year there was a person in the gamedev-wg discord who needed help trying to build a rust project for Switch using the official SDK, so unless it was added extremely recently I don't think Nintendo uses Rust.

@parasyte
Copy link

parasyte commented Jul 17, 2020

IMHO, target env name should not use Nintendo's model code designation. At least not by convention.

If we look at all of Nintendo's model codes, we can see that most of them do not make sense to casual users:

Model Code Common Name Meaning or Codename
HVC Famicom Home Video Computer
NES Nintendo Nintendo Entertainment System
DMG Game Boy Dot Matrix Game
SHVC Super Famicom Super Home Video Computer
SNS Super Nintendo Super Nintendo System
MGB Game Boy Pocket Mini Game Boy?
CGB Game Boy Color Color Game Boy
VUE Virtual Boy Virtual Utopia Experience?
NUS Nintendo 64 Nintendo Ultra Sixty-four
AGB Game Boy Advance Advanced Game Boy
AGS Game Boy Advance SP Advanced Game Boy Special?
OXY Game Boy Micro Oxytocin?
DOL GameCube Dolphin
NTR Nintendo DS Nitro
USG DS Lite Usugata?
RVL Wii Revolution
TWL DSi Twelve?
UTL DSi XL/LL ???
WUP Wii U Wii U Player?
CTR 3DS Control? Citrus?
SPR 3DS XL/LL ???
KTR New 3DS ???
RED New 3DS XL/LL ???
FTR 2DS ???
JAN New 2DS XL/LL ???
CVL Classic Mini Consoles ???
NVL Amiibo ???
HAC Switch Handheld And Console?
HDH Switch Lite ???

Sources:

Even within this list, there is hardly any consistency; Super Famicom has a 4-letter code! There are three variations of Game Boy Advance hardware that all run the same exact games with no internal hardware differences. But each have their own unique model codes. I don't think it would be fair for Rust to use agb for the target env but not the equally relevant ags or oxy model codes.

Finally, there is also the importance of meaning. While agb is relatively acceptable for referring to Game Boy Advance, nus is unacceptable for Nintendo 64 (FWIW I authored cargo-n64). This console is casually known as N64, and maybe rarely "Ultra 64" when referring specifically to early development at SGI, but never "NUS".

And then there is the insane 3DS family of devices with no less than six model variations. And just one model code for all of the following devices:

  • NES Classic Edition
  • Nintendo Classic Mini: Family Computer
  • Nintendo Classic Mini: Nintendo Entertainment System
  • SNES Classic Edition
  • Nintendo Classic Mini: Super Famicom
  • Nintendo Classic Mini: Super Nintendo Entertainment System

KISS, I think just using the common name makes the most sense. Or the common abbreviation, which is probably better. I don't even need to provide descriptions for any of these, since they are self-evident:

  • gba
  • n64
  • 3ds
  • switch

@joshtriplett
Copy link
Member

joshtriplett commented Jul 17, 2020 via email

@leo60228
Copy link
Contributor

leo60228 commented Jul 17, 2020 via email

@joshtriplett
Copy link
Member

joshtriplett commented Jul 17, 2020 via email

@Lokathor
Copy link
Contributor Author

Folks are free to read the Zulip topic themselves but the take-away is that:

  • At least for GBA, the utility of a GBA-specific target compared to just a general thumbv4t-none-eabi target is minimal.
  • The only real use is for setting the target_env="gba" for cfg purposes, for extra MMIO safety in the gba crate. If you were to run the MMIO stuff on some other thumbv4t-none-eabi device it would obviously be wrong and UB.
  • We can just have the user take an opt-in step when building the gba crate to actually get the MMIO usage. They set --cfg gba in their rust flags, or the crate requires they turn on a feature named i_absolutely_promise_the_program_will_only_be_run_on_a_gba, or something else along those lines.

So I will update the PR to become a general thumbv4t-none-eabi target. I hope to have the changes done "within a few hours".

@Lokathor Lokathor changed the title Add a Game Boy Advance target Add a ~~Game Boy Advance~~ thumbv4t-none-eabi target Jul 18, 2020
@Lokathor Lokathor changed the title Add a ~~Game Boy Advance~~ thumbv4t-none-eabi target Add a thumbv4t-none-eabi target Jul 18, 2020
@jonas-schievink
Copy link
Contributor

Looks pretty good! Can we make this inherit from thumb_base.rs now?

@jonas-schievink
Copy link
Contributor

Looks good, I'll just approve as-is now. If there are any remaining issues they can be fixed later.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2020

📌 Commit ec9c8d8 has been approved by jonas-schievink

@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 Jul 19, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2020
…arth

Rollup of 4 pull requests

Successful merges:

 - rust-lang#74333 (Deny unsafe operations in unsafe functions in libstd/alloc.rs)
 - rust-lang#74356 (Remove combine function)
 - rust-lang#74419 (Add a thumbv4t-none-eabi target)
 - rust-lang#74485 (More intra-doc links, add explicit exception list to linkchecker)

Failed merges:

 - rust-lang#74486 (Improve Read::read_exact documentation)

r? @ghost
@bors bors closed this in 7cbff84 Jul 19, 2020
@bors bors merged commit 9016458 into rust-lang:master Jul 19, 2020
@Daxorinator
Copy link

Hurray!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 13, 2020
Pkgsrc changes:
 * Remove patches now integrated upstream, many related to SunOS / Illumos.
 * The LLVM fix for powerpc is also now integrated upstream.
 * Adapt those patches where the source has moved or parts are integrated.
 * The randomness patches no longer applies, and I could not find
   where those files went...
 * Provide a separate bootstrap for NetBSD/powerpc 9.0, since apparently
   the C++ ABI is different from 8.0.  Yes, this appears to be specific to
   the NetBSD powerpc ports.

Upstream changes:

Version 1.47.0 (2020-10-08)
==========================

Language
--------
- [Closures will now warn when not used.][74869]

Compiler
--------
- [Stabilized the `-C control-flow-guard` codegen option][73893], which enables
  [Control Flow Guard][1.47.0-cfg] for Windows platforms, and is ignored on
  other platforms.
- [Upgraded to LLVM 11.][73526]
- [Added tier 3\* support for the `thumbv4t-none-eabi` target.][74419]
- [Upgrade the FreeBSD toolchain to version 11.4][75204]
- [`RUST_BACKTRACE`'s output is now more compact.][75048]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`CStr` now implements `Index<RangeFrom<usize>>`.][74021]
- [Traits in `std`/`core` are now implemented for arrays of any length, not just
  those of length less than 33.][74060]
- [`ops::RangeFull` and `ops::Range` now implement Default.][73197]
- [`panic::Location` now implements `Copy`, `Clone`, `Eq`, `Hash`, `Ord`,
  `PartialEq`, and `PartialOrd`.][73583]

Stabilized APIs
---------------
- [`Ident::new_raw`]
- [`Range::is_empty`]
- [`RangeInclusive::is_empty`]
- [`Result::as_deref`]
- [`Result::as_deref_mut`]
- [`Vec::leak`]
- [`pointer::offset_from`]
- [`f32::TAU`]
- [`f64::TAU`]

The following previously stable APIs have now been made const.

- [The `new` method for all `NonZero` integers.][73858]
- [The `checked_add`,`checked_sub`,`checked_mul`,`checked_neg`, `checked_shl`,
  `checked_shr`, `saturating_add`, `saturating_sub`, and `saturating_mul`
  methods for all integers.][73858]
- [The `checked_abs`, `saturating_abs`, `saturating_neg`, and `signum`  for all
  signed integers.][73858]
- [The `is_ascii_alphabetic`, `is_ascii_uppercase`, `is_ascii_lowercase`,
  `is_ascii_alphanumeric`, `is_ascii_digit`, `is_ascii_hexdigit`,
  `is_ascii_punctuation`, `is_ascii_graphic`, `is_ascii_whitespace`, and
  `is_ascii_control` methods for `char` and `u8`.][73858]

Cargo
-----
- [`build-dependencies` are now built with opt-level 0 by default.][cargo/8500]
  You can override this by setting the following in your `Cargo.toml`.
  ```toml
  [profile.release.build-override]
  opt-level = 3
  ```
- [`cargo-help` will now display man pages for commands rather just the
  `--help` text.][cargo/8456]
- [`cargo-metadata` now emits a `test` field indicating if a target has
  tests enabled.][cargo/8478]
- [`workspace.default-members` now respects `workspace.exclude`.][cargo/8485]
- [`cargo-publish` will now use an alternative registry by default if it's the
  only registry specified in `package.publish`.][cargo/8571]

Misc
----
- [Added a help button beside Rustdoc's searchbar that explains rustdoc's
  type based search.][75366]
- [Added the Ayu theme to rustdoc.][71237]

Compatibility Notes
-------------------
- [Bumped the minimum supported Emscripten version to 1.39.20.][75716]
- [Fixed a regression parsing `{} && false` in tail expressions.][74650]
- [Added changes to how proc-macros are expanded in `macro_rules!` that should
  help to preserve more span information.][73084] These changes may cause
  compiliation errors if your macro was unhygenic or didn't correctly handle
  `Delimiter::None`.
- [Moved support for the CloudABI target to tier 3.][75568]
- [`linux-gnu` targets now require minimum kernel 2.6.32 and glibc 2.11.][74163]

Internal Only
--------
- [Improved default settings for bootstrapping in `x.py`.][73964]
  You can read details about this change in the ["Changes to `x.py`
  defaults"](https://blog.rust-lang.org/inside-rust/2020/08/30/changes-to-x-py-defaults.html)
  post on the Inside Rust blog.

- [Added the `rustc-docs` component.][75560] This allows you to install
  and read the documentation for the compiler internal APIs. (Currently only
  available for `x86_64-unknown-linux-gnu`.)

[1.47.0-cfg]: https://docs.microsoft.com/en-us/windows/win32/secbp/control-flow-guard
[76980]: rust-lang/rust#76980
[75048]: rust-lang/rust#75048
[74163]: rust-lang/rust#74163
[71237]: rust-lang/rust#71237
[74869]: rust-lang/rust#74869
[73858]: rust-lang/rust#73858
[75716]: rust-lang/rust#75716
[75908]: rust-lang/rust#75908
[75516]: rust-lang/rust#75516
[75560]: rust-lang/rust#75560
[75568]: rust-lang/rust#75568
[75366]: rust-lang/rust#75366
[75204]: rust-lang/rust#75204
[74650]: rust-lang/rust#74650
[74419]: rust-lang/rust#74419
[73964]: rust-lang/rust#73964
[74021]: rust-lang/rust#74021
[74060]: rust-lang/rust#74060
[73893]: rust-lang/rust#73893
[73526]: rust-lang/rust#73526
[73583]: rust-lang/rust#73583
[73084]: rust-lang/rust#73084
[73197]: rust-lang/rust#73197
[72488]: rust-lang/rust#72488
[cargo/8456]: rust-lang/cargo#8456
[cargo/8478]: rust-lang/cargo#8478
[cargo/8485]: rust-lang/cargo#8485
[cargo/8500]: rust-lang/cargo#8500
[cargo/8571]: rust-lang/cargo#8571
[`Ident::new_raw`]:  https://doc.rust-lang.org/nightly/proc_macro/struct.Ident.html#method.new_raw
[`Range::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.Range.html#method.is_empty
[`RangeInclusive::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.RangeInclusive.html#method.is_empty
[`Result::as_deref_mut`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref_mut
[`Result::as_deref`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref
[`TypeId::of`]: https://doc.rust-lang.org/nightly/std/any/struct.TypeId.html#method.of
[`Vec::leak`]: https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.leak
[`f32::TAU`]: https://doc.rust-lang.org/nightly/std/f32/consts/constant.TAU.html
[`f64::TAU`]: https://doc.rust-lang.org/nightly/std/f64/consts/constant.TAU.html
[`pointer::offset_from`]: https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.offset_from
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.