third_party/fiat: replace memcpy with OPENSSL_memcpy

[gaojiaqi7]

Align with the other use of OPENSSL_memcpy in curve25519_64_adx.h. string.h will no longer be needed.

Closes #1777

[briansmith]

First, we don't currently support a sysroot-less build for any x86-64 targets. In build.rs we have:

    if (target.arch == "wasm32")
        || (target.os == "linux" && target.is_musl && target.arch != "x86_64")
    {
        if let Ok(compiler) = c.try_get_compiler() {
            // TODO: Expand this to non-clang compilers in 0.17.0 if practical.
            if compiler.is_like_clang() {
                let _ = c.flag("-nostdlibinc");
                let _ = c.define("RING_CORE_NOSTDLIBINC", "1");
            }
        }
    }

I'm guessing that you use CFLAGS or some other customization to ring's build system that does the equivalent of this, for this target x86_64-unknown-none mentioned in the issue? I am open to taking this change but I would rather we try to do it with specific support for it in build.rs by extending the above logic. The ideal scenerio would be that we extend it to support (target.os == "none" || target.os == "linux") (dropping target.is_musl && target.arch != "x86_64")) so that our existing Linux-targeting builds can test it.

This would make all kinds of cross-compiling more practical and make it so we don't break the build for x86_64-unknown-none and friends by testing that this all works correctly in CI.

WDYT?

[codecov]

Codecov Report

Merging #1778 (b4bdf6a) into main (711d9fc) will increase coverage by 0.00%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1778   +/-   ##
=======================================
  Coverage   96.01%   96.01%           
=======================================
  Files         138      138           
  Lines       20788    20788           
  Branches      226      226           
=======================================
+ Hits        19959    19960    +1     
+ Misses        792      790    -2     
- Partials       37       38    +1     

Files	Coverage Δ
third_party/fiat/curve25519_64_adx.h	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[briansmith]

When I try to build with this change, the build fails because:

 cargo:warning=In file included from crypto/curve25519/curve25519_64_adx.c:22:
  cargo:warning=In file included from crypto/curve25519/../../third_party/fiat/curve25519_64_adx.h:3:
  cargo:warning=In file included from /usr/lib/llvm-16/lib/clang/16/include/immintrin.h:26:
  cargo:warning=In file included from /usr/lib/llvm-16/lib/clang/16/include/xmmintrin.h:31:
  cargo:warning=/usr/lib/llvm-16/lib/clang/16/include/mm_malloc.h:13:10: fatal error: 'stdlib.h' file not found

If I remove #include <immintrin.h> from this file then then the build still fails in poly1305_vec.c:

  cargo:warning=In file included from crypto/poly1305/poly1305_vec.c:32:
  cargo:warning=In file included from /usr/lib/llvm-16/lib/clang/16/include/emmintrin.h:17:
  cargo:warning=In file included from /usr/lib/llvm-16/lib/clang/16/include/xmmintrin.h:31:
  cargo:warning=/usr/lib/llvm-16/lib/clang/16/include/mm_malloc.h:13:10: fatal error: 'stdlib.h' file not found

So I don't think this change on its own is enough.

[briansmith]

See the above comment. It seems other changes are needed to get the intended effect.

[briansmith]

/* This header should only be included in a hosted environment as it depends on
 * a standard library to provide allocation routines. */
#if __STDC_HOSTED__
#include <mm_malloc.h>
#endif

It seems like one solution is to build with -ffreestanding but I am concerned about this from https://stackoverflow.com/questions/17692428/what-is-ffreestanding-option-in-gcc#comment134012992_17692510:

As @nee notes, compiling with -ffreestanding may well result in decreased performance of the compiled code. The reason for this is that the -ffreestanding option implicitly enables -fno-builtin, which means that you cannot benefit from the automatic substitution of any standard library function calls with GCC built-ins, such as memcpy being compiled directly into an inline copy loop. The merits of this design decision aside, it is definitely something that one should be aware of when evaluating the use of -ffreestanding.

[briansmith]

I went ahead and merged this as the scope of the issue isn't really "start supporting this configuration". I also verified with the original author of the code that the use of memcpy vs OPENSSL_memcpy was not intentional.

[gaojiaqi7]

Thanks @briansmith. I am using customization for ring's build. On Linux I don't see any issue with both gcc and clang, but when I use clang on windows, I have to add -ffreestanding flag for cross compilation to x86_64-known-none target otherwise I will see the failure with the mm_malloc.h:

/* This header should only be included in a hosted environment as it depends on
 * a standard library to provide allocation routines. */
#if __STDC_HOSTED__
#include <mm_malloc.h>
#endif

This patch will help me to remove the modifications to the header and only keep the customization for the build system. Thanks a lot :)