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

Linux ARM (aarch64 and 32-bit) feature detection should avoid file I/O and dlsym #655

Closed
briansmith opened this issue Jan 28, 2019 · 11 comments
Labels

Comments

@briansmith
Copy link

If getauxval() is provided by a statically-linked libc, like musl C, I think dlsym might not find it. (I'm not 100% sure on this.) If weak linkage is necessary then I think relying on the linker to do the weak linkage (using [linkage = "extern_weak"] if that's stable) would be better.

For -musl targets in particular, getauxval() should just be assumed to be available unless/until somebody mentions a real need to support old, old versions of musl.

For Android targets, Google now requires developers to target API level 26 (see https://developer.android.com/distribute/best-practices/develop/target-sdk) so IMO we should be able to always assume getauxval() is available for all Android targets.

For iOS targets, AFAICT no runtime detection is needed because iOS devices are guaranteed to be AAarch64 with crypto and SIMD extensions enabled.

For AAarch64 Linux targets, I think it is reasonable to require getauxval() be available, unless/until somebody brings a real-world example of where they are targeting AAarch64 with an ancient glibc, simply because AAarch64 itself is newer than the version of glibc that has getauxval().

I also wonder if anybody is really targeting 32-bit ARM Linux with system glibc that is so old that it doesn't provide getauxval(). (This situation is different than x86_64 and x86 where we already know that there are LTS Linux distros that have ancient libcs.) IMO it would be better to be aggressive in simplifying the implementation to require getauxval() statically at link time unless/until somebody brings a real use case.

At the very least, it would be useful for me to have some way to disable the file I/O and parsing of /proc/self/auxv and/or /proc/cpuinfo in a crate depending on stdsimd directly, e.g. with a feature flag.

This would enable me to remove my own getauxval() code in ring in favor of using stdsimd on AAarch64/ARM Linux.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 28, 2019

Thank you @briansmith , I think we can do most of this.

For Android targets, Google now requires developers to target API level 26 (see https://developer.android.com/distribute/best-practices/develop/target-sdk) so IMO we should be able to always assume getauxval() is available for all Android targets.

I think servo is still supporting a very old version of Android. cc @SimonSapin - do you have any thoughts about the minimum version that the std library should support here ?

For iOS targets, AFAICT no runtime detection is needed because iOS devices are guaranteed to be AAarch64 with crypto and SIMD extensions enabled.

If these features are enabled at compile-time, no run-time detection will be performed. AFAIK there is no way to perform run-time feature detection in iOS anyways. If this changes, we'll have to use an iOS SDK API for this. It might be worth it to triple-check that the Rust target file for aarch64-apple-ios has these features as always enabled.

For AAarch64 Linux targets, I think it is reasonable to require getauxval() be available, unless/until somebody brings a real-world example of where they are targeting AAarch64 with an ancient glibc, simply because AAarch64 itself is newer than the version of glibc that has getauxval().

Modern Linux kernels support mrs emulation. We are considering using that instead of getauxval() here: #642 (we should branch this discussion there).

I also wonder if anybody is really targeting 32-bit ARM Linux with system glibc that is so old that it doesn't provide getauxval().

cc @japaric ?

At the very least, it would be useful for me to have some way to disable the file I/O and parsing of /proc/self/auxv and/or /proc/cpuinfo in a crate depending on stdsimd directly, e.g. with a feature flag.

I think we can add some cargo features to the std_detect crate for this, e.g., parse_auxv, parse_cpuinfo, that are enabled by default. Disabling these features could disable support for these.

@briansmith
Copy link
Author

I think we can add some cargo features to the std_detect crate for this, e.g., parse_auxv, parse_cpuinfo, that are enabled by default. Disabling these features could disable support for these.

I suggest we first figure out if we can do without these features. In the end most people will access this functionality through libstd (I would prefer to do that, if/when it is stable), and libstd wouldn't expose such ability to disable the file I/O.

The file I/O and parsing isn't the end of the world but if we can do without it then the simplification would be a worthwhile improvement to robustness.

@SimonSapin
Copy link
Contributor

Right now Servo uses NDK Revision 12b to build Rust and C++ code for Android. I’m not sure about the SDK version, or what that means for getauxval(). We have servo/servo#20184 about upgrading but that hasn’t been a priority so far.

@briansmith
Copy link
Author

See cross-rs/cross#253 (comment) for more details on getauxval() availability for 32-bit ARM.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 5, 2019

@briansmith

I suggest we first figure out if we can do without these features. In the end most people will access this functionality through libstd (I would prefer to do that, if/when it is stable), and libstd wouldn't expose such ability to disable the file I/O.

If somebody needs to target, e.g., an old linux version (for whatever reason), it would be great if they could just recompile libstd with xargo (or in the future with cargo) and enable a cargo feature for that. While this is quite a hassle today, there is ongoing work to fix that. There have also been talks about handling OS versions in the target triple "somehow", e.g., such that x86_64-unknown-linux-gnu-X.Y.Z would target version X.Y.Z of the platform. I'd rather wait a bit to see how these initiatives turn out before removing functionality that would make them harder to pursue (having to re-add this functionality to libstd would be yet another thing that these initiatives would need to do).

I agree with you that, for most users, using dlsym (or doing file i/o as a fallback) for run-time feature detection is not perfect, and that just assuming that getauxval is available and calling that directly and unconditionally would be better. However, run-time feature detection happens at most once in an application, so the current approach isn't horrible either.

I encourage work on gating std_detect functionality appropriately behind cargo features that are enabled by default (e.g. dlsym, file_io, getauxval, etc.). This should allow #[no_std] projects to use std_detect without depending on libstd (e.g. for file_io), which IMO would add most of the value.

Once that is done, we can explore changing the defaults for the std_detect crate, and possibly libstd.

@briansmith
Copy link
Author

For Android targets, Google now requires developers to target API level 26

This is wrong. Developers can build apps that target and run on earlier versions, but they have to opt into API level 26 breaking changes. Sorry for the misinformation.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 11, 2019

Note that libc is only tested with API level 24 for arm, armv7, and aarch64 targets, which in practice means that earlier versions are essentially unsupported (they might work, or they might break, depending on what you are trying to do). It probably makes sense to do the same for this module, and that probably will happen when we move CI to Azure Pipelines.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 11, 2019

Having said that, I still see no reason to break those trying to target older versions.

If you want to avoid file I/O and dlsym, you can do that already by just using the std_detect from crates.io and disabling these features.

@briansmith
Copy link
Author

Note that libc is only tested with API level 24 for arm, armv7, and aarch64 targets, which in practice means that earlier versions are essentially unsupported (they might work, or they might break, depending on what you are trying to do).

Any idea why? I know of serious commercial Rust applications targeting Android 5+. Android 5 is API level 21. It would be great if we could make API level 21 the minimum supported version if practical.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 11, 2019

There are serious projects targeting Android API level 14 as well, and all Android API versions since.

Rust can currently properly support a single API level. There are no different android targets per API level or NDK version.

Libc needs to pick one version to test against. All older and newer versions aren't tested. Which version to pick is arbitrary, but motivated by us wanting to test most of the exposed APIs, some of which are only available in newer versions.

This doesn't mean that other API levels don't work, it just means that they aren't tested. Code written for older versions tends to work on newer ones, but the opposite isn't generally guaranteed. In practice, the main risk is probably that they might end up using a new API that isn't available in the API level they are targeting.

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 31, 2019

I'm closing this not because it has been resolved, but because it cannot be resolved here.

The Rust toolchain supports a minimum version of particular targets. If such a minimum version supports a better API, we can use it, but it might be better to open different separate issues for that.

We can't bump the minimum supported platform version here. If that makes sense for some platform, it makes more sense to open an issue in rust-lang/rust upstream.

In the meantime, users can re-compile libstd with certain std_detect cargo features to choose different behaviors if they know their binaries will only run on newer versions of a supported platform. There is also a version of std_detect on crates.io, but that version is not on the path towards stabilization anymore, and will stop receiving updates. Since nightly is required anyways for configuring this behavior, it is more consistent to do so for the whole binary by re-compiling libstd.
Those #![no_std] users that want to use a particularly configured run-time feature detection run-time can just use stdarch master branch with the latest toolchain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants