Skip to content
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

Bignum: stop duplicating test function arguments for 32-bit vs 64-bit #6370

Closed
gilles-peskine-arm opened this issue Sep 26, 2022 · 3 comments
Labels
component-crypto Crypto primitives and low-level interfaces enhancement priority-medium Medium priority - this can be reviewed as time permits size-m Estimated task size: medium (~1w)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Sep 26, 2022

Some new bignum unit tests use functions that take arguments in pairs, with one argument used on 32-bit systems and one on 64-bit systems. This is useful because in some cases, the interesting inputs, or even some outputs such as carries, depend on the limb size. However, passing pairs of arguments in this way makes lists of test cases harder to read, complicates the test function code, and means that the same test case uses different data on different platforms.

The goal of this issue is to replace this idiom by one that does not involve passing different arguments depending on the limb size. Instead, if some test data is only valid for a given limb size, make the test case depend on MBEDTLS_HAVE_INTnn.

The test data generation framework (generate_bignum_tests.py) is now available to generate pairs of similar test cases in a convenient, easy-to-review way.

Definition of done: no more duplicated test arguments A4/A8, no more conditionals on defined(MBEDTLS_HAVE_INTnn) or sizeof(mbedtls_mpi_uint) in test_suite_mpi*.function.

Follow-up: #6371

@tom-cosgrove-arm
Copy link
Contributor

Marking this as priority-medium as it should be tackled after the priority-high items. Note that already some of this is happening organically, so this will likely be a mop-up task anyway

@tom-cosgrove-arm tom-cosgrove-arm added the priority-medium Medium priority - this can be reviewed as time permits label Oct 24, 2022
@yanesca yanesca added size-m Estimated task size: medium (~1w) and removed size-s Estimated task size: small (~2d) labels Nov 30, 2022
@yanesca
Copy link
Contributor

yanesca commented Nov 30, 2022

These same functions are not using the uniform test generation classes (which were added later). Doing the switch should include switching to the new python superclasses in test generation (OperationCommon and ModOperationCommon). Retaining identical test cases to the original is not a goal, should switch to the common inherited dataset.

Each function being reworked should be handled in a separate PR.

@yanesca
Copy link
Contributor

yanesca commented Dec 30, 2022

Closing as more specific tasks have been raised to track this issue.

@yanesca yanesca closed this as completed Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement priority-medium Medium priority - this can be reviewed as time permits size-m Estimated task size: medium (~1w)
Projects
None yet
Development

No branches or pull requests

3 participants