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

Fix build on non-32/64-bit architectures #1144

Merged
merged 6 commits into from
Jul 13, 2021

Conversation

Palladinium
Copy link
Contributor

I've been trying to build rand for AVR (which has 8-bit pointers), failing with errors like the following:

error[E0599]: no method named `wmul` found for type `usize` in the current scope
   --> /home/patrick/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rand-0.8.4/src/distributions/uniform.rs:491:42
    |
491 |                         let (hi, lo) = v.wmul(range);
    |                                          ^^^^ method not found in `usize`
...
561 | uniform_int_impl! { isize, usize, usize }
    | ----------------------------------------- in this macro invocation
    |
    = help: items from traits can only be used if the trait is implemented and in scope
note: `WideningMultiply` defines an item `wmul`, perhaps you need to implement it
   --> /home/patrick/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/rand-0.8.4/src/distributions/utils.rs:14:1
    |
14  | pub(crate) trait WideningMultiply<RHS = Self> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

This fixes it, and should fix it for any 8-bit and 16-bit architecture.

Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

The changes look fine to me, but we should also consider adding tests for platforms with such small pointer widths.

@josephlr
Copy link
Member

josephlr commented Jul 13, 2021

The changes look fine to me, but we should also consider adding test for platforms with such small pointer widths.

Agreed, we should have tests for a least one target_pointer_width = "16" architecture. Building for AVR is a bit of a pain right now, due to rust-lang/compiler-builtins#400, which is caused by a bug in LLVM. The upstream support for non-32/64-bit targets has never been that great. So if we test this, we definitely want to pin a toolchain.

Running the following commands worked for me to build rand on the builtin avr-unknown-gnu-atmega328 target:

rustup toolchain install nightly-2021-01-07
rustup default nightly-2021-01-07
cargo build -Z build-std=core --target=avr-unknown-gnu-atmega328 --no-default-features --release

@Palladinium can you add test for this to the CI. See existing no-std build tests:

test-no-std:

We may want to add a dedicated `test-avr job, as it will need its own toolchain.

@josephlr
Copy link
Member

I've been trying to build rand for AVR (which has 8-bit pointers),

Note: AVR has 16-bit pointers (and 8-bit registers). See the Wikipedia page and the AVR-Rust Guidebook

Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

Marking "request changes" for adding to the CI and removing the u8 impls.

@Palladinium Palladinium requested a review from josephlr July 13, 2021 05:20
Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing the CI stuff!

@josephlr
Copy link
Member

I'll let @vks handle merging/squashing, as I don't normally merge PRs for this repo.

@vks vks merged commit fb6390f into rust-random:master Jul 13, 2021
@vks
Copy link
Collaborator

vks commented Jul 13, 2021

Thanks everyone!

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

Successfully merging this pull request may close these issues.

4 participants