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.

Sign up for GitHub

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

Jump to bottom

Adds sha3 to arm compile options which is needed for ios compilation. #1934

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Adds sha3 to arm compile options which is needed for ios compilation. #1934

wants to merge 1 commit into from

Conversation

fwh-dc
Copy link

@fwh-dc fwh-dc commented Sep 25, 2024

Fixes #1933

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@Martyrshot
Copy link
Member

Thanks for the contribution! How would this behave on arm platforms that do not have the sha3 extensions?

@fwh-dc
Copy link
Author

fwh-dc commented Sep 26, 2024

Thanks for the contribution! How would this behave on arm platforms that do not have the sha3 extensions?

TBH I don't know and I've trouble finding documentation on these flags. Perhabs you have some suggestions on where to look?

baentsch
baentsch reviewed Sep 26, 2024
View reviewed changes
.CMake/compiler_opts.cmake
@@ -55,7 +55,7 @@ if(CMAKE_C_COMPILER_ID MATCHES "Clang|GNU")
else()
# Assume sensible default like -march=x86-64, -march=armv8-a, etc.
if(ARCH_ARM64v8)
set(OQS_OPT_FLAG "-march=armv8-a+crypto")
set(OQS_OPT_FLAG "-march=armv8-a+crypto+sha3")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not guarded/if'd by reference to ios ("APPLE"?) if it's indeed a problem only there?

Copy link
Author

@fwh-dc fwh-dc Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if it is Apple specific. As I stated in my previous comment I wasn't able to find any documentation on the flags. Do you know where to find the documentation? Maybe this affects other vendors than Apple?

Copy link
Member

Choose a reason for hiding this comment

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

Do you know where to find the documentation?

Nope -- I'm out of my depth with ARM64.

Copy link
Author

Choose a reason for hiding this comment

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

I'll take this one back to draft and have a look at your suggestion in the related issue. Maybe I can get it to work with the toolchain file of this project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @fwh-dc , this PR makes compilation on Cortex-A53 (as you can see in CI) failed. It does not have SHA3 extension.

@fwh-dc fwh-dc marked this pull request as draft September 26, 2024 08:52
@dstebila
Copy link
Member

Tagging @hanno-becker to see if he has any insights on ARM compilation flags.

@hanno-becker
Copy link

I would recommend only using the sha3-flag if one is certain that the target platform supports it. Even if the SHA3-instruction-based Keccak implementation is not actually called, there is risk that a clever compiler will leverage general-purpose instructions such as eor3 in (auto-)vectorized code, leading to an invalid instruction abort at runtime.

How does one detect/convey more details about the target in CMake?

In addition, feat.S should unconditionally guard the code by __ARM_FEATURE_SHA3 (even on Apple).

@vincentvbh
Copy link

I would recommend only using the sha3-flag if one is certain that the target platform supports it. Even if the SHA3-instruction-based Keccak implementation is not actually called, there is risk that a clever compiler will leverage general-purpose instructions such as eor3 in (auto-)vectorized code, leading to an invalid instruction abort at runtime.

How does one detect/convey more details about the target in CMake?

In addition, feat.S should unconditionally guard the code by __ARM_FEATURE_SHA3 (even on Apple).

On my Apple M1, __ARM_FEATURE_SHA3 is not defined by default (it will be defined if one compiles with -march=armv8-a+sha3). So I agree with Hanno that currently by default we should compile with -march=armv8-a. A feature detection is recommended to pass +sha3 conditionally.

@vincentvbh
Copy link

Thanks for the contribution! How would this behave on arm platforms that do not have the sha3 extensions?

TBH I don't know and I've trouble finding documentation on these flags. Perhabs you have some suggestions on where to look?

The testing macros can be found here: https://developer.arm.com/documentation/101028/0012/5--Feature-test-macros. As hardware varies, you might want to test your target platforms yourself.

@fwh-dc
Copy link
Author

fwh-dc commented Oct 8, 2024

Nice thanks. I'll see when I can find the time to have a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baentsch baentsch baentsch left review comments

@cothan cothan cothan left review comments

@dstebila dstebila Awaiting requested review from dstebila dstebila is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compilation error: <instantiation>:16:5: error: instruction requires: sha3
7 participants
@fwh-dc @Martyrshot @dstebila @hanno-becker @vincentvbh @cothan @baentsch