-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add msvc support for AESCE #7316
Add msvc support for AESCE #7316
Conversation
AFAICT |
library/aesce.h
Outdated
#define MBEDTLS_HAVE_ARM64 | ||
#endif | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With just this and the change to aesce.c
, I get
Microsoft (R) Program Maintenance Utility Version 14.35.32215.0
Copyright (C) Microsoft Corporation. All rights reserved.
cl -nologo -Ilibrary/ -Iinclude/ -Fobuild\library\ -c ./library\aesce.c
aesce.c
./library\aesce.c(304): error C2440: 'type cast': cannot convert from '__n64' to 'poly64_t'
./library\aesce.c(301): error C2168: 'neon_pmull_64': too few actual parameters for intrinsic function
./library\aesce.c(368): error C2143: syntax error: missing ')' before ':'
NMAKE : fatal error U1077: 'cl -nologo -Ilibrary/ -Iinclude/ -Fobuild\library\ -c ./library\aesce.c' : return code '0x2'
Stop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the project file changes fix that, do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks like the type errors are related to https://developercommunity.visualstudio.com/t/arm64-wrong-and-missing-types-in-arm64-neonh/1590951
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not get this error. What's your build command. It looks like msvc 2022.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using a custom makefile. The command it executed was as shown
cl -nologo -Ilibrary/ -Iinclude/ -Fobuild\library\ -c ./library\aesce.c
The compiler it was running was
c:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.35.32215\bin\HostARM64\ARM64/cl.exe
i.e. Visual Studio 2022, running on ARM64, targeting ARM64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try it.
I did not install arm64 host compiler. but I think it can be reproduced by same version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can reproduce it now. I forget rebase this PR. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that uwp
is a Windows Store build, and might not be targeted by everyone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be a bug of GCC also.
GCC think poly64_t
and poly64x1_t
are different. So cast
is a MUST for GCC. But I think they should be same, no cast
needed
This probably also needs an update to
|
There also needs to be a fix for around line 368 in
|
42bb05e
to
9ea1033
Compare
I just rebased this PR due to gcm compile fail. |
I have tried some msvc versions from vs2022 with bellow command
The versions are
|
ARMCC reports license server error. Others pass |
8894636
to
472cb58
Compare
Rebase to re-trigger CI |
It is replaced with #7286 |
Actually, could we not do that? i.e. let's not have one PR (7286) that does a bunch of completely different things. There should be one PR to replace the Perl scripts with Python, and another to add MSVC support for AESCE |
Agree. I do not think it more. |
22ea3fa
to
c1ad0ae
Compare
- Add msc version check - remove HAVE_ASM due to conflict with check_config Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Do we want to add a #error if the user tries to build for Arm with MBEDTLS_AESCE_C enabled on an old MSVC? As it stands, I think it will silently not build AESCE_C, so if plain C is still enabled, the user will unexpectedly not get AESCE. |
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
When `MBEDTLS_AESCE_C` enabled and the compiler is not expected, we should raise error to user. Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
c67ba96
to
61c4cfa
Compare
Looks good but the gcc check triggers when building with clang |
I think the recommended way to detect gcc but not clang is |
That's what we normally do - but putting |
It would, but the first if condition also has a version check, so we do need the suggested fix. |
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Fixes #6920
This PR is enable MSVC support for AESCE module.
The code is verified under below condition.
platformtoolsetversion=v143
arm_neon.h
reports ARM64 not supportedselftest
Gatekeeper checklist