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.28] Fix preprocessor guards for C99 format size specifiers #10044

Merged
merged 10 commits into from
Mar 13, 2025

Conversation

bensze01
Copy link
Contributor

@bensze01 bensze01 commented Mar 7, 2025

Description

This PR is the Mbed TLS 2.28 backport of #10020

PR checklist

Sorry, something went wrong.

bensze01 added 2 commits March 7, 2025 17:58
Move the suite's global dependency on MBEDTLS_SSL_TLS_C to the
individual test cases.

Add an preprocesor guard around string_debug to prevent warning about unused
functions.

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
@bensze01 bensze01 added bug needs-review Every commit must be reviewed by at least two team members, component-tls needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Mar 7, 2025
@bensze01 bensze01 changed the base branch from development to mbedtls-2.28 March 7, 2025 17:15
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

The backport is faithfull but the CI is failing for a missing semicolon in test_suite_debug.functon

bensze01 added 5 commits March 8, 2025 00:23
The Windows CRT treats any invalid format specifiers passed to the CRT
as fatal assertion failures. Disable thie behaviour temporarily while
testing if the format specifiers we use are supported.

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Visual Studio 2013 (_MSC_VER == 1800) doesn't support %zu - only use it
on 2015 and above (_MSC_VER >= 1900).

%ldd works on Visual Studio 2013, but this patch keeps the two macro
definitions together, for simplicity's sake.

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
These headers were necessary for compatibility with Visual Studio 2010,
and interfere with the system headers on Visual Studio 2013+, eg. when
building Mbed TLS using the .sln file shipped with the project.

Move the still-required definition of "inline" to callconv.h, where the
definition for GCC also lives.

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
@bensze01 bensze01 force-pushed the msvc-format-size-macros-2.28 branch from 1645f1e to ded3500 Compare March 7, 2025 23:41
@bensze01 bensze01 requested a review from minosgalanakis March 7, 2025 23:44
@bensze01
Copy link
Contributor Author

bensze01 commented Mar 7, 2025

I dropped commit 6fb8f65 from this backport, as MBEDTLS_PRINTF_MS_TIME is not present in the 2.28 branch. Please re-review.

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
minosgalanakis
minosgalanakis previously approved these changes Mar 9, 2025
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@bensze01 bensze01 added priority-very-high Highest priority - prioritise this over other review work and removed needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Mar 10, 2025
@gilles-peskine-arm gilles-peskine-arm added priority-high High priority - will be reviewed soon and removed priority-very-high Highest priority - prioritise this over other review work labels Mar 11, 2025
Remove mention of the shipped .sln files, as those are planned to be
removed from Mbed TLS.

Clarify the affected CRT headers.

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
@bensze01 bensze01 added the needs-ci Needs to pass CI tests label Mar 12, 2025
@bensze01 bensze01 requested a review from minosgalanakis March 12, 2025 16:45
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
@bensze01 bensze01 force-pushed the msvc-format-size-macros-2.28 branch from db71dc5 to cb094f9 Compare March 12, 2025 18:16
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-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: same as #10043 except for the parts about mbedtls_ms_time_t, which doesn't exist in 2.28.

Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@bensze01 bensze01 added this pull request to the merge queue Mar 13, 2025
@bensze01 bensze01 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 Mar 13, 2025
Merged via the queue into mbedtls-2.28 with commit 85cb1f5 Mar 13, 2025
6 checks passed
@bensze01 bensze01 deleted the msvc-format-size-macros-2.28 branch March 13, 2025 17:00
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-tls priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants