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.

Sign up for GitHub

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

Jump to bottom

Improve assembly test for CMSE ABIs #130752

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Improve assembly test for CMSE ABIs #130752

merged 1 commit into from
Sep 25, 2024

Conversation

tdittr
Copy link
Contributor

@tdittr tdittr commented Sep 23, 2024

Tracking issues: #75835 #81391

This ensures the code-gen for these ABIs does not change silently. There is a small chance that this code-gen might change, however even GCC (https://godbolt.org/z/16arxab5x and https://godbolt.org/z/16arxab5x) generates almost the same assembly for these ABIs. I hope the notes in the comments should help fix the tests if it ever breaks.

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2024
jieyouxu
jieyouxu requested changes Sep 23, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR. I have some suggestions, and requests to make this test less sensitive to benign codegen changes.

tests/assembly/cmse.rs Show resolved Hide resolved
tests/assembly/cmse.rs Outdated Show resolved Hide resolved
tests/assembly/cmse.rs Outdated Show resolved Hide resolved
tests/assembly/cmse.rs Show resolved Hide resolved
@jieyouxu jieyouxu assigned jieyouxu and unassigned Mark-Simulacrum Sep 23, 2024
@jieyouxu jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. F-cmse_nonsecure_entry `#![feature(cmse_nonsecure_entry)]` F-abi_c_cmse_nonsecure_call `#![feature(abi_c_cmse_nonsecure_call)]` A-ABI Area: Concerning the application binary interface (ABI) labels Sep 23, 2024
jieyouxu
jieyouxu reviewed Sep 23, 2024
View reviewed changes
tests/assembly/cmse.rs Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2024
@tdittr @folkertdev
Improve assembly test for CMSE ABIs
3d168c2 
This ensures the code-gen for these ABIs does not change silently.

Co-authored-by: Folkert <[email protected]>
@tdittr
Copy link
Contributor Author

tdittr commented Sep 23, 2024

Thanks for the quick response on this one :)

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 23, 2024
jieyouxu
jieyouxu reviewed Sep 23, 2024
View reviewed changes
tests/assembly/cmse.rs Show resolved Hide resolved
tests/assembly/cmse.rs Show resolved Hide resolved
tests/assembly/cmse.rs Show resolved Hide resolved
tests/assembly/cmse.rs Show resolved Hide resolved
@jieyouxu
Copy link
Member

jieyouxu commented Sep 23, 2024 •
edited
Loading

The other labels and reg matches look more robust to me 👍

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2024
jieyouxu
jieyouxu approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Since we established that we don't need to check precisely how the registers are cleared (considering llvm sometimes does things like use a register or function pointer address to zero another register instead of using an immediate zero), this LGTM

@jieyouxu
Copy link
Member

Thanks!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 24, 2024

📌 Commit 3d168c2 has been approved by jieyouxu

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 24, 2024
@tgross35 tgross35 mentioned this pull request Sep 24, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2024
@bors
Auto merge of rust-lang#130807 - tgross35:rollup-p304vzf, r=tgross35
1b5aa96 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#130234 (improve compile errors for invalid ptr-to-ptr casts with trait objects)
 - rust-lang#130752 (Improve assembly test for CMSE ABIs)
 - rust-lang#130764 (Separate collection of crate-local inherent impls from error tracking)
 - rust-lang#130788 (Pin memchr to 2.5.0 in the library rather than rustc_ast)
 - rust-lang#130789 (add InProgress ErrorKind gated behind io_error_inprogress feature)
 - rust-lang#130793 (Mention `COMPILETEST_VERBOSE_CRASHES` on crash test failure)
 - rust-lang#130798 (rustdoc: inherit parent's stability where applicable)

Failed merges:

 - rust-lang#130735 (Simple validation for unsize coercion in MIR validation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1f52c07 into rust-lang:master Sep 25, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 25, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2024
@rust-timer
Unrolled build for rust-lang#130752
e4c5d1b 
Rollup merge of rust-lang#130752 - tdittr:cmse-assembly-tests, r=jieyouxu

Improve assembly test for CMSE ABIs

Tracking issues: rust-lang#75835 rust-lang#81391

This ensures the code-gen for these ABIs does not change silently. There is a small chance that this code-gen might change, however even GCC (https://godbolt.org/z/16arxab5x and https://godbolt.org/z/16arxab5x) generates almost the same assembly for these ABIs. I hope the notes in the comments should help fix the tests if it ever breaks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-ABI Area: Concerning the application binary interface (ABI) A-testsuite Area: The testsuite used to check the correctness of rustc F-abi_c_cmse_nonsecure_call `#![feature(abi_c_cmse_nonsecure_call)]` F-cmse_nonsecure_entry `#![feature(cmse_nonsecure_entry)]` 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
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tdittr @rustbot @jieyouxu @bors @Mark-Simulacrum