Header files should have different names

[irwir]

Suggested enhancement

Add psa_ prefix to the files in /tf-psa-crypto/include/tf-psa-crypto directory (or any other way to make file names distinct).

Justification

Currently there are two sets of files with different contents but the same name: build_info.h and check_config.h in include/mbedtls and /tf-psa-crypto/include/tf-psa-crypto directories. Now IDE would be showing the same name in tab headers if both build_info.h were opened.

Mbed TLS needs this because of added convenience. A similar change had been made previously - when config.h was renamed to mbedtls_config.h (the name was found to be too general).

[mpg]

Hi @irwir and thanks for this suggestion! Indeed it seems to me that the reasons that led us to rename config.h would also apply here. (IIRC, it was not just about convenience - some people use build systems that simply cannot cope with files with the same name in different directories.)

Cc @ronald-cron-arm

[gilles-peskine-arm]

We should do this before the 1.0 release. Otherwise it could break the build of Mbed TLS and applications using Mbed TLS, if mbedtls/tf-psa-crypto/drivers/builtin/include is on the include path before mbedtls/include/mbedtls. We have had complaints before from users who don't have control over the order of the include path. Thus this issue is a regression for such users compared to 3.6.

[mpg]

Note: there are currently 3 instances of build_info.h in an Mbed TLS checkout:

tf-psa-crypto/drivers/builtin/include/mbedtls/build_info.h
tf-psa-crypto/include/tf-psa-crypto/build_info.h
include/mbedtls/build_info.h

The first one, tf-psa-crypto/drivers/builtin/include/mbedtls/build_info.h is simply an alias for the second one. It should be removed and files that include it updated - which will be needed anyway when renaming.

[ronald-cron-arm]

The first one, tf-psa-crypto/drivers/builtin/include/mbedtls/build_info.h is simply an alias for the second one. It should be removed and files that include it updated - which will be needed anyway when renaming.

The files that include it are the files in framework/tests and that's not that easy to change.

[irwir]

A recent issue in other project: different headers with the same name, all in the include path. The result - compilation failure.

[irwir]

4 common.h files in total; and duplication of framework files adds one more. So five in total.

Edit. 2 service.h (3 with duplication of framework).

[mpg]

The files that include it are the files in framework/tests and that's not that easy to change.

I think we have to solve this issue regardless of whether it's easy or not. We know from past experience that having different headers with the same base name will cause trouble to some of our users (and not just inconvenience, but actual build failures). And as Gilles pointed out, we can't really rename headers after 1.0, so it needs to be done before.

We should think about how to do it - perhaps the framework can have some proxy headers like framework_xxx.h that just includes the proper path/to/project_xxx.h depending on the ambient project + version, and then files that need what's currently called xxx.h just include framework_xxx.h instead.

But I don't think the discussion is about how to do it, not if we'll do it, right?

[gilles-peskine-arm]

different headers with the same base name will cause trouble to some of our users (and not just inconvenience, but actual build failures)

