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

Your byte implementation is unsafe under optimisation on GCC and clang #663

Closed
ned14 opened this issue Apr 16, 2018 · 11 comments
Closed

Your byte implementation is unsafe under optimisation on GCC and clang #663

ned14 opened this issue Apr 16, 2018 · 11 comments

Comments

@ned14
Copy link

ned14 commented Apr 16, 2018

The C++ 20 standard defines std::byte* to have the same may-alias properties as char*.

Your byte implementation therefore needs to be marked up with __attribute__((__may_alias__)) on GCC and clang compilers to tell the compiler that it must not assume that byte* cannot alias a pointer to a different type.

ned14 referenced this issue in martinmoene/byte-lite Apr 16, 2018
@neilmacintosh
Copy link
Collaborator

Sounds pretty reasonable to me. I'm guessing that's only necessary if someone is running without the -fno-strict-aliasing switch? Does anyone want to put together a PR?

@xaxxon
Copy link
Contributor

xaxxon commented Apr 17, 2018

only necessary if someone is running without the -fno-strict-aliasing switch

What was the point of calling that out?

@martinmoene
Copy link
Contributor

@xaxxon See the disscussion at PR #390.

@xaxxon
Copy link
Contributor

xaxxon commented Apr 17, 2018

a note to the readme, recommending to use the -fno-strict-aliasing flag on gcc and clang.

Just to be clear, we're not suggesting anyone prefer to flags to enable extensions to use non-standard c++, right?

@martinmoene
Copy link
Contributor

@xaxxon
Copy link
Contributor

xaxxon commented Apr 17, 2018

-fno-strict-aliasing is not c++17 behavior as far as I can tell. no-strict-aliasing treats everything as if it were a char* from what I can see.

@martinmoene
Copy link
Contributor

Ah, had the impression you were referring to __attribute__((__may_alias__)).

@ned14
Copy link
Author

ned14 commented Apr 17, 2018

Strict aliasing has been on by default in GCC for a very long time at -O2 and above. clang gained the same maybe four or five years ago. MSVC once had opt-in strict aliasing a long time ago, but they removed it and its magic compiler options.

In any case, on GCC and clang, __attribute__((__may_alias__)) needs to be applied to the byte type, else the compiler will hard assume it will never alias unrelated types, which is exactly what byte is for!

@jack17529
Copy link
Contributor

@neilmacintosh I think there is some issue regarding your Facebook link on your profile.
I am sorry but this nowhere related to the issue.
I too wanted to talk privately regarding GSL

@neilmacintosh
Copy link
Collaborator

@xaxxon I was simply asking a question, not "calling anything out". I wanted to understand the situations that would/wouldn't expose people to the problem that was being reported.

Both -fno-strict-aliasing and __attribute__ are language extensions as far as I am aware. Both are options for getting something like the behavior of C++17 std::byte if you don't yet have native support for it in your toolchain. So far, the attribute sounds like the path with less side-effects.

@jack17529 If you would like to contact me directly, I can be reached at neilmac@fb.com.

@StephanDollberg
Copy link
Contributor

Have ported @martinmoene's patch and opened a PR.

@annagrin annagrin self-assigned this Apr 25, 2018
annagrin pushed a commit that referenced this issue May 1, 2018
C++17 defines `std::byte*` to have the same aliasing properties as
`char*`. Hence, we mark it with the `__may_alias__` attribute under gcc
and clang.

Based on the fix by Martin Moene in byte-lite.

Fixes #663
@annagrin annagrin removed the open label May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants