WIP: Remove redundant declarations and update docs #727

2026-03-26T03:09:57-05:00

bjwtaylor commented

2026-03-26 03:09:57 -05:00

(Migrated from github.com)

Description

Currently we have a duplicate definition warning caused by the same definition being in crypto.h and crypto_structs.h My understanding from the docs is crypto.h is the public API and crypto_structs.h is the backend implementation. If I have understood correctly there will always be a crypto_structs.h or similar definition and this will always be the definition we need the system to use. Therefore what I can tell this means crypto.h exists purely for documentation. I have therefore resolved the issue by guarding the duplicate definitions with DOXYGEN_ONLY. My understanding is this will remove them from compilation, however they will still be used in the API documentation.

Fix https://github.com/Mbed-TLS/mbedtls/issues/10376

PR checklist

changelog provided
framework PR not required
mbedtls development PR provided Mbed-TLS/mbedtls#https://github.com/Mbed-TLS/mbedtls/pull/10659
mbedtls 3.6 PR not required because: No Backports
tests not required because: No changes.

## Description Currently we have a duplicate definition warning caused by the same definition being in crypto.h and crypto_structs.h My understanding from the docs is crypto.h is the public API and crypto_structs.h is the backend implementation. If I have understood correctly there will always be a crypto_structs.h or similar definition and this will always be the definition we need the system to use. Therefore what I can tell this means crypto.h exists purely for documentation. I have therefore resolved the issue by guarding the duplicate definitions with __DOXYGEN_ONLY__. My understanding is this will remove them from compilation, however they will still be used in the API documentation. Fix https://github.com/Mbed-TLS/mbedtls/issues/10376 ## PR checklist - [ ] **changelog** provided - [ ] **framework PR** not required - [ ] **mbedtls development PR** provided Mbed-TLS/mbedtls#https://github.com/Mbed-TLS/mbedtls/pull/10659 - [ ] **mbedtls 3.6 PR** not required because: No Backports - **tests** not required because: No changes.

gilles-peskine-arm (Migrated from github.com) requested changes

2026-04-03 14:27:18 -05:00

gilles-peskine-arm (Migrated from github.com) left a comment

The proposed change is not desirable.

It's not really clear to me what the goal is. If the goal is to compile with no warnings from gcc -Wredundant-decls, we need a different solution. That could be some header restructuring, or selectively disabling the warning using pragmas.

If we make our code clean with -Wredundant-decls, we need to add this to our build scripts so that it's enforced. However, it's not fully clear to me why this is desirable. We've never promised to pay attention to this warning and I don't see why it's useful.

The proposed change is not desirable. It's not really clear to me what the goal is. If the goal is to compile with no warnings from `gcc -Wredundant-decls`, we need a different solution. That could be some header restructuring, or selectively disabling the warning using pragmas. _If_ we make our code clean with `-Wredundant-decls`, we need to add this to our build scripts so that it's enforced. However, it's not fully clear to me why this is desirable. We've never promised to pay attention to this warning and I don't see why it's useful.

