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

Remove backend name from default namespace #618

Merged
merged 3 commits into from
Jan 7, 2025
Merged

Conversation

hanno-becker
Copy link
Contributor

Previously, the backend type would enter the default namespace
used by mlkem-native.

This means that before the MLKEM_NAMESPACE/FIPS202_NAMESPACE macros
can be used, the backend header has to be included.

This is inconvenient for the introduction of a public API header which
solely depends on the config file, since that header would need to also
include the backend header, and hence be compiled with the same include
paths as the mlkem-native build itself. This is unintuitive and unnecessarily
complicated for the customer.

This PR removes the backend type from the default namespace. Customers
who need the backend type, or at least the target architecture, to be
reflected in the namespace, can overwrite the default namespace as desired.

Further smaller changes related to the config are made; see commits.

@hanno-becker
Copy link
Contributor Author

cc @bhess: With this, you need to explicitly set the namespace to reflect the backend/architecture. Please let us know if this is of any concern.

@hanno-becker hanno-becker force-pushed the default_namespace branch 4 times, most recently from 22593af to 2e9968d Compare January 6, 2025 07:10
Previously, the backend type would enter the default namespace
used by mlkem-native.

This means that before the MLKEM_NAMESPACE/FIPS202_NAMESPACE macros
can be used, the backend header has to be included.

This is inconvenient for the introduction of a public API header which
solely depends on the config file, since that header would need to also
include the backend header, and hence be compiled with the same include
paths as the mlkem-native build itself. This is unintuitive and unnecessarily
complicated for the customer.

This commit removes the backend type from the default namespace. Customers
who need the backend type, or at least the target architecture, to be
reflected in the namespace, can overwrite the default namespace as desired.

Signed-off-by: Hanno Becker <[email protected]>
mlkem-native's source and header files should instead include `common.h`.

This commit fixes a few header and source files in this regard.

Signed-off-by: Hanno Becker <[email protected]>
Previously, `common.h` would explicitly include a custom config
if present, but not explicitly include the default `config.h`
otherwise. Instead, this would be left to `params.h`, which _always_
includes the default config `config.h`, but not a custom config.

This happens to be OK if custom configs and default configs use
the same header guard, but is confusing and non-uniform nonetheless.

This commit uniformly uses the pattern

```
 #if defined(MLKEM_NATIVE_CONFIG_FILE)
 #include MLKEM_NATIVE_CONFIG_FILE
 #else
 #include "config.h"
 #endif /* MLKEM_NATIVE_CONFIG_FILE */
```

to include the configuration.

Signed-off-by: Hanno Becker <[email protected]>
@hanno-becker hanno-becker marked this pull request as ready for review January 6, 2025 07:49
@hanno-becker hanno-becker requested a review from a team as a code owner January 6, 2025 07:50
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mkannwischer mkannwischer merged commit 72c7fbf into main Jan 7, 2025
122 checks passed
@mkannwischer mkannwischer deleted the default_namespace branch January 7, 2025 08:50
@bhess
Copy link
Contributor

bhess commented Jan 10, 2025

cc @bhess: With this, you need to explicitly set the namespace to reflect the backend/architecture. Please let us know if this is of any concern.

Thanks for tagging me, @hanno-becker. Explicitly setting the namespace isn’t a problem, but it would be more convenient if the namespace prefix (e.g., PQCP_MLKEM_NATIVE_MLKEM1024_AARCH64_OPT) could be set via CFLAGS.

Unless I missed something, it appears that currently, one needs to overwrite the MLKEM_NAMESPACE(x) macro by either patching or providing a custom config.h. Would it be possible to introduce an optional compile-time parameter -e.g., MLKEM_NAMESPACE_PREFIX - that defines MLKEM_NAMESPACE(x) as MLKEM_NAMESPACE_PREFIX##_x?

@hanno-becker
Copy link
Contributor Author

@bhess That's no problem, I will open a PR.

@hanno-becker
Copy link
Contributor Author

@bhess Can you have a look at #635 and see if that's what you had in mind, and whether it makes your life in OQS easier?

@bhess
Copy link
Contributor

bhess commented Jan 10, 2025

Great, thanks! Looks like what I had in mind 👍 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants