[DRAFT] Split generated files for error and version_features #2351
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also, the test framework now supports negative dependencies, so let's take advantage of that. We now have, for each of the 3 libraries: - a positive test using something that's enabled in the default config - a negative test using something that's disabled in the default config So all 6 of these tests should be run in the default config, but some might be skipped in larger or smaller configs. In addition, "UNKNOWN" is kept as an always-available negative test, and `VERSION_FEATURES` is used for the (practically-)always-available positive test (if it's undefined, there's not point testing this function).
In our previous attempts at splitting libraries, we ended up with errors only indirectly detected by `ssl-opt.sh`. Let's catch those earlier in the future.
We're never calling it with arguments, and never from the root (checked in this repo and the one that contains release scripts) so we can simplify how sources and targets are set to make it clearer what file the script operates on.
The purpose of putting this data in a header is to allow Mbed Crypto and Mbed TLS to have different versions of that header. Thanks to include priorities, the Mbed TLS version will be picked when building Mbed Crypto as a submodule of Mbed TLS (just like the right version of config.h is picked). Note: this is a temporary state where tests/script/check-generated-files.sh will not pass. This will be fixed in the next commit. The purpose was to define what needs to be generated, before make the script generate it. Note: with this splitting in place, we could easily merge version_features.c back into version.c, as it doesn't make much sense to split the implementation of a single config.h module, VERSION_C, into two small C files. However that will not be done as part of this PR in order to minimize changes to the build system that might conflict with other currently-open PRs. Also, handling of runtime version information is likely to be reworked soonish, so it's probably no worth spending too much time on it now.
This was tested manually by running check-generated-files.sh and observing it passes, then modifying include/mbedtls/version_features.h, running the script again and observing it fails.
Same reasoning as for version_features, with slight technical differences due to the nature of the variable parts (code rather than data). Note: this is again a temporary state where check-generated-files.sh does not pass. This will be fixed in the next few commits.
It was also checked that this script was always invoked without options.
This commit was tested manually by running check-generated-files.sh and observing it pass, then modifying error_high.h before running it again and observing it fail.
mpg
added
enhancement
mbed TLS team
needs-review
| @@ -0,0 +1,375 @@ | |||
| /** | |||
| * \file error_high.h | |||
There was a problem hiding this comment.
Why are the generated header files (error_high.h, error_low.h, error_includes.h, version_features.h) in include/mbedtls and not in library? When would a user of the library ever use them?
There was a problem hiding this comment.
As discussed IRL and for reference here, for a PoC I went for the easiest solution, re-using the existing favourable include order that only applies to include/mbedtls, but for a final version we could adapt the include path to allow for a private include dir with the same virtualisation property.
|
@Patater I'm not familiar with the Mbed OS importer and the Mbed OS build system, so I forgot to ask during the workshop, but how would that kind of header virtualisation affect it? In particular:
Thanks in advance for clarifying that! |
|
Closing this draft as an actual PR is up with a new, cleaner approach: ARMmbed/mbed-crypto#97 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR, based on development-psa, is an exploration of a possible strategy to support deduplicating most files between mbedtls and mbed-crypto without breaking the API as an intermediate step. As such, it is a companion to #2349 and will soon be completed by a technical analysis document.
The key observation is that it's easy to have specific variants of one thing on the TLS side as long as it's in headers. This is thanks to the carefully chosen include order, originally intended for
config.h, and also used forversion.hin #2349. We can extend that strategy to the generated files containing error strings rest. feature strings, in order to have only the crypto part on the crypto side, but get the full version when building libmbedcrypto as a submodule of mbedtls.If the approach is adopted, this PR should be side-ported to mbed-crypto, then both should be merged before rebasing https://github.com/ARMmbed/mbedtls-psa/pull/232 and #2349, making some adaptations in the later (see below) and merging them.
The adaptation in #2349 would likely consist of just appending
<crypto/include/mbedtls/*.h> to the list on line 40 ofscripts/generate_errors.pl` and checking the result.Status
DRAFT - for discussion only! - Ping @sbutcher-arm and @Patater
Things known to be broken for now:
Tested and working so far:
make all testcmake . && make all testtests/scripts/check-generated-files.sh(plus manual negative testing)