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

crypto/sha256: Use pragmas to enforce necessary intrinsics for GCC and Clang #13789

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@luke-jr
Copy link
Member

luke-jr commented Jul 28, 2018

This avoids problems when the user specifies CXXFLAGS explicitly disabling the relevant optimisations.

Partially fixes #13758, only for GCC/Clang.

NOTE: src/leveldb/port/port_posix_sse.cc has the same issue (only with our build system?), which is NOT addressed here.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 28, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13442 (Convert the 1-way SSE4 SHA256 code from asm to intrinsics by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

crypto/sha256: Use pragmas to enforce necessary intrinsics for GCC an…
…d Clang

This avoids problems when the user specifies CXXFLAGS explicitly disabling the relevant optimisations.

@luke-jr luke-jr force-pushed the luke-jr:bugfix_asm_pragmas branch to 8bca9cd Jul 29, 2018

@MarcoFalke MarcoFalke added this to the 0.17.0 milestone Jul 30, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 31, 2018

Not sure this is the right way to go about it. IMO passing the correct compilation flags is something that needs to be fixed in the build system, not worked around with compiler-specific pragmas in the code.

@laanwj laanwj added the Build system label Jul 31, 2018

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 31, 2018

crypto/sha256_sse4.cpp doesn't actually need this, as it's written using inline assembly rather than compiler intrinsics. The assembly is passed through to the assembler without the compiler interpreting or understanding.

@MarcoFalke MarcoFalke removed this from the 0.17.0 milestone Aug 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.