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

add the trap instrinsic #571

Merged
merged 4 commits into from
Nov 11, 2018
Merged

add the trap instrinsic #571

merged 4 commits into from
Nov 11, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Sep 15, 2018

as discussed in rust-lang/rfcs#2512

instead of the generic "trap" name the architecture specific name of the trap instruction has been used

/// Generates the trap instruction `UD2`
#[cfg_attr(test, assert_instr(ud2))]
#[inline]
pub unsafe fn ud2() -> ! {
Copy link
Contributor

Choose a reason for hiding this comment

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

The stdsimd-verify crate (https://github.com/rust-lang-nursery/stdsimd/tree/master/crates/stdsimd-verify) needs to be updated to support bottom.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexcrichton
Copy link
Member

This looks good to me!

For x86 I think this'll be initially unstable, right? If so, yay (can have a formal FCP shortly afterwards)

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 25, 2018

ping @japaric

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 11, 2018

I've decided to take over this. I've rebased and hopefully fixed everything.

@parched
Copy link

parched commented Nov 11, 2018

I know I initially suggested naming these by their underlying instructions, but that was only if they're gong in std::arch::<arch>. I would much prefer a generic trap intrinsic for portability though. Perhaps instead we could have std::arch::trap but one that is cfg-gated to architectures that are known not to lower to __abort. Or, perhaps it could be always available with the default implementation just being an infinite loop and, only for architectures that are known no to lower to __abort, call llvm.trap instead.

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 11, 2018

@parched these are all going in std::arch::<arch>; this change does not introduce a portable trap intrinsic.

Once these are stabilized, one could implement one in stable Rust, although I agree with you in that it would make sense to include a portable trap intrinsic "somewhere" in standard (e.g. core::{arch, mem, hint, ...}::trap() ?). It might make sense to open a new RFC-repo issue to discuss a "portable" trap intrinsic.

@gnzlbg gnzlbg merged commit e020398 into rust-lang:master Nov 11, 2018
@parched
Copy link

parched commented Nov 12, 2018

@gnzlbg I guess my concern is, for example on AArch64, there are now 2 BRK intrinsics (std::arch::aarch64::brk and std::arch::aarch64::__breakpoint), neither of which are standard vendor intrinsics in C. The "vendor intrinsic" for this in C is the portable __builtin_trap.

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 12, 2018

@parched

Note that these APIs are unstable, and that we haven't signed off on their stabilization yet. The rfc repo issue achieved consensus that this was a venue worth exploring and I've signed off here that the proposed implementation is good enough for nightly, but that's it. We will need an RFC to stabilize these, and I think the RFC issue showed that we need more experience with these before writing an RFC because of some unresolved questions.

The "vendor intrinsic" for this in C is the portable __builtin_trap.

I do think that a portable __builtin_trap intrinsic is worth pursuing and recommend you to open a small issue in the RFC repo about it. After its discussed, I volunteer to implement it if nobody beats me to it.

There were some unresolved questions in the previous issue about how portable can such an intrinsic actually be, what to do in the platforms that might not support it, in which part of libcore does it belong, etc.

So the previous issue was mixing in my view two things:

  • providing access to platform specific trap intrinsics / instructions
  • providing a portable way to trap

Providing access to platform specific trap intrinsics and instructions was pretty uncontroversial, and its what's been merged here - this allows implementing a portable way to trap in an external crate (even though this might not be as good as __builtin_trap).

Providing a portable way to trap had the unresolved questions mentioned above, and the discussion derailed a bit, which is why I think it is worth to have a focused discussion that's specific about that only.

I think that once we have implemented both alternatives on nightly, we will be in a much better place to consider whether we ought to expose both, even though that introduces some duplication, or whether a portable way to trap is enough, in which case we can just remove the platform specific intrinsics. Once we are there, we'll have a much better feeling for what the "right" solution is, and it would be RFC time.

@jethrogb
Copy link
Contributor

When updating the rust submodule to the latest master, I get this error:

error[E0433]: failed to resolve. Maybe a missing `extern crate _core;`?
   --> libcore/../stdsimd/coresimd/x86/mod.rs:523:7
    |
523 |     ::_core::intrinsics::abort()
    |       ^^^^^ Maybe a missing `extern crate _core;`?

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 19, 2018

The paths added here might need some tuning to build upstream (we can't really test that here so it is normal that things break every now and then). Try the following:

  • replace ::_core::intrinsics with just intrinsics in all uses inside coresimd (not only x86, from the looks, this will fail for all archs)
  • add an appropriate import of the intrinsics module to crates/coresimd/src/mod.rs

You might need to tweak things a bit, so my recommendation is to just create a fork, change rust-lang/rust to use your fork, tweak things there until it builds properly, and then send a PR here to fix things. Let me know if you need help with this.

@jethrogb
Copy link
Contributor

Fixed in #599

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.

5 participants