include/psa/crypto.h

					
				@ -111,6 +111,7 @@ psa_status_t psa_crypto_init(void);

				 * @{

				 */

				#ifdef __DOXYGEN_ONLY__

Marking some function prototypes as Doxygen-only doesn't make sense. The reason to have prototypes is so that the C compiler sees them.

The reason some static inline functions have a prototype in crypto.h and a definition in crypto_struct.h is that the prototypes are the same for everyone, but the function definitions are not. By default, TF-PSA-Crypto presents itself a library. But it can also be integrated as a client and server, in which case the integrator makes some changes:

On the client side:
- Define MBEDTLS_PSA_CRYPTO_CLIENT but not MBEDTLS_PSA_CRYPTO_C.
- Edit crypto_struct.h to change the definitions of operation structures, typically to have an operation handle instead of cryptographic material.
- Provide implementations of the PSA API functions (excluding the static inline functions that manipulate a local struct) that makes remote procedure calls to the server.
On the server side:
- Probably define MBEDTLS_PSA_CRYPTO_SPM and likely MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER.
- Implement the server side of the remote procedure calls.

While the definitions of the struct constructors in crypto_struct.h are customizable, their prototypes are not. Hence the prototypes live in crypto.h. And the prototypes must be visible to the C compiler.

Marking some function prototypes as Doxygen-only doesn't make sense. The reason to have prototypes is so that the C compiler sees them. The reason some `static inline` functions have a prototype in `crypto.h` and a definition in `crypto_struct.h` is that the prototypes are the same for everyone, but the function definitions are not. By default, TF-PSA-Crypto presents itself a library. But it can also be integrated as a client and server, in which case the integrator makes some changes: * On the client side: * Define `MBEDTLS_PSA_CRYPTO_CLIENT` but not `MBEDTLS_PSA_CRYPTO_C`. * Edit `crypto_struct.h` to change the definitions of operation structures, typically to have an operation handle instead of cryptographic material. * Provide implementations of the PSA API functions (excluding the `static inline` functions that manipulate a local struct) that makes remote procedure calls to the server. * On the server side: * Probably define `MBEDTLS_PSA_CRYPTO_SPM` and likely `MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER`. * Implement the server side of the remote procedure calls. While the _definitions_ of the struct constructors in `crypto_struct.h` are customizable, their _prototypes_ are not. Hence the prototypes live in `crypto.h`. And the prototypes must be visible to the C compiler.

That being said, is the current code really what it should be? No, there are two things that I never got around to fixing.

psa_key_attributes_t is not supposed to be user-serviceable, unlike operation structures. So it should be in crypto_types.h, and its inline accessor functions should not be defined in crypto_struct.h.
The definitions of the struct constructor functions (psa_xxx_init()) don't actually need to be in crypto_struct.h, because their textual definition is actually the same for everyone. They're defined in terms of the corresponding PSA_XXX_INIT macro. Only the macro needs to be customized if you customize the struct layout.

We could fix this. It would be an incompatible change for integrators who customize crypto_struct.h. We don't explicitly promise any backward compatibility for such custom integrations, so I think it would be ok to do what I just wrote in the development branch. But I don't think we should do it in an LTS branch unless there's a compelling reason, and I don't see a compelling reason.

That being said, is the current code really what it should be? No, there are two things that I never got around to fixing. * `psa_key_attributes_t` is not supposed to be user-serviceable, unlike _operation_ structures. So it should be in `crypto_types.h`, and its inline accessor functions should not be defined in `crypto_struct.h`. * The definitions of the struct constructor functions (`psa_xxx_init()`) don't actually need to be in `crypto_struct.h`, because their textual definition is actually the same for everyone. They're defined in terms of the corresponding `PSA_XXX_INIT` macro. Only the macro needs to be customized if you customize the struct layout. We could fix this. It would be an incompatible change for integrators who customize `crypto_struct.h`. We don't explicitly promise any backward compatibility for such custom integrations, so I think it would be ok to do what I just wrote in the development branch. But I don't think we should do it in an LTS branch unless there's a compelling reason, and I don't see a compelling reason.

bjwtaylor (Migrated from github.com) reviewed

2026-04-07 02:18:21 -05:00

include/psa/crypto.h

					
				@ -111,6 +111,7 @@ psa_status_t psa_crypto_init(void);

				 * @{

				 */

				#ifdef __DOXYGEN_ONLY__

OK, thanks for your comments. Let's put this one on hold pending design review of the original issue. There is also a related PR here: https://github.com/Mbed-TLS/mbedtls/pull/10659. It's the same issue, but not the same solution. Let me know what you think about this one, maybe we just decide we don't want to fix these warnings?

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'bjwtaylor/redundant-declarations:bjwtaylor/redundant-declarations'

git switch 'bjwtaylor/redundant-declarations'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'

git merge --no-ff 'bjwtaylor/redundant-declarations'

git switch 'bjwtaylor/redundant-declarations'

git rebase 'development'

git switch 'development'

git merge --ff-only 'bjwtaylor/redundant-declarations'

git switch 'bjwtaylor/redundant-declarations'

git rebase 'development'

git switch 'development'

git merge --no-ff 'bjwtaylor/redundant-declarations'

git switch 'development'

git merge --squash 'bjwtaylor/redundant-declarations'

git switch 'development'

git merge --ff-only 'bjwtaylor/redundant-declarations'

git switch 'development'

git merge 'bjwtaylor/redundant-declarations'

git push origin 'development'

Sign in to join this conversation.

No reviewers

gilles-peskine-arm

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto!727

Rows
Columns