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

x86 _rdtsc has wrong return type #559

Closed
GabrielMajeri opened this issue Sep 5, 2018 · 7 comments · Fixed by #733
Closed

x86 _rdtsc has wrong return type #559

GabrielMajeri opened this issue Sep 5, 2018 · 7 comments · Fixed by #733
Labels

Comments

@GabrielMajeri
Copy link
Contributor

GabrielMajeri commented Sep 5, 2018

This issue is related to the recently stabilised x86 intrinsic function _rdtsc

The issue is that the function is defined to return a signed i64. This is in accordance with Intel's documentation. However, that is not how most people define it, including C/C++ compilers.

This is how GCC defines it:

extern __inline unsigned long long // < note the return type
__attribute__((__gnu_inline__, __always_inline__, __artificial__))
__rdtsc (void)
{
  return __builtin_ia32_rdtsc ();
}

This Clang example proves it also defines it as unsigned long long.

Now, I'm confused. Since the timestamp counter is a monotonically increasing timer, it wouldn't make sense to return negative values. In fact, I think it is allowed to return values with the most significant bit set (which would make the returned value negative, unless transmuted). Is the mismatch intended?

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 5, 2018

This is in accordance with Intel's documentation.

core::arch implements the Intel's spec as close as possible. Bugs in the spec should be reported to Intel, but the spec uses signed integers in many places in which it could use unsigned ones. Why? No idea.

FYI the main reason we decided to implement the spec 1:1 is that we don't have the manpower to design a new API for each of the tens of thousand intrinsics that belong in core::arch.


FWIW you can write a thin wrapper that transmutes i64 into an u64 if you prefer that API in stable Rust (its a no-op). Also, clang probably just did the same thing as GCC to be compatible with it.

@GabrielMajeri
Copy link
Contributor Author

@gnzlbg I have raised an issue with Intel about this intrinsics. Hopefully they'll reply and fix their spec.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 5, 2018

I have raised an issue with Intel about this intrinsics. Hopefully they'll reply and fix their spec.

So I don't hold my hopes up here because this would be a breaking change, so I'd suspect that nothing will happen. However, Intel and other vendors do send errata and make breaking changes every now and then, and I'm interested in seeing how we would deal with that.

@GabrielMajeri in the mean time, this particular function is defined in the Intel spect as just reading the content of a register, so it doesn't really matter whether the return value is a signed or unsigned integer (or an [u8; 8] or whatever) as long as it is 64-bit wide. Its "the user"s job to interpret those bits as appropriate. I don't know what the value of the register "means" when the sign bit is set, but I'd expect it will just be an unsigned integer.

I have a crate in progress to strongly type all intel intrinsics here and expose them using a safe API when possible: https://github.com/gnzlbg/typed_arch I'll take a PR that implemented the rdtsc intrinsics with a "better" and safe API.


EDIT: do you have a link to the Intel issue / bug report / tracking id?

@GabrielMajeri
Copy link
Contributor Author

GabrielMajeri commented Sep 5, 2018

@gnzlbg Well I submitted a comment to this forum thread which is where the Intrinsics guide told me to post issues to. The comment is still waiting for moderator approval, and then I'll wait for a reply from Intel staff.
Edit: the comment is approved now, we just need to wait

To me it just looks like an issue with their documentation, considering they did mess up the types in their intrinsics guide before.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 19, 2019

@GabrielMajeri is there an acknowledgment of the bug or where you contacted about it ? Otherwise it might be worth it to start a new thread there just about that.

@GabrielMajeri
Copy link
Contributor Author

I've tried submitting another message, although it seems the intrinsics guide is not exactly a priority. I wouldn't consider it to be an official specification in any way, just a helpful resource for developers who are looking for documentation on how to use the intrinsics.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 22, 2019

Sadly, there does not appear to be a real "specification" of Intel compiler C intrinsics beyond the intrinsics guide. I'm going to merge the PR, and try to update stdsimd upstream doing a crater run with it. Maybe we can fix it.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jul 5, 2019
Pkgsrc changes:
 * NetBSD/sparc64 disabling of "packed" removed ("packed" removed upstream)
 * Adapt src_libstd_build.rs patch, update sed'ing of .cargo-checksum.json

Build verified on NetBSD 8.0/amd64.

Upstream changes:

Version 1.36.0 (2019-07-04)
==========================

Language
--------
- [Non-Lexical Lifetimes are now enabled on the 2015 edition.][59114]
- [The order of traits in trait objects no longer affects the semantics of that
  object.][59445] e.g. `dyn Send + fmt::Debug` is now equivalent to
  `dyn fmt::Debug + Send`, where this was previously not the case.

