Backport 2.16: bn_mul.h: require at least ARMv6 to enable the ARM DSP code #2778
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
Commit 16b1bd8 "bn_mul.h: add ARM DSP optimized MULADDC code" added some ARM DSP instructions that was assumed to always be available when __ARM_FEATURE_DSP is defined to 1. Unfortunately it appears that the ARMv5TE architecture (GCC flag -march=armv5te) supports the DSP instructions, but only in Thumb mode and not in ARM mode, despite defining __ARM_FEATURE_DSP in both cases. This patch fixes the build issue by requiring at least ARMv6 in addition to the DSP feature.
Non-regression test for "bn_mul.h: require at least ARMv6 to enable the ARM DSP code"
gilles-peskine-arm
added
bug
CLA valid
needs-review
This was referenced Aug 5, 2019
Without any -O option, the default is -O0, and then the assembly code is not used, so this would not be a non-regression test for the assembly code that doesn't build.
gilles-peskine-arm
force-pushed
the
bn_mul-arm-dsp-fix-2.16
branch
from
8784bc5 to
e07b9ff
Compare
AndrzejKurek
previously approved these changes
Aug 9, 2019
|
I see a test failure
Is this a known issue? |
|
@jainvikas8 Oh, DTLS proxy tests consistency was an issue some time ago, I thought that it has already been fixed :) This seems unrelated to the PR. |
|
The test failure is one instance of one DTLS test, presumably due to a timeout, which is a known problem and obviously unrelated to this PR. So CI is ok. |
jainvikas8
previously approved these changes
Aug 9, 2019
k-stachowiak
previously approved these changes
Aug 9, 2019
Call the component xxx_arm5vte, because that's what it does. Explain "armel", and more generally why this component exists, in a comment.
gilles-peskine-arm
dismissed stale reviews from k-stachowiak, jainvikas8, and AndrzejKurek
via
560f332
k-stachowiak
approved these changes
Aug 12, 2019
|
@k-stachowiak It's ok, we resolve add/add changelog conflicts at merge time. |
AndrzejKurek
approved these changes
Aug 13, 2019
gilles-peskine-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.
Fix the build on ARMv5TE in ARM mode. The assembly code in
bn_mul.huses DSP instructions that are not available on this particular architecture, so set the conditional compilation directives to exclude them.Fix originally contributed in #2169 by @aurel32. I rebased to mbedtls-2.16 to facilitate testing and added a changelog entry and a non-regression test.