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

keccak: guard against misaligned memory accesses on ARM #5724

Merged
merged 1 commit into from
Jul 13, 2019

Conversation

moneromooo-monero
Copy link
Collaborator

No description provided.

for ( ; inlen >= rsiz; inlen -= rsiz, in += rsiz) {
for (i = 0; i < rsizw; i++) {
uint64_t ina;
memcpy(&ina, &((uint64_t*)in)[i], 8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't the source pointer be (void *)? otherwise the compiler will still assume uint64_t alignment, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I also added a better unit test for this. I can't seem to be able to connect to ssh for now, I'll push later when I can.

Choose a reason for hiding this comment

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

Tested and implemented. This will be successful

Copy link
Contributor

@vtnerd vtnerd left a comment

Choose a reason for hiding this comment

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

When I've disassembled similar code in the past, GCC / Clang will generate identical code whether its an uint64_t cast/alias or a memcpy when the target platform supports unaligned accesses. When the platform doesn't, they will fall back into the slower copy code (but typically don't call the library memcpy function either).

Instead of carrying two versions - is it worth always doing the memcpy? The penalty will be slightly slower when armv7 (or similar) has an aligned memory access case since it won't branch into the cast/alias version. But it will be less code duplication in our codebase and ASM.

The code generated is exactly the same as the direct access
one on x86_64
@moneromooo-monero
Copy link
Collaborator Author

It was 100% identical code indeed. I changed it to only use the memcpy version now.

@luigi1111 luigi1111 merged commit c223832 into monero-project:master Jul 13, 2019
luigi1111 added a commit that referenced this pull request Jul 13, 2019
c223832 keccak: guard against misaligned memory accesses on ARM (moneromooo-monero)
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.

6 participants