-
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
Fix for arm64_32 (aka ILP32) on Clang #7808
Fix for arm64_32 (aka ILP32) on Clang #7808
Conversation
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
Possibly something like |
Im getting this error:
|
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
@paulocoutinhox does the latest version help? |
The latest version isn't working on arm64. I suspect we need three cases here (maybe 4 with thumb? Did we thoroughly test that module like we did with bn_mul?): arm/aarch32, aarch64, and arm64_32 with a different mixture of register sizes and C type sizes than the other two. |
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
I've tested the existing code (via qemu) on thumb1, thumb2, arm and aarch64 (it hits the asm codepath and works correctly for all these cases). The only types it uses are a pointer to char, and If the latest variant doesn't work, I'll simply disable the aarch64 asm for ILP32 (i.e., where |
We don't have a compiler in the CI that will build for watchos, but I've been able to test this approach locally with the following test program:
and then compile with
The error is reproduced in the aarch64 version (error at line 13), but is fixed for the ILP32 version (no error at line 6):
|
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
library/constant_time.c
Outdated
@@ -79,7 +80,12 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi | |||
#if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) | |||
asm volatile ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); | |||
#elif defined(__aarch64__) | |||
#if (SIZE_MAX == 0xffffffff) |
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 don't understand how SIZE_MAX
matters here. Is it a proxy for the size of a pointer? If so, this needs at the very least a comment to explain. And it's wrong for targets with 64-bit pointers but 32-bit size_t
, which is possible in theory but I don't know if any actually exist on arm. But then why not use UINTPTR_MAX
? It doesn't actually have to match sizeof(unsigned char *)
, but I think it's less likely to diverge if the choice is 32-bit vs 64-bit.
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 don't think UINTPTR_WIDTH
is in C99 (although we only care about GNUC compilers here so maybe that's OK). UINTPTR_MAX
could work.
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.
WIDTH/MAX: yes, I realized immediately after posting.
Also a minor thing: these two lines should be #elif
.
Signed-off-by: Dave Rodgman <dave.rodgman@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.
Looks good to me except that since this is fixing a build error (not just a warning) I think this requires a changelog entry. Never mind I was looking at the wrong tab.
This makes sense to me and I have verified that the following works on Linux with official Clang-15 packages from llvm.org:
- Patch out some header inclusions because I don't have suitable headers.
diff --git a/library/alignment.h b/library/alignment.h index 41823485ad..acae9f618a 100644 --- a/library/alignment.h +++ b/library/alignment.h @@ -24,8 +24,9 @@ #define MBEDTLS_LIBRARY_ALIGNMENT_H #include <stdint.h> -#include <string.h> -#include <stdlib.h> +//#include <string.h> +//#include <stdlib.h> +void *memcpy(void *, const void *, unsigned long); /* * Define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS for architectures where unaligned memory diff --git a/library/constant_time.c b/library/constant_time.c index f7da39f8e1..a0063e8996 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -45,7 +45,7 @@ #include "constant_time_invasive.h" #endif -#include <string.h> +//#include <string.h> #if defined(MBEDTLS_USE_PSA_CRYPTO) #define PSA_TO_MBEDTLS_ERR(status) PSA_TO_MBEDTLS_ERR_LIST(status, \ psa_to_ssl_errors, \
- Configure out the bits that need more stdlib:
scripts/config.py crypto scripts/config.py unset MBEDTLS_RSA_C scripts/config.py unset MBEDTLS_HAVE_TIME_DATE scripts/config.py unset MBEDTLS_HAVE_TIME
- Build:
clang-15 -I include -I library -target arm64_32-apple-watchos4.0 -Wall -Wextra -Werror -Os -c library/constant_time.c clang-15 -I include -I library -target arm64_32-apple-watchos4.0 -Wall -Wextra -Werror -O3 -c library/constant_time.c clang-15 -I include -I library -target arm64_32-apple-watchos4.0 -Wall -Wextra -Werror -S library/constant_time.c
I looked at the unoptimized assembly and it seems sensible to me (but I'm not an expert). It assumes that unaligned access is ok, which is already the case for the existing arm and aarch64 variants, so that's fine.
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 except maybe the changelog entry needs minor tweaks.
ChangeLog.d/fix-ilp32.txt
Outdated
@@ -0,0 +1,4 @@ | |||
Bugfix | |||
* Fix a compile failure in the constant_time module when building |
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.
Minor, not worth updating just for that:
* Fix a compile failure in the constant_time module when building | |
* Fix a compilation failure in the constant_time module when building |
ChangeLog.d/fix-ilp32.txt
Outdated
@@ -0,0 +1,4 @@ | |||
Bugfix | |||
* Fix a compile failure in the constant_time module when building | |||
for watchos (i.e. for Aarch64 ILP32). Reported by Paulo Coutinho |
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.
Minor, perhaps updating just for that: watchos is not really material here, what's material is arm64_32. Suggested wording: “when building for arm64_32, e.g. for watchOS”.
That's not an assumption - it's gated by |
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
library/common.h
Outdated
@@ -181,7 +181,7 @@ inline void mbedtls_xor(unsigned char *r, const unsigned char *a, const unsigned | |||
#define MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT "p" | |||
#elif UINTPTR_MAX == 0xfffffffffffffffful | |||
/* Normal case (64-bit pointers): use "r" as the constraint for pointer operands to asm */ | |||
#define MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT "r" | |||
#define MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT "p" |
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 tried that locally (as before, commenting out the inclusion of string.h
) and both work:
aarch64-linux-gnu-gcc -I include -I library -Wall -Wextra -Werror -S library/constant_time.c
clang-15 -I include -I library -target aarch64 -Wall -Wextra -Werror -S library/constant_time.c
I don't know what the problem is in the Travis build.
According to godbolt ( https://gcc.godbolt.org/z/sWooPKd1n ), clang trunk recognises 'p' but does not recognise '+p'. According to docs & local testing, armclang does not recognise 'p' at all. So I think we need the option to use 'r' for armclang. I am not sure what we can do for clang ILP32, other than turn off the bn_mul.h asm. I'm assuming that it really does need to update the pointer value - will check this. |
5ca4696
to
f69c57d
Compare
Those two operands do not need to be written to, so I've moved them into the inputs section and avoided the need to use '+p'. 'p' probably is the preferred option for this kind of parameter, with an exception for armclang, but it feels safer to make the minimal change (i.e., only use 'p' for ILP32 where it seems needed). As it stands it won't build on armclang for ILP32 (it will try to use the 'p' constraint which armclang doesn't recognise), but I don't know if this is even a supported target for armclang. |
The latest commit work for me when compile for watchOS. For tvOS:
For macOS:
|
This is not correct - they are updated so they need to be outputs. |
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
f69c57d
to
e6c9996
Compare
Casting to |
Co-authored-by: Tom Cosgrove <tom.cosgrove@arm.com> Signed-off-by: Dave Rodgman <dave.rodgman@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
@paulocoutinhox Would you mind checking that the latest version works for you? |
Sure, I will do and post here
|
Hi, Im testing now the latest commit and get this error: watchOS
tvOS:
macOS:
iOS:
|
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
@paulocoutinhox please let us know if the latest update addresses the |
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
Hi, watchOS:
Thanks |
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
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
Possible fix for ILP32 compile issue described in #7787
Manually tested because I haven't been able to get clang or armclang to build the full library for ILP32.
PR checklist
Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")