[libc-commits] [PATCH] D119145: [libc] Fix illegal type punning

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/src/__support/CPP/Bit.h:35
+  char *dst = reinterpret_cast<char *>(&to);
+  const char* src=reinterpret_cast<const char *>(&from));
+#if defined(LLVM_LIBC_HAS_BUILTIN_MEMCPY_INLINE)
----------------
lntue wrote:
> Extra space around `=`, and extra closing parenthesis to be removed.
Duh!? What happened here...


================
Comment at: libc/src/__support/CPP/Bit.h:37-39
+  __builtin_memcpy_inline(dst, src, SIZE);
+#else
+  for (unsigned i = 0; i < SIZE; ++i)
----------------
lntue wrote:
> Is SIZE defined anywhere?
Bad copy/paste, thx for noticing.


================
Comment at: libc/src/fenv/fesetexceptflag.cpp:28
                 "fexcept_t value cannot fit in an int value.");
-  int excepts_to_set = static_cast<const int>(*flagp) & excepts;
+  using IntType = TypeSelector<sizeof(fexcept_t)>::type;
+  const int excepts_as_int = __llvm_libc::bit_cast<IntType>(*flagp);
----------------
sivachandra wrote:
> I am still confused. Previously, we de-referenced (`*flagp`) before static-casting it. So, there is no type-punning involved (assuming `fexcept_t` is of integral type), no? Can you share the error you are seeing?
Oh I see, there is no type punning here indeed. I got confused by using `bit_cast` that expects both type to have exact same size.

That said this is still //implementation defined// because of [[ https://en.cppreference.com/w/c/language/conversion#:~:text=otherwise%2C%20if%20the%20target%20type%20is%20signed%2C%20the%20behavior%20is%20implementation%2Ddefined%20(which%20may%20include%20raising%20a%20signal) | integer conversion from unsigned to signed ]].

This currently works for GCC and Clang but the standard does not enforce this behavior [[ https://en.cppreference.com/w/cpp/20#:~:text=signed%20integers%20are%202%27s%20complement | before c++20 ]].


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119145/new/

https://reviews.llvm.org/D119145