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

Fix BINARY_ALIGN in a few OpenCL formats #5639

Merged

Conversation

magnumripper
Copy link
Member

It's amazing we still have this problem 12 years after introducing the alignment warnings.

Copy link
Member

@solardiz solardiz left a comment

Choose a reason for hiding this comment

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

FWIW, as I recall from PHC list discussions, only char and unsigned char are allowed to alias any type. uint8_t isn't. Of course, in practice it's usually handled just the same as unsigned char, but theoretically a compiler is free to optimize things away when uint8_t * aliases something larger. So I suggest changing those two instances here to unsigned char *.

@solardiz
Copy link
Member

It's amazing we still have this problem 12 years after introducing the alignment warnings.

I think the reasons for this are:

  1. Just because greater expected alignment is not guaranteed does not mean it won't be there in a specific build/run - quite often it will be.
  2. I think our CI runs don't check for those warnings at all - they're just buried in the logs. Maybe we should fix that.
  3. Our CI setup tests only a handful of OpenCL formats (as part of ci/circleci: encoding-opencl, which is mostly an encoding test). We should add explicit OpenCL tests.

@magnumripper
Copy link
Member Author

only char and unsigned char are allowed to alias any type. uint8_t isn't.

So the standard says "a character type". That begs the question whether wchar_t is "a character type" and consequently can alias anything. I expect the answer to that is "no" which makes me really hope OpenCL's uchar is "a character type".

I'll fix it, to increase the chance of remembering this.

It's amazing we still have this problem 12 years after introducing the
alignment warnings.
@magnumripper magnumripper merged commit cf7bcbc into openwall:bleeding-jumbo Dec 25, 2024
35 of 36 checks passed
@magnumripper magnumripper deleted the opencl-binary-align branch December 25, 2024 13:07
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