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.

Sign up for GitHub

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

Jump to bottom

Backport 2.28: Fix compiling AESNI in Mbed-TLS with clang on Windows #8373

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Backport 2.28: Fix compiling AESNI in Mbed-TLS with clang on Windows #8373

merged 1 commit into from
Oct 26, 2023

Conversation

sergio-nsk
Copy link
Contributor

@sergio-nsk sergio-nsk commented Oct 16, 2023 •
edited
Loading

Description

Fixes #8372. Does not conflict with #8339.

It can successfully compile w/ or w/o the clang options -maes -mpclmul.

The macro __GNUC__ is not defined by Clang on Windows, thus use the macro __clang__ for -maes -mpclmul.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog done
  • backport not required
  • tests not required

@sergio-nsk sergio-nsk changed the title Fix #8372 - Error compiling AESNI in Mbed-TLS with clang on Windows 2.28: Fix #8372 - Error compiling AESNI in Mbed-TLS with clang on Windows Oct 16, 2023
@sergio-nsk sergio-nsk mentioned this pull request Oct 16, 2023
Merged
3 tasks
lpy4105
lpy4105 suggested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@lpy4105 lpy4105 left a comment

Choose a reason for hiding this comment

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

Minor code style issue.
Have you checked this work on 2.28 branch?

library/aesni.c Outdated
@@ -191,7 +191,7 @@ void mbedtls_aesni_gcm_mult(unsigned char c[16],
const unsigned char a[16],
const unsigned char b[16])
{
__m128i aa, bb, cc, dd;
__m128i aa = {0}, bb = {0}, cc, dd;
Copy link
Contributor

Choose a reason for hiding this comment

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

code style

Suggested change
__m128i aa = {0}, bb = {0}, cc, dd;
__m128i aa = { 0 }, bb = { 0 }, cc, dd;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. BTW clang thought the variables were not initialized and raised errors with -Werror.

@lpy4105 lpy4105 added bug needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Oct 17, 2023
@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Oct 17, 2023 •
edited
Loading

Minor code style issue. Have you checked this work on 2.28 branch?

Yes, of course. I use the LTS version, hence I have fixed the compiling issue here and then in developement. It's now compiled well with MSVC, clang and clang -maes -mpclmul. The latter is not required in 2.28 like in devlopement https://github.com/Mbed-TLS/mbedtls/blob/development/library/aesni.h#L67-L72.

lpy4105
lpy4105 previously approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@lpy4105 lpy4105 left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm tom-cosgrove-arm changed the title 2.28: Fix #8372 - Error compiling AESNI in Mbed-TLS with clang on Windows Backport 2.28: Fix compiling AESNI in Mbed-TLS with clang on Windows Oct 18, 2023
@tom-cosgrove-arm tom-cosgrove-arm removed the needs-reviewer This PR needs someone to pick it up for review label Oct 18, 2023
tom-cosgrove-arm
tom-cosgrove-arm previously approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@sergio-nsk
Fix Mbed-TLS#8372 - Error compiling AESNI in Mbed-TLS with clang on W…
9902a6b 
…indows

It can successfully compile w/ or w/o the clang options -maes -mpclmul.

Signed-off-by: Sergey Markelov <sergey@solidstatenetworks.com>
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

lpy4105
lpy4105 approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@lpy4105 lpy4105 left a comment

Choose a reason for hiding this comment

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

LGTM

@lpy4105 lpy4105 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-ci Needs to pass CI tests labels Oct 19, 2023
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Oct 25, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 25, 2023
@sergio-nsk
Copy link
Contributor Author

I don't know why my PR is kicked out the merge queue. How can I investigate it?

@lpy4105
Copy link
Contributor

lpy4105 commented Oct 26, 2023

Merge queue failure seems to be spurious, need to resubmit.

@gilles-peskine-arm
Copy link
Contributor

Weird infrastructure failure on the merge queue:

Channel "unknown": Remote call on container-host.eu-west-1b (i-007507a020fad60fb) failed. The channel is closing down or has closed down

This is a new error to me, but it seems to happen often since a day or two ago. Same error in #8374.

@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Oct 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 26, 2023
@bensze01 bensze01 added this pull request to the merge queue Oct 26, 2023
Merged via the queue into Mbed-TLS:mbedtls-2.28 with commit 3ccb844 Oct 26, 2023
6 checks passed
@sergio-nsk sergio-nsk deleted the sergio-nsk/8372/1 branch August 14, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@lpy4105 lpy4105 lpy4105 approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sergio-nsk @lpy4105 @gilles-peskine-arm @tom-cosgrove-arm @bensze01