Feature: code wrappers subpackage

[minosgalanakis]

This PR adds the placeholder directroy for the code-generation wrappers, refactors generate_psa_wrappers.py and add some minor utility functions in c_parsing_helper.py

In case it gets hidden by GitHub, the PR to incorporate this to the main repo is Mbed-TLS/mbedtls#9224

[valeriosetti]

I only left 1 comment for a (potential) extra new line. A part from that the rest of the PR looks good to me.

[gilles-peskine-arm]

Unfinished review because psa_buffer.py needs to be in mbedtls_framework.

[gilles-peskine-arm]

I've reviewed the changes in scripts/mbedtls_framework/c_parsing_helper.py.

For the rest, there's a big bunch of code that disappears in a8d6b10 and reappears with modifications in 4d3153b. This makes it effectively necessary to review the whole code from scratch, which is a waste of time.

Please restructure the commits so that code that is moved appears as a move with git diff --color-moved SHA, and changes to the content are done in separate commits. For example, in Mbed-TLS/mbedtls#8930, “docs: Move TLS 1.3 early data doc to a dedicated file” is a separate commit b372b2e with no content changes.

[gilles-peskine-arm]

This only captures some typedefs. Why those? What will happen if we edit a header and the typedef no longer follows that format, and how much of a risk is there that this happens?

I don't know what this function is meant to be used for, and currently it isn't used in #25 or Mbed-TLS/mbedtls#9224.

[minosgalanakis]

Reading simple typedefs is something that could be usefull for a FunctionInfo parsing class.

It is currently not being used, but will be used when parsing psa headers for generating serialisation logic. It can also be very usefull for debugging purposes since it generates a simple to read dictionary e.g

{'mbedtls_svc_key_id_t': 'psa_key_id_t',
 'psa_algorithm_t': 'uint32_t',
 'psa_dh_family_t': 'uint8_t',
 'psa_ecc_family_t': 'uint8_t',
 'psa_key_attributes_t': 'psa_key_attributes_s',
 'psa_key_derivation_step_t': 'uint16_t',
 'psa_key_id_t': 'uint32_t',
 'psa_key_lifetime_t': 'uint32_t',
 'psa_key_location_t': 'uint32_t',
 'psa_key_persistence_t': 'uint8_t',
 'psa_key_production_parameters_t': 'psa_key_production_parameters_s',
 'psa_key_slot_number_t': 'uint64_t',
 'psa_key_type_t': 'uint16_t',
 'psa_key_usage_t': 'uint32_t',
 'psa_status_t': 'int32_t'}

There is very little risk, since we don't change psa headers that much, and evem when we do, the data structures are very rarely updated. Also this will be used in test code generation so whoever uses it needs to make sure it is capturing everything the requirements need it to, and update it accordingly.

For now the simple parsing should work.

[gilles-peskine-arm]

Why isn't the struct captured in type?

[minosgalanakis]

Because we do not have to capture it to know it is a structure.

[gilles-peskine-arm]

We're getting close!

[gilles-peskine-arm]

(Resuming #25 (comment))

These all should be instance variables, not class variables. Otherwise we will be very confused when we instantiate or override the class.

Why isn't the class a NamedTuple?

Can you elaborate on how that should be addressed?

Either use a NamedTuple, or make it a data class, as in, a class whose sole purpose is to store data into fields — what C would call a struct. Pylint doesn't like data classes, but it doesn't have a very strong justification. It wants to drive you to the mechanisms for building data classes through the standard library, but those mechanisms aren't particularly useful unless you want to do some introspection, which we don't care about here.

A custom data class would be just a constructor that initializes the fields to their default value.

class PSAWrapperConfiguration:
    """Configuration data class for PSA Wrapper."""
    def __init__(self) -> None:
        self.cpp_guards = ["MBEDTLS_PSA_CRYPTO_C", "MBEDTLS_TEST_HOOKS", "!RECORD_PSA_STATUS_COVERAGE_LOG"]
        …

[minosgalanakis]

I am getting a bit of mixed signals, as to why we are going around in circles on this one. The aim was to use a generic key:value configuration datastore, which allows other modules to import and .update() as required.

Then as requested this was changed into a class because a using a dictionary is stringy typing? Now I am being asked to use a NamedTuple which completely fails the original intent, and is also stringy typing?

Tuples are immutable, which means in order to update an entry the consumer will have to recreate it. There is a reason that dictionaries are universaly used for configuration input.

Now for dataclass argument, yes I understand that this is not a dataclass as introduced in Python3.7, but the proposal is to basically implement in Python3.6 what the dataclass decorator does.

Is that all that for the shake of adding-complexity really needed?

[gilles-peskine-arm]

The aim was to use a generic key:value configuration datastore, which allows other modules to import and .update() as required.

Uh? That's the first time this comes up. Why should this be a generic configuration datastore? Why would modules add their own configuration entries that the base class doesn't know about? That seems like a solution in need of a problem.

a NamedTuple which completely fails the original intent, and is also stringy typing?

A NamedTuple isn't extensible. But extensibility is a new requirement and I don't see the point of this new requirement.

A NamedTuple is not stringly typed. Its type is the NamedTuple. The set of keys is known, and the type of the key corresponding to each value is known.

Note once again that I'm not saying you have to use a NamedTuple. I'd be happy if you just made the change I posted above, changing the class fields into instance fields. With # pylint: disable=too-few-public-methods because Pylint is opinionated about not doing your own data class, but there isn't a strong objective justification behind that opinion.

[gilles-peskine-arm]

Almost there

[gilles-peskine-arm]

LGTM except one edge case

[gilles-peskine-arm]

LGTM