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

Use memcpy for unaligned reads to appease UBSan #7

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

Conversation

SpexGuy
Copy link

@SpexGuy SpexGuy commented Dec 31, 2021

This change uses memcpy to perform unaligned reads, instead of reinterpret_cast. In tests on godbolt, this generated a single unaligned read on x86 and x86_64 platforms even under -O0. Other platforms generate byte reads and then stitch them together with shifting. Writing it this way removes the need for the build to force byte reads when UBSan is enabled. However, unfortunately FPNG_USE_UNALIGNED_LOADS is still needed for performance in READ_RGB_PIXEL, because most compilers cannot remove the vestigial fourth byte read on platforms that require aligned reads.

@SpexGuy
Copy link
Author

SpexGuy commented Jan 1, 2022

Hmm, actually it looks like MSVC takes a little more convincing, it will actually call memcpy on /O1, it needs /O2 to generate good code. Depending on how important debug mode performance is on MSVC, this may or may not be worth integrating.

@richgel999
Copy link
Owner

richgel999 commented Jan 1, 2022

Thanks - let me look at this and profile it. (This stuff is way trickier than it should be in C/C++.) Not concerned very much with debug perf, as long as it's good enough.

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