ring icon indicating copy to clipboard operation
ring copied to clipboard

Conditional all uses of external C/assembly files on whether they are actually compiled

Open briansmith opened this issue 9 months ago • 2 comments

In build.rs we have some logic for determining whether/which assembly language source files to compile. Separately, we have other logic within the source code to determine whether to use functions within those source files. For the most part this logic syncs up. However, there are notable cases where it doesn't match. In particular, when compiling for an operating system that isn't "Linux compatible" and isn't otherwise explicitly supported within build.rs, we won't build the assembly sources. But, on targets for which we do dynamic feature detection, x86-64 in particular, we assume that the assembly was compiled. Then we get missing symbols.

The way things worked made more sense in the past because we didn't have fallback C/Rust implementations of every function. But nowadays, we do. Consequently we can (with suboptimal performance) "support" every target by "just" using the fallback implementations all the time. To do this, we need to communicate from build.rs whether certain assembly language source files were compiled.

Increasingly it seems it won't be the case that it is all-or-nothing either. That is, it isn't going to be the case that all x86-64 assembly source files are compiled or none of them are. Currently we handle this on an ad-hoc basis. For example, for x86-64 Windows build.rs skips all non-PerlAsm .S files, and then in the source code we guard any use of functions defined in those non-PerlAsm .S files with #[cfg(all(target_arch = "x86_64", not(target_os = "windows")))]. This doesn't scale.

We could instead do the following:

For every (group of) source files, hereafter "source pack," define a cfg directive. For example:

Sources {
    cfg_directive: "curve25519_adx",
    target_arches = &[X86_64], 
    sources: &[
        "third_party/fiat/asm/fiat_curve25519_adx_mul.S",
        "third_party/fiat/asm/fiat_curve25519_adx_square.S",
        "crypto/curve25519/curve25519_64_adx.c",
    ],
}

Note that an "assembly" pack might sometimes also contain C files.

Emit cargo:rustc-cfg={cfg_directive} from build.rs when we compile a source pack.

Within the source code, change all the #cfg(target_arch = "") to instead be cfg(cfg_directive).

If multiple source packs implement a common API, then the source packs can share a common cfg_directive value. For example, for SHA-2 we have assembly implementations for 4 target architectures that all end up exposing a sha{256,512}_block_data_order function, so these source packs can all share same "sha_asm" cfg_directive value.

All of the uses of #[cfg(target_arch = "...")] need to be replaced by use of these cfg directives, except uses in tests or test-like const _XXX: () = assert!(....) that are verifying that certain features are available for certain targets. In particular, the definition of the features! macro in arm.rs needs to be redone so that it considers not just target_arch and target_feature but also each individual cfg_directive that indicates whether the assembly code was actually assembled.

briansmith avatar Nov 28 '23 20:11 briansmith

All of the uses of #[cfg(target_arch = "...")] need to be replaced by use of these cfg directives,

@pixlwave noted in PR #1914 that there is a target, arm64_32-apple-watchos, which is an Aarch64 target with target_pointer_width="32", so all of the target_arch = "aarch64" conditional logic is wrong for this target, as I think the 64-bit assembly code will only work for 64-bit pointers. I don't see any special assembly support in upstream OpenSSL either. So in order to properly support arm64_32 targets we probably need to make at least some minimal progress on this issue to replace the target_arch="aarch64" conditions at a minimum.

briansmith avatar Jan 16 '24 01:01 briansmith

We have defined LINUX_ABI and MACOS_ABI in build.rs to provide a shorthand for AsmTargets that are "Linux-like" and "MacOS-like". However, these have oversimplified the situation and created an attractive nuisance where people just append an OS to the list to get it to build. As part of this work, we should remove these shortcuts.

briansmith avatar Jan 16 '24 03:01 briansmith