Support 16-bit pointers as well as i/usize #33460
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
|
This all seems pretty straightforward and easy to land, but in terms of keeping this up to date is there a way that we could add a simple regression test? There's a lot more places than I would have expected that need to be updated for 16-bit pointers, and ensuring that any new location stays up to date may be difficult. |
I'm 100% behind this, but I'm pretty stumped on how to do it. It seems like I need to be able to say "hey, this platform has 16-bit pointers", but there aren't any that do so (for now). Do you think there's a way I could use a target description JSON file, specify
You're telling me! 🌴 Unfortunately, most of them were found The Hard Way — by finding a compiler ICE building the compiler or my end code. It's a shame that there wasn't more "hey you forgot this enum value here", but most cases (reasonably) have a default value case. |
|
The way I remember, on 16-bit systems you usually would have a distinction between far (segment+ptr) and near pointers, which seems to not be addressed here. What gives? |
|
@nagisa I have heard tales of Ye Olde Days of |
|
@shepmaster is right in saying AVR doesn't use segmented memory. Normal pointers are the usual 16 bits. There are a few AVR chips with a bit more memory that 16 bits can address - to access memory higher than this, there's a special IO register for which you must load the other address bits into first. It still works as an ordinary pointer, but spread over two registers. When you try and address memory with a 32-bit pointer, the compiler does the special IO register loading for you. |
Mmm, that doesn't seem to work because it ultimately looks like cross compiling and there's no libcore or libstd for that made up platform. I'm open to any suggestions! |
|
And If I try to follow the lead of the target-specification tests and eschew libstd and libcore, I end up triggering an LLVM assertion: |
|
I can get a tiny bit of test with this: let _a: usize = 0x1_0000; // literal out of range for usizeWhich indicates that, yes, 16-bit pointers only take up 16-bits. I'm not sure of the value of the test, but I'll continue poking at it. I'd still appreciate suggestions that anyone has for useful tests. |
|
Hm yeah I suspect that our main recourse for testing a patch like this would be adding automation which generated at least libcore for a 16-bit target. Also, forgot to do this earlier, but cc @rust-lang/compiler and @rust-lang/tools |
| @@ -178,6 +179,7 @@ impl TargetDataLayout { | |||
|
|
|||
| pub fn ptr_sized_integer(&self) -> Integer { | |||
| match self.pointer_size.bits() { | |||
| 16 => I16, | |||
nrc
added
T-lang
|
The tools team discussed this PR during triage yesterday and the conclusion was that from our perspective this seems fine. This would basically just be expanding our tier 3 platforms, which we regularly take PRs for, so no sweat on that front! |
|
@alexcrichton that's great! I found one more tricky spot that needs to be updated, I can rebase and squash that to this commit and it should be good-to-merge then! |
|
@alexcrichton was there any comment one way or the other about tests that would need to be added before the merge? |
|
At least from a tooling perspective we were ok not having a bunch of rigorous tests or anything, none of our tier 3 platforms do either :) For merging, though, this cuts across a few other teams as well, so at least @rust-lang/compiler will probably want to sign off as well |
This is based on the original work of Dylan McKay for the [avr-rust project][ar]. [ar]: https://github.com/avr-rust/rust
shepmaster
force-pushed
the
16-bit-pointers
branch
from
40e2489
to
bc7595c
Compare
|
I've fixed up the one extra spot I've found and rebased for good measure. |
|
I gave it a quick read. Looks like a fairly straight-forward patch; I've not thought deeply about what else might be needed to support 16 bit platforms, but this at least doesn't look like it would harm 32 or 64 bit. 👍 |
|
The libs team discussed this briefly at yesterday's triage and the conclusion was that from that perspective we're ok along the lines of "let's see how this plays out without bending over backwards too much" |
alexcrichton
removed
T-libs-api
|
I'm cool with us landing this. Might give us more street cred with micro controller community |
|
Thanks for the positive reviews all around! What other teams remain to sign off? |
|
Judging by the area of change, I'd expect that at least the compiler team should weigh in. If they'd like to escalate, however, we can discuss at the core meeting. |
|
@nikomatsakis we did discuss at compiler meeting (twice I think, right?) We didn't really attempt to form any solid consensus (we almost never have a formal quorum anyway), but no one present objected... |
|
@pnkfelix indeed, we did discuss, and everybody was more-or-less in favor of doing this on an experimental basis. |
|
I think the conclusion is that this is ok. @bors r+ |
|
📌 Commit bc7595c has been approved by |
Support 16-bit pointers as well as i/usize I'm opening this pull request to get some feedback from the community. Although Rust doesn't support any platforms with a native 16-bit pointer at the moment, the [AVR-Rust][ar] fork is working towards that goal. Keeping this forked logic up-to-date with the changes in master has been onerous so I'd like to merge these changes so that they get carried along when refactoring happens. I do not believe this should increase the maintenance burden. This is based on the original work of Dylan McKay (@dylanmckay). [ar]: https://github.com/avr-rust/rust
I'm opening this pull request to get some feedback from the community.
Although Rust doesn't support any platforms with a native 16-bit pointer at the moment, the AVR-Rust fork is working towards that goal. Keeping this forked logic up-to-date with the changes in master has been onerous so I'd like to merge these changes so that they get carried along when refactoring happens. I do not believe this should increase the maintenance burden.
This is based on the original work of Dylan McKay (@dylanmckay).