I can't think of any build failures due to headers with the same base name. I'm aware of linking failures if two .c files have the same base names, because lots of platforms combine objects from multiple projects into a single whole-platform binary. And I'm aware of problems when editable files such as config.h have the same basename. But I'm not aware of problems when different header paths (as in, different strings passed to #include) have the same basename.

So common.h could be a problem because we have #include "common.h" referring to different files in different places. (In fact a problem has already surfaced: https://github.com/Mbed-TLS/mbedtls/issues/10223.) But I don't see a breaks-the-build problem with tf-psa-crypto/build_info.h vs mbedtls/build_info.h.

We do have a problem with mbedtls/build_info.h because there are two files with the same include path.

I do agree that it costs very little to rename the files even if they don't cause a build failure, and it's a minor but genuine quality-of-life improvement, so we might as well do it.

[irwir]

I can't think of any build failures due to headers with the same base name.

One case has been already mentioned (March, 18, not mbedTLS library): different versions of header files with the same name - one local, another somewhere in the compiler's standard paths. This is always a possiblity while the order of header files lookup is implementation-defined.

[ronald-cron-arm]

I have had a look to the comments in this PR and in #10223.

To address this issue, I propose to rename in TF-PSA-Crypto the following files:

• include/tf-psa-crypto/build_info.h to include/tf-psa-crypto/tf_psa_crypto_build_info.h
• include/tf-psa-crypto/check_config.h to include/tf-psa-crypto/tf_psa_crypto_check_config.h
• core/common.h to core/tf_psa_crypto_common.h

The main reason being to avoid too common file names, and this has the benefit of removing the duplicates with Mbed TLS.

All the other header files mentioned in this PR comments are test header files and we can change their name whenever we want. Note that tf-psa-crypto/drivers/builtin/include/mbedtls/build_info.h has been moved to tf-psa-crypto/tests thus now a change in the spirit of what is proposed in https://github.com/Mbed-TLS/mbedtls/issues/9862#issuecomment-2943678825 can be done in 1.x/4.x.

[gilles-peskine-arm]

include/tf-psa-crypto/build_info.h to include/tf-psa-crypto/tf_psa_crypto_build_info.h

I don't think we should rename this one. It's part of the API. The header gets included with #include <tf-psa-crypto/build_info.h>, there's no ambiguity there.

include/tf-psa-crypto/check_config.h to include/tf-psa-crypto/tf_psa_crypto_check_config.h

Let's move it out of the public directory, which we already meant to do in 3.0 but didn't get around to. It's only needed when you build the library. When using the library, you must use the same config as when it was built. Checking that the config is consistent when using the library is pointless, and even bad because it gives a false sense of safety.

[ronald-cron-arm]

include/tf-psa-crypto/build_info.h to include/tf-psa-crypto/tf_psa_crypto_build_info.h

I don't think we should rename this one. It's part of the API. The header gets included with #include <tf-psa-crypto/build_info.h>, there's no ambiguity there.

That's fine by me. My only weak argument from my reading of the issue was: name too common, but that's not obviously the case.

include/tf-psa-crypto/check_config.h to include/tf-psa-crypto/tf_psa_crypto_check_config.h

Let's move it out of the public directory, which we already meant to do in 3.0 but didn't get around to. It's only needed when you build the library. When using the library, you must use the same config as when it was built. Checking that the config is consistent when using the library is pointless, and even bad because it gives a false sense of safety.

And we keep the name check_config.h then, right?

[gilles-peskine-arm]

And we keep the name check_config.h then, right?

We need to keep an include path (the string you pass to #include) that's different in Mbed TLS and TF-PSA-Crypto. If we put the file directly in library or core then we'll need different base names, e.g. #include "mbedtls_check_config.h" in Mbed TLS and #include "tf_psa_crypto_check_config.h" in TF-PSA-Crypto.

[irwir]

My only weak argument from my reading of the issue was: name too common, but that's not obviously the case.

Common or not, it still causes inconvenience. Quoting from the opening message as an example:

Now IDE would be showing the same name in tab headers if both build_info.h were opened.

[gilles-peskine-arm]

Now IDE would be showing the same name in tab headers if both build_info.h were opened.

I consider that a fairly weak argument: most IDEs should show an unambiguous name. And that applies especially for files that non-maintainers are unlikely to open in an IDE. For maintainers, showing an unambiguous name is critical, since we very often have the same file name open in multiple checkouts containing different branches.

[irwir]

I consider that a fairly weak argument: most IDEs should show an unambiguous name.

Unfortunately, well known IDEs such as Visual Studio or RAD Studio do not know they should do so. Tab headers simply have no space to show long paths - only file name is displayed. For very long names, part of if is replaced with ellipsis.

And that applies especially for files that non-maintainers are unlikely to open in an IDE.

Still happens, and maybe too frequently - when the code is not up to non-maintainer's requirements.

[bjwtaylor]

I currently have PR https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/366 completing this work, that's been sitting around for a while. Is it worth closing that PR and opening a new one once the scope of this task has been agreed?

[ronald-cron-arm]

I currently have PR Mbed-TLS/TF-PSA-Crypto#366 completing this work, that's been sitting around for a while. Is it worth closing that PR and opening a new one once the scope of this task has been agreed?

PR #366 is not related to this issue. Wrong PR number or this comment should be in another PR?

[bjwtaylor]

Apologies the one you need is https://github.com/Mbed-TLS/mbedtls/pull/10081

[ronald-cron-arm]

Apologies the one you need is #10081

Thanks. I think #10081 and Mbed-TLS/TF_PSA-Crypto#234 are quite far from what we are eventually going to do thus I would just close them. Sorry about that.

[bjwtaylor]

No worries, thanks for your advice. I'll close them.

[ronald-cron-arm]

Thanks for all the inputs. Thus eventually I would just rename common.h to tf_psa_crypto_common.h.

The move of tf-psa-crypto/crypto_config.h to core/tf_psa_crypto_crypto_config.h, while probably desirable, seems out of scope here. If I understand correctly, this would require moving its inclusion from build_info.h to (probably) common.h, which could have non-trivial consequences — though I haven’t tested this yet.

I believe the tab issue is also mitigated by the fact that, eventually, we aim to work primarily in Mbed TLS without needing to edit the underlying crypto code.

[gilles-peskine-arm]

I'm taking care of check_config.h in https://github.com/Mbed-TLS/mbedtls/pull/10319 and https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/387. It did have one non-trivial consequence: we were accidentally relying on the the inclusion of <limits.h> in check_config.h in several files.

[ronald-cron-arm]

Not completed yet.

[ronald-cron-arm]

I'm taking care of check_config.h in #10319 and Mbed-TLS/TF-PSA-Crypto#387. It did have one non-trivial consequence: we were accidentally relying on the the inclusion of <limits.h> in check_config.h in several files.

Great, thanks.

[irwir]

It did have one non-trivial consequence: we were accidentally relying on the the inclusion of <limits.h> in check_config.h in several files.

Possibly this could be fixed with <limits.h> included in build_info.h only.

[ronald-cron-arm]

Two PRs remaining.