Skip to content

Temporarily add pragmas #10271

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

bjwtaylor
Copy link

@bjwtaylor bjwtaylor commented Jul 3, 2025

Description

Temporarily add pragmas to allow switching of function prototypes contributes to Mbed-TLS/TF-PSA-Crypto#346.

This PR is part of a chain which needs to be merged in the following order:

  1. Temporarily add pragmas #10271
  2. Create new enum mbedtls_pk_sigalg_t TF-PSA-Crypto#356
  3. Update functions for new mbedtls_pk_sigalg_t #10290
  4. Revert Pragmas TF-PSA-Crypto#377

PR checklist

@bjwtaylor bjwtaylor changed the title New mbedtls pk sigalg t Create New mbedtls pk sigalg t Jul 3, 2025
@bjwtaylor bjwtaylor marked this pull request as ready for review July 8, 2025 06:38
@bjwtaylor bjwtaylor marked this pull request as draft July 8, 2025 06:38
@bjwtaylor bjwtaylor force-pushed the new-mbedtls_pk_sigalg_t branch 2 times, most recently from b34a28f to 4963a78 Compare July 8, 2025 09:30
@bjwtaylor bjwtaylor changed the title Create New mbedtls pk sigalg t Update functions to use new mbedtls pk sigalg t Jul 8, 2025
@bjwtaylor bjwtaylor force-pushed the new-mbedtls_pk_sigalg_t branch 2 times, most recently from fa7a36e to b24487c Compare July 9, 2025 10:37
@bjwtaylor bjwtaylor changed the title Update functions to use new mbedtls pk sigalg t Temporarily disable werror Jul 9, 2025
@bjwtaylor bjwtaylor marked this pull request as ready for review July 10, 2025 06:42
@bjwtaylor bjwtaylor added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jul 10, 2025
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.

As discussed in Slack, this is not acceptable. It'll hide errors for everyone until we undo it.

I don't know what the problem is precisely. It would be ok to disable a specific error locally with a #pragma GCC diagnostic (grep for examples where we know that some warning is spurious). Or maybe we should do the transition differently.

@gilles-peskine-arm gilles-peskine-arm added needs-work priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jul 11, 2025
CMakeLists.txt Outdated
@@ -271,9 +271,6 @@ function(set_gnu_base_compile_options target)
target_compile_options(${target} PRIVATE $<$<CONFIG:Check>:-Os>)
target_compile_options(${target} PRIVATE $<$<CONFIG:CheckFull>:-Os -Wcast-qual>)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about the following transition, would this work?

  1. In crypto, make mbedtls_pk_sigalg_t an alias of mbedtls_pk_type_t.
  2. In mbedtls, replace mbedtls_pk_type_t by mbedtls_pk_sigalg_t (so we'd be doing most of Switch to mbedtls_pk_sigalg_t #10264 before Create new enum mbedtls_pk_sigalg_t TF-PSA-Crypto#356).
  3. In crypto, change mbedtls_pk_sigalg_t to be the desired type.
  4. In mbedtls, finish Switch to mbedtls_pk_sigalg_t #10264.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the chain to use pragma's, which I think is the easiest way to get the changes in. Let me know what you think though?

@bjwtaylor bjwtaylor force-pushed the new-mbedtls_pk_sigalg_t branch from db651aa to 6ca4ce4 Compare July 14, 2025 09:02
@bjwtaylor bjwtaylor changed the title Temporarily disable werror Temporarily add pragmas Jul 14, 2025
@valeriosetti valeriosetti self-requested a review July 15, 2025 07:20
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

The #pragma change is OK to me, but putting it on top of a C file then it applies to it all. I know that this change is temporary and since there's no other change in this PR we should be fine, but I would prefer to limit this "disabling" only on where it's strictly needed inside the C file. I think you can use:

#pragma GCC diagnostic push
#pragma GCC diagnostic warning "-Wenum-conversion"
....
#pragma GCC diagnostic pop

to store/resume checks around the problematic lines. Do you think this is feasible?

Signed-off-by: Ben Taylor <[email protected]>
@bjwtaylor bjwtaylor force-pushed the new-mbedtls_pk_sigalg_t branch from 6ca4ce4 to 2c1b078 Compare July 16, 2025 08:06
Signed-off-by: Ben Taylor <[email protected]>
@gilles-peskine-arm
Copy link
Contributor

The pragmas need to be protected from compilers that don't recognize them.

I think it would be easier to write a single conversion function and use that, rather than put pragmas everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-work priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Development

Successfully merging this pull request may close these issues.

3 participants