Libraries
---------
- [`HashMap`'s implementation has been replaced with `hashbrown::HashMap` implem
entation.][58623]
- [`TryFromSliceError` now implements `From<Infallible>`.][60318]
- [`mem::needs_drop` is now available as a const fn.][60364]
- [`alloc::Layout::from_size_align_unchecked` is now available as a const fn.][6
0370]
- [`String` now implements `BorrowMut<str>`.][60404]
- [`io::Cursor` now implements `Default`.][60234]
- [Both `NonNull::{dangling, cast}` are now const fns.][60244]
- [The `alloc` crate is now stable.][59675] `alloc` allows you to use a subset
  of `std` (e.g. `Vec`, `Box`, `Arc`) in `#![no_std]` environments if the
  environment has access to heap memory allocation.
- [`String` now implements `From<&String>`.][59825]
- [You can now pass multiple arguments to the `dbg!` macro.][59826] `dbg!` will
  return a tuple of each argument when there is multiple arguments.
- [`Result::{is_err, is_ok}` are now `#[must_use]` and will produce a warning if
  not used.][59648]

Stabilized APIs
---------------
- [`VecDeque::rotate_left`]
- [`VecDeque::rotate_right`]
- [`Iterator::copied`]
- [`io::IoSlice`]
- [`io::IoSliceMut`]
- [`Read::read_vectored`]
- [`Write::write_vectored`]
- [`str::as_mut_ptr`]
- [`mem::MaybeUninit`]
- [`pointer::align_offset`]
- [`future::Future`]
- [`task::Context`]
- [`task::RawWaker`]
- [`task::RawWakerVTable`]
- [`task::Waker`]
- [`task::Poll`]

Cargo
-----
- [Cargo will now produce an error if you attempt to use the name of a required
dependency as a feature.][cargo/6860]
- [You can now pass the `--offline` flag to run cargo without accessing the netw
ork.][cargo/6934]

You can find further change's in [Cargo's 1.36.0 release notes][cargo-1-36-0].

Clippy
------
There have been numerous additions and fixes to clippy, see [Clippy's 1.36.0 rel
ease notes][clippy-1-36-0] for more details.

Misc
----

Compatibility Notes
-------------------
- [`std::arch::x86::_rdtsc` returns `u64` instead of `i64`][stdsimd/559]
- [`std::arch::x86_64::_mm_shuffle_ps` takes an `i32` instead of `u32` for `mask
`][stdsimd/522]
- With the stabilisation of `mem::MaybeUninit`, `mem::uninitialized` use is no
  longer recommended, and will be deprecated in 1.38.0.

[60318]: rust-lang/rust#60318
[60364]: rust-lang/rust#60364
[60370]: rust-lang/rust#60370
[60404]: rust-lang/rust#60404
[60234]: rust-lang/rust#60234
[60244]: rust-lang/rust#60244
[58623]: rust-lang/rust#58623
[59648]: rust-lang/rust#59648
[59675]: rust-lang/rust#59675
[59825]: rust-lang/rust#59825
[59826]: rust-lang/rust#59826
[59445]: rust-lang/rust#59445
[59114]: rust-lang/rust#59114
[cargo/6860]: rust-lang/cargo#6860
[cargo/6934]: rust-lang/cargo#6934
[`VecDeque::rotate_left`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_left
[`VecDeque::rotate_right`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_right
[`Iterator::copied`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#tymethod.copied
[`io::IoSlice`]: https://doc.rust-lang.org/std/io/struct.IoSlice.html
[`io::IoSliceMut`]: https://doc.rust-lang.org/std/io/struct.IoSliceMut.html
[`Read::read_vectored`]: https://doc.rust-lang.org/std/io/trait.Read.html#method.read_vectored
[`Write::write_vectored`]: https://doc.rust-lang.org/std/io/trait.Write.html#method.write_vectored
[`str::as_mut_ptr`]: https://doc.rust-lang.org/std/primitive.str.html#method.as_mut_ptr
[`mem::MaybeUninit`]: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html
[`pointer::align_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset
[`future::Future`]: https://doc.rust-lang.org/std/future/trait.Future.html
[`task::Context`]: https://doc.rust-lang.org/beta/std/task/struct.Context.html
[`task::RawWaker`]: https://doc.rust-lang.org/beta/std/task/struct.RawWaker.html
[`task::RawWakerVTable`]: https://doc.rust-lang.org/beta/std/task/struct.RawWakerVTable.html
[`task::Waker`]: https://doc.rust-lang.org/beta/std/task/struct.Waker.html
[`task::Poll`]: https://doc.rust-lang.org/beta/std/task/enum.Poll.html
[clippy-1-36-0]: https://github.com/rust-lang/rust-clippy/blob/master/CHANGELOG.md#rust-136
[cargo-1-36-0]: https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-136-2019-07-04
[stdsimd/522]: rust-lang/stdarch#522
[stdsimd/559]: rust-lang/stdarch#559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants