Compile error for AES-NI with MSVC on ARM32 (UWP), AES-NI intrinsics are only on x86 and x64

[akien-mga]

Summary

Since upgrading to 2.28.3 (from 2.28.0), Godot 3.x builds for UWP fail for the ARM32 target:

cl /Fothirdparty\mbedtls\library\aesni.uwp.opt.32.arm.obj /c thirdparty\mbedtls\library\aesni.c /std:c++14 /nologo /MD /O2 /GL /utf-8 /FS /MP /GS /Zc:wchar_t /Gm- /fp:precise /errorReport:prompt /Zc:forScope /Gd /EHsc /nologo /AI "C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\\vcpackages" /AI "C:\Program Files (x86)\Windows Kits\10\\References\CommonConfiguration\Neutral" /EHsc /w /AI "C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\lib/store/references" /AI "C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\lib/x86/store/references" /DNO_EDITOR_SPLASH /DUWP_ENABLED /DWINDOWS_ENABLED /DTYPED_METHOD_BIND /DGLES_ENABLED /DGL_GLEXT_PROTOTYPES /DEGL_EGLEXT_PROTOTYPES /DANGLE_ENABLED /DWINVER=0x0602 /D_WIN32_WINNT=0x0602 /DWIN32 /D__WRL_NO_DEFAULT_LIB__ /DPNG_ABORT=abort /D_UNICODE /DUNICODE /DWINAPI_FAMILY=WINAPI_FAMILY_APP /DNDEBUG /DPTRCALL_ENABLED /DMINIZIP_ENABLED /DZSTD_STATIC_LINKING_ONLY /DXAUDIO2_ENABLED /Ithirdparty\mbedtls\include /Ithirdparty\libpng /Ithirdparty\zstd /Ithirdparty\zlib /Iplatform\uwp /Idrivers\windows /IC:\angle\include /I.
C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\include\immintrin.h(17): fatal error C1189: #error:  This header is specific to X86 and X64 targets
scons: *** [thirdparty\mbedtls\library\aesni.uwp.opt.32.arm.obj] Error 2

library/aesni.c includes immintrin.h inconditionally on _MSC_VER, and MSVC 14 / VS 2017 at least doesn't seem to like this on ARM.

This seems consistent with the MSDN documentation for VS 2022, which lists AESNI intrinsics for x64-amd64 and x86, but not ARM and ARM64:

• https://learn.microsoft.com/en-us/cpp/intrinsics/arm-intrinsics?view=msvc-170
• https://learn.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics?view=msvc-170
• https://learn.microsoft.com/en-us/cpp/intrinsics/x86-intrinsics-list?view=msvc-170
• https://learn.microsoft.com/en-us/cpp/intrinsics/x64-amd64-intrinsics-list?view=msvc-170

I haven't bisected nor tested latest master branch, but looking at the code I believe the same issue exists there too.
I believe it's related to recent work on adding support for AES-NI intrinsics on MSVC, which was apparently done unconditionally in d671917 and follow-ups.

mbedtls/library/aesni.c

Lines 33 to 42 in 5859098

	#if defined(MBEDTLS_AESNI_HAVE_CODE)
	#if MBEDTLS_AESNI_HAVE_CODE == 2
	#if !defined(_WIN32)
	#include <cpuid.h>
	#else
	#include <intrin.h>
	#endif
	#include <immintrin.h>
	#endif

mbedtls/library/aesni.h

Lines 57 to 61 in 5859098

	#if defined(_MSC_VER)
	/* Visual Studio supports AESNI intrinsics since VS 2008 SP1. We only support
	* VS 2013 and up for other reasons anyway, so no need to check the version. */
	#define MBEDTLS_AESNI_HAVE_INTRINSICS
	#endif

For Godot, I used this patch which works it around successfully:

diff --git a/thirdparty/mbedtls/include/mbedtls/aesni.h b/thirdparty/mbedtls/include/mbedtls/aesni.h
index 6741dead05..6c545bd4a3 100644
--- a/thirdparty/mbedtls/include/mbedtls/aesni.h
+++ b/thirdparty/mbedtls/include/mbedtls/aesni.h
@@ -54,9 +54,10 @@
  *       macros that may change in future releases.
  */
 #undef MBEDTLS_AESNI_HAVE_INTRINSICS
