xen0n
commented
10 months ago 1555
Seems the infra is already there, and one only needs to teach the build system to recognize big-endian MIPS.
Closed brocaar closed 9 months ago
xen0n
commented
10 months ago Seems the infra is already there, and one only needs to teach the build system to recognize big-endian MIPS.
briansmith
commented
10 months ago PR #1859 was submitted to add the big-endian variant. I asked for a couple of changes. If somebody updates that PR or submits a new one with the requested changes made, I will be happy to merge it.
brocaar
commented
10 months ago Thanks @xen0n and @briansmith for your quick responses. I'm happy to work on the requested changes for #1859. If I'm not mistaken, this is what you are asking for: https://github.com/brocaar/ring/commit/9467d8cd6ac9a6ac7acf4c1056b8cd2390755292.
However, it looks like #1859 might still have some issues. For my project I'm using --target mips-unknown-linux-musl. My build fails with many inlining related warnings:
cargo:warning=crypto/fipsmodule/aes/aes_nohw.c: In function 'aes_nohw_transpose':
cargo:warning=crypto/fipsmodule/aes/aes_nohw.c:432:20: error: inlining failed in call to 'aes_nohw_swap_bits.constprop': call is unlikely and code size would grow [-Werror=inline]
cargo:warning= 432 | static inline void aes_nohw_swap_bits(aes_word_t *a, aes_word_t *b,
cargo:warning= | ^~~~~~~~~~~~~~~~~~
cargo:warning=crypto/fipsmodule/aes/aes_nohw.c:454:3: note: called from here
cargo:warning= 454 | aes_nohw_swap_bits(&batch->w[6], &batch->w[7], 0x55555555, 1);
cargo:warning= | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cargo:warning=crypto/fipsmodule/aes/aes_nohw.c:432:20: error: inlining failed in call to 'aes_nohw_swap_bits.constprop': call is unlikely and code size would grow [-Werror=inline]
cargo:warning= 432 | static inline void aes_nohw_swap_bits(aes_word_t *a, aes_word_t *b,
cargo:warning= | ^~~~~~~~~~~~~~~~~~Because of -Werror=inline these are treated as errors.
Any idea if there is something else that needs to be changed?
DavidHorton5339
commented
10 months ago I'm updating PR 1859 with requested changes. Sorry about the delay - other priorities took precedence.
briansmith
commented
10 months ago Because of -Werror=inline these are treated as errors.
@brocaar Are you building optimized for size? And/or using PGO? One thing I can say is that if you are using AES at all then these functions are definitely not "unlikely" and AFAICT really do benefit a lot from inlining, so perhaps it is worth it for us to try to convince the compiler to do the inlining against its own judgement.
brocaar
commented
10 months ago @briansmith, you are right, this was triggered by opt-level set to "z" or "s" (in both cases it would fail). With 3 I can compile my code fine.
vavrecan
commented
10 months ago I had this command
"mips-openwrt-linux-musl-gcc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-Os" "-pipe" "-mno-branch-likely" "-mips32r2" "-mtune=24kc" "-fno-caller-saves" "-fno-plt" "-fhonour-copts" "-msoft-float" "-mips16" "-minterlink-mips16" "-Wformat" "-Werror=format-security" "-fstack-protector" "-D_FORTIFY_SOURCE=1" "-Wl,-z,now" "-Wl,-z,relro" "-fvisibility=hidden" "-std=c1x" "-pedantic" "-Wall" "-Wextra" "-Wbad-function-cast" "-Wcast-align" "-Wcast-qual" "-Wconversion" "-Wenum-compare" "-Wfloat-equal" "-Wformat=2" "-Winline" "-Winvalid-pch" "-Wmissing-field-initializers" "-Wmissing-include-dirs" "-Wnested-externs" "-Wredundant-decls" "-Wshadow" "-Wsign-compare" "-Wsign-conversion" "-Wstrict-prototypes" "-Wundef" "-Wuninitialized" "-Wwrite-strings" "-g3" "-DNDEBUG" "-Werror" ... and what helped was to add -Wno-error=inline
briansmith
commented
9 months ago Does anybody here have a suggestion for how we can annotate the code in aes_nohw.c to tell the compiler that, even if the compiler optimization setting is to optimize for size by default, that it should do this inlining and not worry if it would grow the code?
brocaar
commented
9 months ago @briansmith I'm not sure if this is related to my development environment or applies to all, but I noticed that when using [patch.crates-io] this issue occured and without not, even when the line under [patch.crates-io] would point to the latest ring version on crates.io. So maybe this is not an issue at all?
DavidHorton5339
commented
9 months ago @brocaar the difference you might have noticed is that referencing crates.io uses prebuilt version of ring. If you patch it (to point to a git reference or local drive) you will cause code to be built - and any compiler warnings generated.
DavidHorton5339
commented
9 months ago @briansmith __attribute__((always_inline)) might help (and remove inline keyword). This works differently from inline keyword, in terms of its coupling to optimisation levels. So, I think it's worth a try.
BTW regarding portability the build already seems wedded to GNU-specific extensions elsewhere, so I'd be less worried about this than in otherwise portable code.
briansmith
commented
9 months ago @briansmith I'm not sure if this is related to my development environment or applies to all, but I noticed that when using
[patch.crates-io]this issue occured and without not, even when the line under[patch.crates-io]would point to the latestringversion on crates.io. So maybe this is not an issue at all?
We only enable -Werror when building from Git (.git is present) to avoid the build failing unnecessarily when people are building from crates.io. Search build.rs for .git to see what I mean. The idea is to be stricter during development without risking build breakage "in production." Admittedly, it doesn't work well for certain ways of vendoring though.
DavidHorton5339
commented
9 months ago New PR https://github.com/briansmith/ring/pull/1894 raised for MIPS BE support. @briansmith Unfortunately when I tested the latest code I found a couple of issues that affect not only MIPS but other targets too: https://github.com/briansmith/ring/issues/1895 https://github.com/briansmith/ring/issues/1896 As they don't just affect MIPS BE I've reported them separately from my PR.
briansmith
commented
9 months ago I merged a slightly tweaked versoin of @DavidHorton5339's PR as part of #1910. I also added all the supported MIPS targets to ring's CI, tested only on the Rust 1.71.0 toolchain. I expect this will be released as part of ring 0.17.8 next week.
Ragnt
commented
8 months ago @briansmith any word on that 0.17.8 release with these changes on crates.io?
deadash
commented
6 months ago To enable compilation for MIPS64, you only need to add the following code under the existing code in include/ring-core/target.h:
#elif defined(__MIPSEB__) && !defined(__LP64__)
#define OPENSSL_32_BITAdd this:
#elif defined(__MIPSEB__) && defined(__LP64__)
#define OPENSSL_64_BITThis will configure the system to use MIPS64.
Testing with ring v0.17.7, compiling for
mipsel-works fine (e.g.mipsel-unknown-linux-musl), however compiling formips-(e.g.mips-unknown-linux-musl) returns with an error:Full error
``` The following warnings were emitted during compilation: warning: In file included from include/ring-core/base.h:74, warning: from include/ring-core/mem.h:60, warning: from crypto/curve25519/curve25519.c:22: warning: include/ring-core/target.h:63:2: error: #error "Unknown target CPU" warning: 63 | #error "Unknown target CPU" warning: | ^~~~~ warning: In file included from crypto/curve25519/internal.h:20, warning: from crypto/curve25519/curve25519.c:24: warning: crypto/curve25519/../internal.h:211:2: error: #error "Must define either OPENSSL_32_BIT or OPENSSL_64_BIT" warning: 211 | #error "Must define either OPENSSL_32_BIT or OPENSSL_64_BIT" warning: | ^~~~~ warning: crypto/curve25519/../internal.h:224:15: error: unknown type name 'crypto_word_t' warning: 224 | static inline crypto_word_t value_barrier_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:224:45: error: unknown type name 'crypto_word_t' warning: 224 | static inline crypto_word_t value_barrier_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:236:15: error: unknown type name 'crypto_word_t' warning: 236 | static inline crypto_word_t constant_time_msb_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:236:49: error: unknown type name 'crypto_word_t' warning: 236 | static inline crypto_word_t constant_time_msb_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:241:15: error: unknown type name 'crypto_word_t' warning: 241 | static inline crypto_word_t constant_time_is_zero_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:241:53: error: unknown type name 'crypto_word_t' warning: 241 | static inline crypto_word_t constant_time_is_zero_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:256:15: error: unknown type name 'crypto_word_t' warning: 256 | static inline crypto_word_t constant_time_is_nonzero_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:256:56: error: unknown type name 'crypto_word_t' warning: 256 | static inline crypto_word_t constant_time_is_nonzero_w(crypto_word_t a) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:261:15: error: unknown type name 'crypto_word_t' warning: 261 | static inline crypto_word_t constant_time_eq_w(crypto_word_t a, warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:261:48: error: unknown type name 'crypto_word_t' warning: 261 | static inline crypto_word_t constant_time_eq_w(crypto_word_t a, warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:262:48: error: unknown type name 'crypto_word_t' warning: 262 | crypto_word_t b) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:269:15: error: unknown type name 'crypto_word_t' warning: 269 | static inline crypto_word_t constant_time_select_w(crypto_word_t mask, warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:269:52: error: unknown type name 'crypto_word_t' warning: 269 | static inline crypto_word_t constant_time_select_w(crypto_word_t mask, warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:270:52: error: unknown type name 'crypto_word_t' warning: 270 | crypto_word_t a, warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:271:52: error: unknown type name 'crypto_word_t' warning: 271 | crypto_word_t b) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:283:46: error: unknown type name 'crypto_word_t' warning: 283 | static inline uint8_t constant_time_select_8(crypto_word_t mask, uint8_t a, warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:300:59: error: unknown type name 'crypto_word_t' warning: 300 | const crypto_word_t mask) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h: In function 'constant_time_conditional_memcpy': warning: crypto/curve25519/../internal.h:305:14: warning: implicit declaration of function 'constant_time_select_8' [-Wimplicit-function-declaration] warning: 305 | out[i] = constant_time_select_8(mask, in[i], out[i]); warning: | ^~~~~~~~~~~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:305:14: warning: nested extern declaration of 'constant_time_select_8' [-Wnested-externs] warning: crypto/curve25519/../internal.h: At top level: warning: crypto/curve25519/../internal.h:314:59: error: unknown type name 'crypto_word_t' warning: 314 | const crypto_word_t mask) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h: In function 'constant_time_conditional_memxor': warning: crypto/curve25519/../internal.h:319:15: warning: implicit declaration of function 'value_barrier_w' [-Wimplicit-function-declaration] warning: 319 | out[i] ^= value_barrier_w(mask) & in[i]; warning: | ^~~~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:319:15: warning: nested extern declaration of 'value_barrier_w' [-Wnested-externs] warning: crypto/curve25519/../internal.h: At top level: warning: crypto/curve25519/../internal.h:351:15: error: unknown type name 'crypto_word_t' warning: 351 | static inline crypto_word_t constant_time_declassify_w(crypto_word_t v) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/../internal.h:351:56: error: unknown type name 'crypto_word_t' warning: 351 | static inline crypto_word_t constant_time_declassify_w(crypto_word_t v) { warning: | ^~~~~~~~~~~~~ warning: crypto/curve25519/curve25519.c: In function 'x25519_ge_scalarmult_small_precomp': warning: crypto/curve25519/curve25519.c:725:35: warning: implicit declaration of function 'constant_time_eq_w' [-Wimplicit-function-declaration] warning: 725 | cmov(&e, &multiples[j-1], 1&constant_time_eq_w(index, j)); warning: | ^~~~~~~~~~~~~~~~~~ warning: crypto/curve25519/curve25519.c:725:35: warning: nested extern declaration of 'constant_time_eq_w' [-Wnested-externs] error: failed to run custom build command for `ring v0.17.7` Caused by: process didn't exit successfully: `/target/release/build/ring-d0f3c232ff4d944c/build-script-build` (exit status: 1) --- stdout cargo:rerun-if-env-changed=RING_PREGENERATE_ASM cargo:rustc-env=RING_CORE_PREFIX=ring_core_0_17_7_ OPT_LEVEL = Some("z") TARGET = Some("mips-unknown-linux-musl") HOST = Some("x86_64-unknown-linux-gnu") cargo:rerun-if-env-changed=CC_mips-unknown-linux-musl CC_mips-unknown-linux-musl = None cargo:rerun-if-env-changed=CC_mips_unknown_linux_musl CC_mips_unknown_linux_musl = Some("mips-linux-muslsf-gcc") cargo:rerun-if-env-changed=CRATE_CC_NO_DEFAULTS CRATE_CC_NO_DEFAULTS = None DEBUG = Some("false") CARGO_CFG_TARGET_FEATURE = None cargo:rerun-if-env-changed=CFLAGS_mips-unknown-linux-musl CFLAGS_mips-unknown-linux-musl = None cargo:rerun-if-env-changed=CFLAGS_mips_unknown_linux_musl CFLAGS_mips_unknown_linux_musl = None cargo:rerun-if-env-changed=TARGET_CFLAGS TARGET_CFLAGS = None cargo:rerun-if-env-changed=CFLAGS CFLAGS = None ```@briansmith, looking at https://github.com/briansmith/ring/blob/main/include/ring-core/target.h#L40, I only see
__MIPSEL__definitions. Looking at https://github.com/briansmith/ring/issues/562#issuecomment-407131724, it looks like issue #562 was intended to also implement themips-targets, but maybe this has changed or this was overlooked?