[3.6] Fix build C++ apps with MSVC #10200

aslze · 2025-06-01T18:12:08Z

Description

This fixes Bug #7087 in the 3.6 branch (unable to build C++ apps that use mbedTLS on MSVC 2019 Windows).

Ports the main commit in TF-PSA-Crypto#258, changing crypto_extra.h.
Also fixes building a couple test programs inside mbedtls due to a warning in winbase.h in C11 mode.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog provided.
development PR not required because: This is a backport of Fix MSVC build issue from MbedTls issue 7087 TF-PSA-Crypto#258
TF-PSA-Crypto PR Fix MSVC build issue from MbedTls issue 7087 TF-PSA-Crypto#258
framework PR not required
3.6 PR provided
tests not required because: minor changes, existing tests are enough

aslze · 2025-06-01T19:59:02Z

Oops. I missed the signing-off part. That means commits must be done by actual team members and not external contributors? How can this proceed?

gilles-peskine-arm · 2025-06-01T20:13:30Z

@aslze Thanks for the contribution. You need to add a sign-off line to every commit message, to convey that you are allowed to make this contribution and that you allow us to use it as described in dco.txt.

Practically, you can run git rebase -i --signoff mbedtls-3.6 to add a signoff line to the new commits since mbedtls-3.6, or e.g. git rebase -i --signoff HEAD~3 to sign off the last 3 commits.

In crypto_extra.h, move PAKE size calculation macros, the definition of psa_pake_cipher_suite_s and psa_pake_operation_s just after PAKE type and values definitions. This aligns with the order of crypto header inclusions in crypto.h: crypto_types.h, then crypto_values.h, then crypto_sizes.h, and then crypto_struct.h. Take care of keeping them outside of the pake Doxygen group as they used to be. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

As the definition of psa_pake_operation_s has been moved the "xyt_t" structure types can not be used anymore (defined later). Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm · 2025-06-03T08:18:01Z

