Fix for arm64_32 (aka ILP32) on Clang #7808
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
daverodgman
added
bug
component-platform
|
Possibly something like |
|
Im getting this error: |
Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
|
@paulocoutinhox does the latest version help? |
daverodgman
added
the
needs-info
|
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): |
daverodgman
added
needs-review
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.
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.
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.
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>
gilles-peskine-arm
requested changes
Jun 21, 2023
There was a problem hiding this comment.
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.
gilles-peskine-arm
previously approved these changes
Jun 21, 2023
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.
Minor, not worth updating just for that:
Suggested change
| * 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.
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.
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. |
daverodgman
force-pushed
the
fix-ct-compile-warning
branch
from
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>
daverodgman
force-pushed
the
fix-ct-compile-warning
branch
from
f69c57d
to
e6c9996
Compare
|
Casting to |
Co-authored-by: Tom Cosgrove <tom.cosgrove@arm.com> Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>
|
@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 |
daverodgman
removed
the
needs-info
tom-cosgrove-arm
added
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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")