-#if defined(_MSC_VER)
+#if defined(_MSC_VER) && (defined(_M_AMD64) || defined(_M_IX86))
 /* Visual Studio supports AESNI intrinsics since VS 2008 SP1. We only support
- * VS 2013 and up for other reasons anyway, so no need to check the version. */
+ * VS 2013 and up for other reasons anyway, so no need to check the version.
+ * Only supported on x64 and x86. */
 #define MBEDTLS_AESNI_HAVE_INTRINSICS
 #endif
 /* GCC-like compilers: currently, we only support intrinsics if the requisite

While I've experienced this trying to compile for UWP ARM, it may be reproducible with "Win32" builds targeting ARM and ARM64 too.

System information

Mbed TLS version (number or commit id): 2.28.4
Operating system and version: It's complicated :) Fedora 36 container running Visual Studio 2017 / MSVC 14.16.27023 with Wine. I can't easily test different configurations, but I believe the issue is fairly explicit from the MSDN documentation.
Configuration (if not default, please attach mbedtls_config.h): https://github.com/godotengine/godot/blob/3.x/thirdparty/mbedtls/include/mbedtls/config.h
Compiler and options (if you used a pre-built binary, please indicate how you obtained it): MSVC 14.16.27023, see log output above for options.
Additional environment information: This was seen downstream in the Godot project, which builds mbedtls from source via its own SCons buildsystem. Relevant buildsystem config: https://github.com/godotengine/godot/blob/3.x/modules/mbedtls/SCsub#L12-L107

Expected behavior

aesni.c should compile with MSVC on ARM.

Actual behavior

Fails to build from source.

Steps to reproduce

Possibly (untested), try to compile mbedtls 2.28.4 (or latest 3.x) for the UWP target, ARM arch, with recent MSVC on Windows 10 or 11.

More specific to reproduce it in Godot, it should be possible following those steps: https://docs.godotengine.org/en/3.6/development/compiling/compiling_for_uwp.html
With Godot's 3.x branch: https://github.com/godotengine/godot/tree/3.x

My specific reproduction steps are more complex and involved Godot's build server and containers.

Additional information

Happy to make a PR if my above patch seems useful. I'm not too familiar with MSVC intrinsics so I'm not confident this is the absolute correct solution.

[davidhorstmann-arm]

This may be the same issue as in #7704 (however we should keep this issue open as it adds more detail)

[akien-mga]

My above patch seems to work for Godot on UWP, I'll submit a PR for discussion.

[akien-mga]

Actually, I checked the state in the development branch to make a patch, and it seems the bug was already fixed there by @yuhaoth in #7384. The PR was marked as not relevant to backport, but it seems to be nowadays.

That seems like the better option instead of an ad hoc patch like I suggested, so 2.28 and development don't diverge unnecessarily.

Edit: Ouch that PR is big, I guess not all of it is relevant for 2.28.
I found at least:
8189f32
35b59d7
e62ff09

@yuhaoth might have a clearer overview of what's needed exactly.

[tom-cosgrove-arm]

Indeed, #7384 is a new feature, so won't be back-ported.

[gilles-peskine-arm]

Indeed, it used to be the case that enabling MBEDTLS_AESNI_C on a platform other than x86-64 was a no-op, and we broke that when we added support for AESNI through intrinsics rather than assembly (which we did primarily to support MSVC in addition to GCC-like compilers).

Your proposed fix looks good but incomplete: we're only activating the assembly implementation on x86_64 (not x86_32 because it would need different assembly code), so we should do the same (plus we might as well allow x86_32) for intrinsics. So the next block with __GNUC__ should also be conditioned to x86.

[lpy4105]

I've raised a PR(#8209) to fix the issue.

[lpy4105]

I have checked that on development branch there is no such problem for AESNI (resolved by #7384). But I found issue on development when building AESCE with MSVC(visual studio 2022, _MSC_VER=1937) for ARM64 target, see the following error:

Error (active)  E0035   #error directive: "Target does not support NEON instructions" mbedTLS C:\Users\lpy4105\projects\test\mbedtls\library\aesce.c        72
Error (active)  E0035   #error directive: "Required feature(__ARM_FEATURE_AES) is not enabled." mbedTLS C:\Users\lpy4105\projects\test\mbedtls\library\aesce.c   92

It seems like some __ARM_XXX macros (e.g. __ARM_NEON) is not defined by MSVC?

[gilles-peskine-arm]

Resolved by #8209