@aslze thanks for this pull request. Revisiting the changes in crypto_extra.h in this LTS context, I think we should do better actually in terms of not changing the Doxygen documentation and aligning with the re-organization done in crypto.h (#10196).
Instead of 101b0e9 I propose the two last commits of https://github.com/ronald-cron-arm/mbedtls/tree/fix-crypto-extra-for-msvc-3.6. I've tested the changes against our CI and I believe they should fix the issue MSVC as well. Would you be able to check that it is actually the case?

aslze · 2025-06-03T10:13:35Z

@aslze thanks for this pull request. Revisiting the changes in crypto_extra.h in this LTS context, I think we should do better actually in terms of not changing the Doxygen documentation and aligning with the re-organization done in crypto.h (#10196). Instead of 101b0e9 I propose the two last commits of https://github.com/ronald-cron-arm/mbedtls/tree/fix-crypto-extra-for-msvc-3.6. I've tested the changes against our CI and I believe they should fix the issue MSVC as well. Would you be able to check that it is actually the case?

I'll check later today. I don't understand the reference to Doxygen. IIRC, the idea of the commit was to move the definition of some structs up in the file. Struct names are the same, I think.

aslze · 2025-06-03T21:41:29Z

@ronald-cron-arm yes, those two commits fix the issue, thanks.

Would you add this commit from my PR? It's a tiny change. Fixes compilation of two test programs due to an apparent bug in a Windows header in C11 mode (as the compilation sets warnings as errors).

ronald-cron-arm · 2025-06-04T07:15:03Z

@ronald-cron-arm yes, those two commits fix the issue, thanks.

Would you add this commit from my PR? It's a tiny change. Fixes compilation of two test programs due to an apparent bug in a Windows header in C11 mode (as the compilation sets warnings as errors).

Yes sure. Actually, would you mind updating this PR with the two last commits of https://github.com/ronald-cron-arm/mbedtls/tree/fix-crypto-extra-for-msvc-3.6 instead of 101b0e9 and address #10200 (comment)?

Otherwise, if you prefer I can create a new PR based on https://github.com/ronald-cron-arm/mbedtls/tree/fix-crypto-extra-for-msvc-3.6 including your commit.

ChangeLog.d/fix_msvc_build.txt

… winbase.h) Signed-off-by: Alvaro Segura <alvaro.segura@gmail.com>

Signed-off-by: Alvaro Segura <alvaro.segura@gmail.com>

ronald-cron-arm · 2025-06-05T07:23:14Z

@aslze Thanks for the update. I've just undone the squash in the last commit to three commits. That's better for the git history and that should help for the reviews. And I've started a run of CI.

davidhorstmann-arm

LGTM, thanks!

aslze · 2025-06-09T18:04:20Z

Can someone please take a look. The system is complaining about API changes in the naming of some structs:

psa_crypto_driver_pake_inputs_t -> struct psa_crypto_driver_pake_inputs_s
psa_pake_cipher_suite_t -> struct psa_pake_cipher_suite_s
psa_jpake_computation_stage_t -> struct psa_jpake_computation_stage_s

But I thought that was intentional and the _t types are aliases of the _s structs. So there is no API breakage.

ronald-cron-arm · 2025-06-10T07:02:56Z

Can someone please take a look. The system is complaining about API changes in the naming of some structs:
psa_crypto_driver_pake_inputs_t -> struct psa_crypto_driver_pake_inputs_s
psa_pake_cipher_suite_t -> struct psa_pake_cipher_suite_s
psa_jpake_computation_stage_t -> struct psa_jpake_computation_stage_s
But I thought that was intentional and the _t types are aliases of the _s structs. So there is no API breakage.

Yes to me this is a false positive of our API/ABI checker script.

include/psa/crypto_extra.h

minosgalanakis

Looks good, it is an incremental build-up ofMbed-TLS/TF-PSA-Crypto#258 following #10196.

The abi check is a false positive and the mangling of the function signature will not be affected if we refer ot it by struct or by typedef alias, when building with C/C++.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

minosgalanakis

LGTM

minosgalanakis

While reviewing Mbed-TLS/TF-PSA-Crypto#258 I noticed that commit #31f7446957212fb95089bdb45c87a631fefaa8e1 is missing.

It looks like a minor fix that is easy to back-port. If it was needed in the tf-psa-crypto version we should have it here too.

Signed-off-by: Cesar Cruz <cesar.cruz@philips.com> Signed-off-by: ccrugoPhilips <cesar.cruz@philips.com> Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm · 2025-06-18T08:15:11Z

While reviewing Mbed-TLS/TF-PSA-Crypto#258 I noticed that commit #31f7446957212fb95089bdb45c87a631fefaa8e1 is missing.

It looks like a minor fix that is easy to back-port. If it was needed in the tf-psa-crypto version we should have it here too.

I've added it.

irwir · 2025-06-18T14:18:19Z

Yes to me this is a false positive of our API/ABI checker script.

Not really. Declarations and definitions should match fully.
As was explained in my issue, such differences could break navigation to declaration/definition in IDE, and static analysis might generate diagnostic messages.
The issue was closed as not planned.

davidhorstmann-arm

LGTM, thanks!

minosgalanakis

LGTM

My comments have been addressed. I am one of the authors now thus cannot approve.

gilles-peskine-arm requested a review from ronald-cron-arm June 1, 2025 20:14

gilles-peskine-arm added bug needs-ci Needs to pass CI tests size-xs Estimated task size: extra small (a few hours at most) priority-medium Medium priority - this can be reviewed as time permits labels Jun 1, 2025

gilles-peskine-arm added this to Non-roadmap pull requests Jun 1, 2025

github-project-automation bot moved this to In Development in Non-roadmap pull requests Jun 1, 2025

aslze force-pushed the mbedtls-3.6 branch 2 times, most recently from e6b9b9f to 11ded46 Compare June 1, 2025 23:16

ronald-cron-arm added 2 commits June 3, 2025 10:06

Fix psa_pake_operation_s member types

3819005

As the definition of psa_pake_operation_s has been moved the "xyt_t" structure types can not be used anymore (defined later). Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm previously requested changes Jun 4, 2025

View reviewed changes

ChangeLog.d/fix_msvc_build.txt Outdated Show resolved Hide resolved

aslze force-pushed the mbedtls-3.6 branch 2 times, most recently from f9e0b5a to 6b08688 Compare June 4, 2025 21:49

aslze added 2 commits June 5, 2025 09:09

Fix build test programs in MSVC (due to a warning treated as error in…

673e1eb

… winbase.h) Signed-off-by: Alvaro Segura <alvaro.segura@gmail.com>

Fix change log entry

41422e1

Signed-off-by: Alvaro Segura <alvaro.segura@gmail.com>

ronald-cron-arm force-pushed the mbedtls-3.6 branch from 6b08688 to 41422e1 Compare June 5, 2025 07:14

ronald-cron-arm removed the needs-ci Needs to pass CI tests label Jun 5, 2025

davidhorstmann-arm self-requested a review June 5, 2025 11:16

davidhorstmann-arm previously approved these changes Jun 6, 2025

View reviewed changes

minosgalanakis self-requested a review June 10, 2025 09:38

minosgalanakis reviewed Jun 10, 2025

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

minosgalanakis previously approved these changes Jun 10, 2025

View reviewed changes

ronald-cron-arm mentioned this pull request Jun 12, 2025

Fix MSVC build issue from MbedTls issue 7087 Mbed-TLS/TF-PSA-Crypto#258

Merged

4 tasks

Remove blank line

d1f5169

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm dismissed stale reviews from minosgalanakis and davidhorstmann-arm via d1f5169 June 16, 2025 09:11

ronald-cron-arm requested review from davidhorstmann-arm and minosgalanakis June 16, 2025 09:12

minosgalanakis approved these changes Jun 16, 2025

View reviewed changes

minosgalanakis requested changes Jun 17, 2025

View reviewed changes

fix: additional MSVC v142 build issue with tls1.3 configuration enabled.

7df8992

Signed-off-by: Cesar Cruz <cesar.cruz@philips.com> Signed-off-by: ccrugoPhilips <cesar.cruz@philips.com> Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm force-pushed the mbedtls-3.6 branch from fc92438 to 7df8992 Compare June 18, 2025 08:14

ronald-cron-arm requested a review from minosgalanakis June 18, 2025 08:27

davidhorstmann-arm approved these changes Jun 19, 2025

View reviewed changes

minosgalanakis approved these changes Jun 19, 2025

View reviewed changes

davidhorstmann-arm added the approved Design and code approved - may be waiting for CI or backports label Jun 19, 2025

davidhorstmann-arm added this pull request to the merge queue Jun 19, 2025

Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 54ceaf7 Jun 19, 2025
3 of 4 checks passed

github-project-automation bot moved this from In Development to Done in Non-roadmap pull requests Jun 19, 2025

davidhorstmann-arm mentioned this pull request Jun 20, 2025

Improve detection of CMake library installation problems #10239

Merged

5 tasks

ronald-cron-arm mentioned this pull request Jun 23, 2025

Unable to build simple c++ example with Visual Studio 15 2017 #7087

Closed

[3.6] Fix build C++ apps with MSVC #10200

[3.6] Fix build C++ apps with MSVC #10200

Uh oh!

Conversation

aslze commented Jun 1, 2025 • edited by minosgalanakis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

aslze commented Jun 1, 2025

Uh oh!

gilles-peskine-arm commented Jun 1, 2025

Uh oh!

ronald-cron-arm commented Jun 3, 2025

Uh oh!

aslze commented Jun 3, 2025

Uh oh!

aslze commented Jun 3, 2025

Uh oh!

ronald-cron-arm commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ronald-cron-arm commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

Uh oh!

aslze commented Jun 9, 2025

Uh oh!

ronald-cron-arm commented Jun 10, 2025

Uh oh!

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm commented Jun 18, 2025

Uh oh!

irwir commented Jun 18, 2025

Uh oh!

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aslze commented Jun 1, 2025 •

edited by minosgalanakis

Loading

ronald-cron-arm commented Jun 4, 2025 •

edited

Loading

ronald-cron-arm commented Jun 5, 2025 •

edited

Loading