FIX: arm64 amalgamations + better CI coverage#3931
Merged
randombit merged 7 commits intorandombit:masterfrom Mar 22, 2024
Merged
FIX: arm64 amalgamations + better CI coverage#3931randombit merged 7 commits intorandombit:masterfrom
randombit merged 7 commits intorandombit:masterfrom
Conversation
This was referenced Mar 5, 2024
1a5e8ec to
df6dcab
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This is a follow up for randombit#3780 that fixes the ISA issues for both clang (for Android NDK) and gcc. Co-Authored-By: Philippe Lieser <philippe.lieser@rohde-schwarz.com>
On Apple Silicon, when disabling 'neon' via BOTAN_CLEAR_CPUID, _all_ other CPUID flags went away, leaving the result of the CLI invocation empty. The test didn't handle that correctly and failed.
Unclear how that worked on other platforms. It did fail on macOS with Apple silicon.
5c3a1d8 to
b88819c
Compare
Collaborator
Author
|
Rebased after merging #3932 |
randombit
approved these changes
Mar 19, 2024
Owner
randombit
left a comment
There was a problem hiding this comment.
I agree things are not necessarily in sync with the info files. I also don’t like the repetition of having these magic (compiler-specific, sometimes undocumented) strings everywhere - ideally we’d have a header that encapsulates this like we do for the SIMD API annotations. (No need to do this here.)
Change itself lgtm.
Owner
|
Good to merge? Not sure if more was going to happen here. |
Collaborator
Author
|
Should be fine to merge. I didn't plan to address the discussed ISA flag duplication here. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This continues the work @lieser started in #3780. It aims to fix #3927 and #3774. It also incorporates #3930.
Along with fixes to the relevant
BOTAN_FUNC_ISA()annotations, this adds CI targets for amalgamation builds on arm64 (using GCC) and Android NDK arm64 (using clang). Also, this adds builds on themacos-14runner released a month ago, that is based on Apple Silicon, i.e. arm64 😅.Note: This also fixes an issue in the Python wrapper, where the
ffi_api()declaration failed to explicitly specify ac_intbuffer length parameter. On Apple silicon that resulted in this parameter being0on the C/C++ side. Not entirely sure how that ever worked on all other platforms we test in CI.@randombit I'm somewhat worried that these inline ISA-annotations aren't necessarily in sync with whatever is defined in the
src/build-data/cc/*.txtfiles. Currently, for arm64 the definitions are as such:clang.txt
gcc.txt
... i.e.
clang.txtdoesn't even define all of them. I'm guessing that the inline annotations usingBOTAN_FUNC_ISA()are saving the day for those, no?