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

Fix compiling AESNI in Mbed-TLS with clang on Windows #8374

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Fix compiling AESNI in Mbed-TLS with clang on Windows #8374

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/ 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")

lpy4105
lpy4105 previously approved these 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.

LGTM, I've tested the build succeeds with default configuration using clang + Visual Studio.

@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-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
@lpy4105
Copy link
Contributor

lpy4105 commented Oct 17, 2023

Please fix the code style issue reported by CI.

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Oct 17, 2023 •
edited
Loading

Please fix the code style issue reported by CI.

@lpy4105 Fixed.

lpy4105
lpy4105 previously approved these 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.

LGTM

@tom-cosgrove-arm tom-cosgrove-arm changed the title Fix #8372 - Error compiling AESNI in Mbed-TLS with clang on Windows 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

@tom-cosgrove-arm
Copy link
Contributor

Do we need a ChangeLog entry for this and the backport?

(Testing definitely excluded, since we don't have clang-cl installed on Windows in the CI)

@lpy4105
Copy link
Contributor

lpy4105 commented Oct 19, 2023

Do we need a ChangeLog entry for this and the backport?

Agreed.

@sergio-nsk Could you please add a ChangLog entry for this and the backport?

@sergio-nsk sergio-nsk dismissed stale reviews from tom-cosgrove-arm and lpy4105 via 78d1076 October 19, 2023 02:48
@sergio-nsk
Copy link
Contributor Author

Is the update good enough? I seemed to add the first changelog entry after the release, and don't know how to start a new chapter.

@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Oct 19, 2023 •
edited
Loading

Is the update good enough? I seemed to add the first changelog entry after the release, and don't know how to start a new chapter.

Don't update ChangeLog directly (for all the reasons you've just noted) - instead, create a file in the ChangeLog.d directory with a .txt extension. The contents could be something like

Features
   * AES-NI is now supported in Windows builds with clang-cl.exe.

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

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

Signed-off-by: Sergey Markelov <sergey@solidstatenetworks.com>
@tom-cosgrove-arm tom-cosgrove-arm added the needs-ci Needs to pass CI tests label Oct 19, 2023
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, thank you!

@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 20, 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
@lpy4105
Copy link
Contributor

lpy4105 commented Oct 26, 2023

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

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Oct 26, 2023
Merged
3 tasks
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Oct 26, 2023
@daverodgman daverodgman mentioned this pull request Oct 26, 2023
Merged
3 tasks
@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 enabled auto-merge October 26, 2023 15:55
@bensze01 bensze01 added this pull request to the merge queue Oct 26, 2023
Merged via the queue into Mbed-TLS:development with commit 5132816 Oct 26, 2023
@sergio-nsk sergio-nsk deleted the sergio-nsk/8372/2 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

@lpy4105 lpy4105 lpy4105 approved these changes

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

Error compiling AESNI in Mbed-TLS with clang on Windows
4 participants
@sergio-nsk @lpy4105 @tom-cosgrove-arm @bensze01