Skip to content
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

Backport 2.x: Fix x86_64 assembly for bignum multiplication #4941

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Sep 13, 2021

Backport of #4947

@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-crypto Crypto primitives and low-level interfaces needs-reviewer This PR needs someone to pick it up for review labels Sep 13, 2021
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a 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.

@davidhorstmann-arm davidhorstmann-arm linked an issue Sep 14, 2021 that may be closed by this pull request
@laumor01 laumor01 requested a review from xffbai September 15, 2021 10:04
@xffbai
Copy link
Contributor

xffbai commented Sep 15, 2021

Looks good to me.
I think you mean (%%rsi) in the commit message?

xffbai
xffbai previously approved these changes Sep 15, 2021
MULADDC_CORE reads from (%%rsi) and writes to (%%rdi). This fragment is
repeated up to 16 times, and %%rsi and %%rdi are s and d on entry
respectively. Hence the complete asm statement reads 16 64-bit words
from memory starting at s, and writes 16 64-bit words starting at d.

Without any declaration of modified memory, Clang 12 and Clang 13 generated
non-working code for mbedtls_mpi_mod_exp. The constraints make the unit
tests pass with Clang 12.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Sep 15, 2021
@gilles-peskine-arm
Copy link
Contributor Author

I fixed the register name in the commit message.

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

xffbai
xffbai previously approved these changes Sep 15, 2021
This had actually been reported multiple times.

Signed-off-by: Gilles Peskine <[email protected]>
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@xffbai xffbai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels Sep 16, 2021
@gilles-peskine-arm gilles-peskine-arm merged commit 02e17c0 into Mbed-TLS:development_2.x Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8e464c4 breaks clang 12 -O2 (make test failed)
3 participants