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 undefined behavior in PxUnionCast #65

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

Conversation

rsfb
Copy link

@rsfb rsfb commented Mar 28, 2018

Fixes crash in linux with a new version of clang.

The C++ standard does not define behavior of accessing a different union member from the one that was last written. std::memcpy is a defined way to accomplish the same.

Fixes crash in linux with a new version of clang.

The C++ standard does not define behavior of accessing a different union member from the one that was last written. std::memcpy is a defined way to accomplish the same.
@AlesBorovicka
Copy link
Contributor

Hi,
please, can you give us the clang version so we can try to reproduce the crash on our side?
Thanks a lot,
regards Ales

@rsfb
Copy link
Author

rsfb commented Mar 28, 2018

Hi Ales,

Thanks for the quick reply! LLVM versions r327616 and r327695 were the ones we tested.

A bit more detail:

In PsUnixSse2InlineAoS.h there's a gMaskXYZ constant which is initialised with the output of PxUnionCast - with these new compiler versions, the mask had all zeros on all 4 components (instead of all 1, all 1, all 1, all 0 as intended).

Cheers,
Ricardo

@rsfb
Copy link
Author

rsfb commented Apr 24, 2018

Hi again Ales,

Was this or a similar fix submitted? Currently we have fixed the gMaskXYZ = 0 problem with the change in this PR.

Regards,
Ricardo

@AlesBorovicka
Copy link
Contributor

Hi Ricardo,
no, we have not fixed the issue yet. Still not resolved.
Regards,
Ales

@AlesBorovicka
Copy link
Contributor

The issue should be resolved in next update. Thanks for the report!

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