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

[compiler-rt][builtins] Switch to using __builtin_memcpy_inline #69784

Closed

Conversation

alexander-shaposhnikov
Copy link
Collaborator

@alexander-shaposhnikov alexander-shaposhnikov commented Oct 20, 2023

Switch to using __builtin_memcpy_inline.
This code has existed for quite a while, to the best of my knowledge compilers
try to be nice not to exploit this UB (writing to one field of a union and reading from another),
yet strictly speaking this UB.

Test plan: ninja && ninja check-compiler-rt

@frederick-vs-ja
Copy link
Contributor

Would it be simpler to use __builtin_bit_cast?

@alexander-shaposhnikov
Copy link
Collaborator Author

__builtin_bit_cast seems to be available only in C++ (https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html), not in C

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I think that the functional change here is fine and the right thing to do. I worry about compatibility with compilers. Could you please do a follow up (or merge into this change) a change to ensure that the compiler supports this builtin and if not, report a proper error.

@alexander-shaposhnikov
Copy link
Collaborator Author

@compnerd - yeah, MSVC doesn't seem to support it https://godbolt.org/z/EbPhMKqdz

@compnerd
Copy link
Member

@alexander-shaposhnikov, right. Older clang will also not support this (it was added in 2020?). I think that we should double check with @rnk and @HansW about the need for building the builtins with MSVC support.

@alexander-shaposhnikov
Copy link
Collaborator Author

alexander-shaposhnikov commented Oct 23, 2023

I'm under impression that i've seen build bots failures (related to compiler-rt/builtins) on Windows, so unfortunately I suspect I won't be able to land this change for now.

@frederick-vs-ja
Copy link
Contributor

__builtin_bit_cast seems to be available only in C++ (https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html), not in C

Oh, I'm sorry for the wrong suggestion. It seems that only clang supports __builtin_bit_cast in C (demo).

Do these builtins even need to be compiled in C++? I think the existing approach is UB in C++ but not in C.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants