Skip to content

add __breakpoint #558

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

Merged
merged 6 commits into from
Nov 11, 2018
Merged

add __breakpoint #558

merged 6 commits into from
Nov 11, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Sep 3, 2018

this is an ARM compiler specific intrinsic which can be used to implement
CMSIS's __BKPT intrinsic

r? @gnzlbg

@japaric japaric mentioned this pull request Sep 3, 2018
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

Modulo the documentation nitpicks this LGTM.

///
/// `val` is a compile-time constant integer whose range is:
///
/// - `0...65535` if you are compiling source as A32 code.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be removed since it currently won't work and can be added later once it works in a backwards compatible way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, I see you also updated the Note below. So I think this is fine. I'll merge once CI is green. Let me know if there are any spurious CI failures.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 4, 2018

This is failing on aarch64 with:

error: <inline asm>:1:2: error: unrecognized instruction mnemonic
        BKPT 0
        ^
error: aborting due to previous error

We might need a cfg on target arch and use a different instruction there.

@paoloteti
Copy link
Contributor

We might need a cfg on target arch and use a different instruction there.

If I'm not wrong is BKPT in aarch32 and BRK in aarch64.

@andre-richter
Copy link
Member

@alexcrichton
Copy link
Member

Should this macro perhaps panic if any of the upper bits above 0xff are set? (I think right now it silently masks them off)

@japaric
Copy link
Member Author

japaric commented Sep 15, 2018

error: cannot find macro `panic!` in this scope
  --> crates/coresimd/src/../../../coresimd/arm/armclang.rs:43:5
   |
43 |     assert!(val >= 0 && val <= 255);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error

Interesting.

Thinking more about it: Could we omit the assert! and document the behavior of % 256? I would prefer to avoid panicking branches in intrinsics as they can pull in the formatting machinery (few KBs) in unoptimized programs.

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 11, 2018

Should this macro perhaps panic if any of the upper bits above 0xff are set? (I think right now it silently masks them off)

@alexcrichton it appears that it is not possible right now as @japaric mentioned. I've taken over this PR and just document that the only accepted values of val are in range [0, 255], and that values out-of-range are undefined behavior. In the implementation, I've documented the behavior of the constify macro on out-of-range values but that's an implementation detail that we can change however we want.

@gnzlbg gnzlbg merged commit 7fbd636 into rust-lang:master Nov 11, 2018
jethrogb pushed a commit to jethrogb/stdsimd that referenced this pull request Nov 27, 2018
Revert "do not validate the argument to the __breakpoint intrinsic", "aarch64 support and range validation", "fix typo", "fix assert_instr test", "remove TODO; add link to doc comment", "add __breakpoint"

This reverts commits 7fbd636, 72b4143, 9e5c960, 8b84dc4, af7c166, 40f61c7.
jethrogb pushed a commit to jethrogb/stdsimd that referenced this pull request Nov 27, 2018
Revert "do not validate the argument to the __breakpoint intrinsic", "aarch64 support and range validation", "fix typo", "fix assert_instr test", "remove TODO; add link to doc comment", "add __breakpoint"

This reverts commits 7fbd636, 72b4143, 9e5c960, 8b84dc4, af7c166, 40f61c7.
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.

6 participants