non-x86 runtime detection updates #229

lu-zero · 2017-12-10T03:00:48Z

The arm32 variant is not tested due a small hardware failure.

alexcrichton · 2017-12-10T15:20:31Z

This looks great to me, thanks!

Mind adding some more documentation as well? For example for a newbie like me, I'm not even sure what auxv is :(. It'd be great if that file explained a little bit about the purpose of the file, the format, and then maybe provided a link to more info?

Additionally for the bits defined in powerpc64, could documentation be referenced for where the values come from?

gnzlbg · 2017-12-10T17:05:29Z

src/runtime/aarch64.rs

    {
        let mut enable_feature = |f| {
            if x.has_feature(&f) {
-                bit::set(value, f as u32);
+                value = bit::set(value, f as u32);


thanks for fixing this! The x86 run-time did not have this issue, I wish there was an easy way to unify this code. https://github.com/rust-lang-nursery/stdsimd/blob/master/coresimd/src/runtime/x86.rs#L329

gnzlbg · 2017-12-10T17:10:34Z

src/runtime/linux/auxvec.rs

+    // TODO: make it less ugly if possible
+    pub fn new() -> Result<Self, ::std::io::Error> {
+        use std::io::Read;
+        let mut file = ::std::fs::File::open("/proc/self/auxv")?;


This is a good first step, but we should only do this if libc's getauxval does not exist. I think we can add getauxval as an extern C weak_symbol, and check whether the value of a function pointer to it is null or not at run-time. If it is not null we should just call that, but if it is null then the binary does not have a linked extern C getauxval function, and we can proceed to read proc/self/auxv as a fallback.

I wish someone (@alexcrichton , @BurntSushi ) would review #211 since that would allow us to add some more ARM targets for testing and use qemu-system instead of qemu-user to test this on linux and android targets.

I avoided getauxval since it is nonstandard, I wouldn't rely on weak_symbol to check for its presence (static linking and non-gnu libcs aren't uncommon:))

Typically we don't use weak symbols as they're not very portable, but a solution is to do what libstd does which is dlsym at runtime.

IIRC the problem was that /proc/self/auxv is not guaranteed to exist on all Android targets, but libc::getauxval is (and also, libc::getauxval is always faster than reading /proc/self/auxv on all platforms that have it).

Anyways, we don't need to add libc::getauxval on this PR, that was more me thinking out loud. This PR is a good step forward as it is. Good job!

gnzlbg · 2017-12-10T18:09:59Z

src/runtime/aarch64.rs

@@ -45,6 +45,23 @@ pub fn detect_features<T: linux::FeatureQuery>(mut x: T) -> usize {
    value
 }

+/// Probe the ELF Auxiliary vector for hardware capabilities
+///
+/// The values are part of the platform-specific `asm/hwcap.h`


Could you maybe add a link to the linux header where hwcap is defined? That is, here: https://github.com/torvalds/linux/blob/master/arch/arm64/include/uapi/asm/hwcap.h

(and do the same for arm?)

gnzlbg · 2017-12-10T18:22:19Z

src/runtime/linux/auxvec.rs

+/// The desugared version is a usize tag followed by a union with
+/// the same storage size.
+pub struct AuxVec {
+    raw: Vec<usize>,


I think we should avoid the Vec here. Can't we just use a [usize; 64] + a length?

I could just copy the hwcaps when present maybe?

I think that would be better.

I know we are already doing a lot of syscals by reading the file, and that allocating the vector should not matter much, but I was hoping that if we managed to add support for libc::getauxval in the future we could reuse at least some of this for that.

gnzlbg · 2017-12-10T18:23:32Z

src/runtime/linux/mod.rs

 }

 /// Detects ARM features:
 pub fn detect_features() -> usize {
-    // FIXME: use libc::getauxval, and if that fails /proc/auxv


We should keep this fixme because we are still not calling libc::getauxval. We should try to do that first, and if that fails, then read /proc/self/auxv, and if that fails, then try to read /proc/cpuinfo.

gnzlbg · 2017-12-10T18:25:00Z

src/runtime/powerpc64.rs

+
+#[cfg(test)]
+mod tests {
+    #[test]


Can you add a test that parses one or two dumps of /proc/cpuinfo for powerpc CPUs?

No, since the cpuinfo is w/out information at least for now.

Check this out: https://github.com/randombit/cpuinfo/tree/master/ppc

That's a collection of cpuinfo files for a lot of powerpc cpus. Maybe one could c&p one of those files here and add it as a test?

I'm afraid the comment is not clear in the function: /proc/cpuinfo on powerpc does not provide useful information, I left that code there since I plan to ask the linux kernel developers to provide the same information as arm does. I can change the function with a simple return false so it is less misleading for not.

The comment is clear, I just skimmed too fast through it.

What is the value of adding the cpuinfo implementation? We can always add it later once a kernel is released that "supports" it right?

I just prefer to leave the placeholder since it is sort of future proof.

Does the linux mod require this implementation to exist? If that is the case I think we should fix that instead.

I can do that later if you prefer, but I think that leaving it with the expectation that a future Linux kernel version implements this is a long shot.

The linux mod requires the implementation and I wouldn't spend more time on that. I updated it so it leverages the small bit of information to get to know if at least 1 feature is enabled.

Now that the examples of cpuinfo for powerpc are there everything has become clear (sorry that it took so long). Just to check that I understand correctly, the only thing that can be checked from /proc/cpuinfo for PowerPC is whether altivec is supported or not, is that right?

gnzlbg · 2017-12-10T18:26:51Z

src/runtime/linux/auxvec.rs

+#[cfg(test)]
+mod tests {
+    use super::*;
+


I don't know if its easily possible but we should try to refactor this a bit so that we can test it by adding one or two dumps of auxv for arm cpus.

I do not have an arm system handy right now, could somebody else do that?

Yeah just open an issue and tag it here. Otherwise you can also try googling for dumps of auxv, maybe you get lucky :D

Actually it won't work since the auxv entry size is system dependent so the synthetic 32bit tests would fail on 64bit platforms.

We could use #[cfg(target_pointer_width = "32")] to enable the tests on 32-bit platforms or to add different tests for 32 and 64 bit platforms.

My idea is to avoid getauxval completely and just walk from argv in imp::init() and save a GLOBAL_AUXV_PTR there. then wrap the whole thing as it is done for args.

Before we go to the extent of bypassing getauxval() and doing our own unsafe version, I think it would make sense to evaluate whether or not there is actually a problem with getauxval() availability in practice. Are people going to be writing Rust code that uses SIMD that gets deployed on very old Android devices? I suspect that that is likely to be a marginal use case at most.

I assume that the getauxval() support is sorted out already and I looked at how hard would be avoiding it to support the fringe cases I mentioned.

Apparently it would be within the 50-lines range.

@lu-zero I thought that the init call was only available in std, is it also usable from core?

@marshallpierce once getauxval lands things will be in my opinion very good for Linux users and my time is better spent on improving things for macos users (macos x86 users are fine already, but iOS users on ARM chips are not). Having said this, if someone wants to spearhead support for libc's without getauxval on coresimd I'd completely support that (for steed users it might be better to just implement getauxval on steed though). Then there is also arm-thumb users which are currently left a bit in the dark; we might be able to help them in coresimd but it is unclear to me what the best way to do that is (probably offer something like what we offer for x86).

@lu-zero if you want to pursue this a good way to get initial feedback is to fill an issue with your thoughts on the matter, we are kind of mixing many different discussions in this PR, and while many are subscribed to the issues in this repo, most of them are probably not being notified about any of this.

gnzlbg · 2017-12-10T18:29:53Z

Looks very good, thanks! I've left some comments in the code, but they are all minor issues.

gnzlbg · 2017-12-11T11:53:06Z

src/runtime/linux/cpuinfo.rs

+        assert_eq!(cpuinfo.field("cpu"), "POWER8E (raw), altivec supported");
+
+        assert!(cpuinfo.field("cpu").has("altivec"));
+    }


This looks great, thanks! Could you add a second test for a CPU without altivec, like for example this one: https://github.com/randombit/cpuinfo/blob/master/ppc/power5 ?

@lu-zero it looks like the Power5 test got lost in the merge?

Check for neon/asimd and pmull for arm and aarch64.

gnzlbg · 2017-12-12T07:06:18Z

I've opened #234 to track testing auxv. Modulo the Power5 tests that got lost in the merge this LGTM.

Rely mainly on parsing auxv since the cpuinfo information is incomplete.

lu-zero · 2017-12-12T10:27:10Z

I updated patch to add it, I hope it doesn't cause additional disruption

gnzlbg · 2017-12-12T10:54:53Z

@lu-zero thank you for working on this! great job!

marshallpierce · 2017-12-12T13:25:58Z

src/runtime/linux/auxvec.rs

+    // reliable way.
+    pub fn new() -> Result<Self, ::std::io::Error> {
+        use std::io::Read;
+        let mut file = ::std::fs::File::open("/proc/self/auxv")?;


Apparently it is common on Android for this file to not be readable (permissions wise), but getauxval() is widely available because bionic (their libc) has implemented it for some years now.

lu-zero force-pushed the master branch from c491839 to af55577 Compare December 10, 2017 16:16

gnzlbg reviewed Dec 10, 2017

View reviewed changes

lu-zero force-pushed the master branch 3 times, most recently from 02254b9 to d303269 Compare December 11, 2017 11:32

gnzlbg reviewed Dec 11, 2017

View reviewed changes

gnzlbg mentioned this pull request Dec 11, 2017

make __m64/__m128i/__m256i types #232

Merged

lu-zero force-pushed the master branch from d303269 to 02254b9 Compare December 11, 2017 16:58

lu-zero added 2 commits December 11, 2017 18:12

Unbreak detect_features for arm and aarch64

29415a9

Extract the cpu capabilities from the auxiliary vector

0f2fdb1

Check for neon/asimd and pmull for arm and aarch64.

lu-zero force-pushed the master branch from 8ae05d9 to 79b9eee Compare December 11, 2017 18:19

alexcrichton approved these changes Dec 12, 2017

View reviewed changes

Initial PowerPC support

a50f58b

Rely mainly on parsing auxv since the cpuinfo information is incomplete.

lu-zero force-pushed the master branch from e5d673f to a50f58b Compare December 12, 2017 10:25

gnzlbg merged commit 098787d into rust-lang:master Dec 12, 2017

marshallpierce reviewed Dec 12, 2017

View reviewed changes

non-x86 runtime detection updates #229

non-x86 runtime detection updates #229

Conversation

lu-zero commented Dec 10, 2017

alexcrichton commented Dec 10, 2017

Choose a reason for hiding this comment

gnzlbg Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg commented Dec 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg commented Dec 12, 2017

lu-zero commented Dec 12, 2017

gnzlbg commented Dec 12, 2017

Choose a reason for hiding this comment

gnzlbg Dec 10, 2017 •

edited

Loading

gnzlbg Dec 10, 2017 •

edited

Loading

gnzlbg Dec 10, 2017 •

edited

Loading

gnzlbg Dec 11, 2017 •

edited

Loading

gnzlbg Dec 15, 2017 •

edited

Loading