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

Make CIRCL runs on non-amd64 architectures. #121

Merged
merged 12 commits into from
Jun 2, 2020

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented May 27, 2020

Motivation: The principal offender why CIRCL breaks on s390x was due to endianness ordering.
Changes:

  • remove unsafe pointer conversion (as in C) since it breaks on arch with bigEndian ordering.
  • Updates CI, so it tests on arm64, amd64 and s390x.
  • Some packages do have non-arch specific implementations, but it wasn't compiling well. Now, it's fixed.
  • golangci-lint uses a github action.
  • s390x emulator breaks when multiple goroutines listen for signals. Setting GODEBUG='asyncpreemptoff=1' removes this restriction.

@armfazh armfazh added enhancement Improvement over something already in the project maintenance Related to issues on the repository fix-A-bug code that fix a bug labels May 27, 2020
@armfazh armfazh self-assigned this May 27, 2020
@armfazh armfazh requested review from wbl and bwesterb May 27, 2020 19:11
@armfazh armfazh mentioned this pull request May 27, 2020
@claucece
Copy link
Contributor

Awesome! I'll check this tomorrow. I was checking out possibilities for not using vendor. What do you think of this proposal: golang/go#32966 ?

We can also add a 32 architecture machine, now that we are it, as 'qemu-user-static' also supports arm32v6 and arm32v7...

math/fp25519/fp_generic.go Outdated Show resolved Hide resolved
math/fp25519/fp_generic.go Outdated Show resolved Hide resolved
@armfazh
Copy link
Contributor Author

armfazh commented May 29, 2020

Awesome! I'll check this tomorrow. I was checking out possibilities for not using vendor.

This PR does not requires to vendor anything.

What do you think of this proposal: golang/go#32966 ?

There might be cases for which this could be helpful.

We can also add a 32 architecture machine, now that we are it, as 'qemu-user-static' also supports arm32v6 and arm32v7...

Yes, that can be included in a different workflow triggered with a special tag. I want the current workflow runs fast.

@claucece
Copy link
Contributor

claucece commented Jun 1, 2020

This PR does not requires to vendor anything.

I was referring to go test -mod=vendor.

Yes, that can be included in a different workflow triggered with a special tag. I want the current workflow runs fast.

Sure. Looking at the docs, seems like using if will help: https://help.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idif

Copy link

@wbl wbl left a comment

Choose a reason for hiding this comment

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

Should we think about which architectures we want to test support for?

@armfazh
Copy link
Contributor Author

armfazh commented Jun 1, 2020

Should we think about which architectures we want to test support for?

Most of the current code targets AMD64 architecture, and most of the algorithms provide generic implementations relying on stdlib, ( and this is one way to support other architectures).

Testing on s390x allowed us to discover errors on our generic implementations without giving explicit support to s390x.

So, I think testing on other architectures is good. The only downside is that CI takes longer to execute.

@bwesterb
Copy link
Member

bwesterb commented Jun 1, 2020

Should we think about which architectures we want to test support for?

Definitely ARM64.

@armfazh armfazh merged commit b8e907c into cloudflare:master Jun 2, 2020
@armfazh armfazh deleted the fixingS390X branch July 24, 2020 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement over something already in the project fix-A-bug code that fix a bug maintenance Related to issues on the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants