Skip to content

Conversation

@antznin
Copy link

@antznin antznin commented May 27, 2025

The current logic for enabling or disabling the CRYPTOPP_ARM_*_AVAILABLE macros seems wrong, because we do an OR between all of the conditions following defined(__ARM_FEATURE_CRYPTO). Fix the logic so that the CRYPTOPP_ARM_*_AVAILABLE macros are set if __ARM_FEATURE_CRYPTO is set AND any of the following condition is true.

The current logic for enabling or disabling the CRYPTOPP_ARM_*_AVAILABLE
macros seems wrong, because we do an OR between all of the conditions
following `defined(__ARM_FEATURE_CRYPTO)`. Fix the logic so that the
CRYPTOPP_ARM_*_AVAILABLE macros are set if __ARM_FEATURE_CRYPTO is set
_AND_ any of the following condition is true.

Signed-off-by: Antonin Godard <[email protected]>
@antznin antznin force-pushed the fix-header-logic branch from 1eb7a64 to 765e4d1 Compare May 27, 2025 07:15
@irwir
Copy link
Contributor

irwir commented Jun 12, 2025

The current logic for enabling or disabling the CRYPTOPP_ARM_*_AVAILABLE macros seems wrong

OR'ed condition contains a list of known and allowed compilers; otherwise __ARM_FEATURE_CRYPTO could be used to force enabling of CRYPTOPP_ARM_*_AVAILABLE macros.

@antznin
Copy link
Author

antznin commented Jun 28, 2025

The current logic for enabling or disabling the CRYPTOPP_ARM_*_AVAILABLE macros seems wrong

OR'ed condition contains a list of known and allowed compilers; otherwise __ARM_FEATURE_CRYPTO could be used to force enabling of CRYPTOPP_ARM_*_AVAILABLE macros.

That was the case before my patch, yes.

Now after my modification, a known and allowed compiler must be used and __ARM_FEATURE_CRYPTO must be set.

So I'm not sure I understand your comment.

@irwir
Copy link
Contributor

irwir commented Jun 29, 2025

Now after my modification, a known and allowed compiler must be used and __ARM_FEATURE_CRYPTO must be set.

Incorrect interpretation of code logic.
Should cryptopp need a single on/off switch here, then __ARM_FEATURE_CRYPTO would be checked before other conditions. In fact, all three blocks you suggested to change could have been placed under one check.

@antznin
Copy link
Author

antznin commented Jul 3, 2025

Should cryptopp need a single on/off switch here, then __ARM_FEATURE_CRYPTO would be checked before other conditions. In fact, all three blocks you suggested to change could have been placed under one check.

Sure, I can change the code so __ARM_FEATURE_CRYPTO is checked first, then the three compilers. This also makes sense to me. But I'm not sure that what we want after your comments.

@irwir
Copy link
Contributor

irwir commented Jul 3, 2025

This PR goes against portability, because any non-listed compiler would require source code changes.

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.

2 participants