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

Add bit_cast, and use it for _beginthreadex calls #3380

Merged
merged 2 commits into from
Apr 16, 2022

Conversation

N-Dekker
Copy link
Contributor

bit_cast helps to avoid undefined behavior when doing a bitwise cast.

Added a rudimentary `bit_cast` implementation for C++14/C++17, mimicking
C++20 `std::bit_cast`.
A C-style cast of a `uintptr_t` (the return type of `_beginthreadex`) to a
pointer type (`void *` or `HANDLE`) may easily lead to undefined behavior.
This is avoided hereby, by using `bit_cast` instead.
@github-actions github-actions bot added area:Core Issues affecting the Core module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Apr 14, 2022
@N-Dekker N-Dekker marked this pull request as ready for review April 15, 2022 12:14
@dzenanz dzenanz merged commit 548b45f into InsightSoftwareConsortium:master Apr 16, 2022
@N-Dekker
Copy link
Contributor Author

@dzenanz I'm sorry, my claim that the (HANDLE) cast would cause undefined behavior was too strong. (Meaning I was wrong 😸.) As was just explained to me at the #include <C++> discussion channel, referring to http://eel.is/c++draft/expr.reinterpret.cast#4

A pointer can be explicitly converted to any integral type large enough to hold all values of its type. The mapping function is implementation-defined.

In this particular case, the (HANDLE) cast would behave just like a reinterpret_cast<HANDLE>.

I still think that in this particular case, bit_cast<HANDLE> is preferable to reinterpret_cast<HANDLE>, because the behavior of C++20 bit_cast is defined by the C++ Standard, rather than just "implementation-defined".

In other cases, bit_cast does actually prevent undefined behavior, for example (https://godbolt.org/z/xearxvxM6):

float f = 0.1f;
int i1 = reinterpret_cast<int & >(f); // Undefined behavior!
int i2 = bit_cast<int>(f); // OK

@N-Dekker
Copy link
Contributor Author

For the record, this pull request + discussion appears related to issue isocpp/CppCoreGuidelines#1517 "Favor bit_cast over reinterpret_cast".

@N-Dekker
Copy link
Contributor Author

@Leengit Could you possibly have a look at this issue as well? It seems that I was too eager to start using bit_cast, rather than reinterpret_cast! You may find the discussion at issue isocpp/CppCoreGuidelines#1517 "Favor bit_cast over reinterpret_cast" interesting. 😄

Jason Turner has two short video's on this topic, however, they don't seem to apply specifically to the (HANDLE) cast that I was addressing with this pull request:

So I'm now considering to replace the itk::bit_cast<HANDLE> calls from this pull request by reinterpret_cast's. What do you think?

@Leengit
Copy link
Contributor

Leengit commented Apr 21, 2022

This is a new one for me and I haven't followed all your links, so I may end up wishing to eat my words in the next few hours or minutes but ... I am thinking that I would continue to use reinterpret_cast for objects of different sizes and for arrays and pointers. I would use bit_cast only if the from and to objects are neither pointers nor arrays and are of the same size.

  double * pDouble = nullptr;
  // this next line is good enough because it is to char* (assuming sizeof(char)==1) or void*
  char * pChar = reinterpret_cast<char *>(pDouble);

  float        f = 1.0f;
  unsigned int i1 = std::bit_cast<unsigned int>(f);        // yes
  unsigned int i2 = *reinterpret_cast<unsigned int *>(&f); // now deprecated

And if they are not trivially copyable then I don't know.

N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Apr 26, 2022
It appears that commit 548b45f ("BUG: Replace
C-style casts from `_beginthreadex` with `bit_cast<HANDLE>`", from pull request
InsightSoftwareConsortium#3380) was wrong:
`bit_cast<HANDLE>` might do a different conversion than the corresponding
C-style cast, `(HANDLE)`.

The C-style cast `(HANDLE)` behaves exactly like `reinterpret_cast<HANDLE>`, by
definition.

`reinterpret_cast<HANDLE>` does an implementation-defined conversion, as was
explained by Jonathan Wakely at isocpp/CppCoreGuidelines#1517 (comment)
(issue "Favor bit_cast over reinterpret_cast").

Fixed by replacing all `bit_cast<HANDLE>` calls with `reinterpret_cast<HANDLE>`.
dzenanz pushed a commit that referenced this pull request Apr 26, 2022
It appears that commit 548b45f ("BUG: Replace
C-style casts from `_beginthreadex` with `bit_cast<HANDLE>`", from pull request
#3380) was wrong:
`bit_cast<HANDLE>` might do a different conversion than the corresponding
C-style cast, `(HANDLE)`.

The C-style cast `(HANDLE)` behaves exactly like `reinterpret_cast<HANDLE>`, by
definition.

`reinterpret_cast<HANDLE>` does an implementation-defined conversion, as was
explained by Jonathan Wakely at isocpp/CppCoreGuidelines#1517 (comment)
(issue "Favor bit_cast over reinterpret_cast").

Fixed by replacing all `bit_cast<HANDLE>` calls with `reinterpret_cast<HANDLE>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants