Skip to content

fix - aarch64_be tests #1786

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Jamesbarford
Copy link
Contributor

Fixes issues that were surfaced through; #1776 (comment)

I've not hooked this up to the ci as it seems there is a PR in tow that will do that; #1785

@rustbot
Copy link
Collaborator

rustbot commented Apr 22, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@@ -27,6 +27,7 @@ run() {
--env NOSTD \
--env NORUN \
--env RUSTFLAGS \
--env CARGO_UNSTABLE_BUILD_STD \
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would get used by the CI, thinking about it; it probably makes sense for this PR to also include the code to make the CI run seeing as that's why these changes are being made

ENV STDARCH_TEST_SKIP_FEATURE=tme
# The table instructions, while correct, generate some rev64 instructions which
# increases the number of instructions generated
ENV STDARCH_ASSERT_INSTR_LIMIT=32
Copy link
Member

Choose a reason for hiding this comment

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

Currently all the logic for selecting the instruction limit is in crates/stdarch-test/src/lib.rs so it would be good to keep it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly sure how to weave in something specific for aarch64_be without bloating out the file with changes. https://github.com/rust-lang/stdarch/blob/master/crates/stdarch-test/src/lib.rs#L119 there shows a check for; STDARCH_ASSERT_INSTR_LIMIT so I reasoned it was a standardised way of setting the instruction count limit?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to just have a blanket rule for aarch64_be that sets the limit to 32, it's just that I would like that to be in stdarch-test/src/lib.rs instead of in the dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f4a7aeb 👍

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2025

CI failure seems to be fallout from rust-lang/rust#139261.

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.

3 participants