This is an archive of the discontinued LLVM Phabricator instance.

[libc] Replace type punning with bit_cast
ClosedPublic

Authored by gchatelet on Feb 7 2022, 7:43 AM.

Details

Summary

Although type punning is defined for union in C, it is UB in C++.
This patch introduces a bit_cast function to convert between types in a safe way.

This is necessary to get llvm-libc compile with GCC.
This patch is extracted from D119002.

Diff Detail

Event Timeline

gchatelet created this revision.Feb 7 2022, 7:43 AM
gchatelet requested review of this revision.Feb 7 2022, 7:43 AM
lntue added inline comments.Feb 7 2022, 8:29 AM
libc/src/__support/CPP/Bit.h
35

Extra space around =, and extra closing parenthesis to be removed.

37–39

Is SIZE defined anywhere?

sivachandra added inline comments.Feb 7 2022, 9:35 AM
libc/src/fenv/fesetexceptflag.cpp
28

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?

gchatelet updated this revision to Diff 406743.Feb 8 2022, 2:33 AM
gchatelet marked 3 inline comments as done.
  • Address comments
libc/src/__support/CPP/Bit.h
35

Duh!? What happened here...

37–39

Bad copy/paste, thx for noticing.

libc/src/fenv/fesetexceptflag.cpp
28

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 integer conversion from unsigned to signed.

This currently works for GCC and Clang but the standard does not enforce this behavior before c++20.

gchatelet updated this revision to Diff 406744.Feb 8 2022, 2:34 AM
  • Remove leftover
gchatelet updated this revision to Diff 406752.Feb 8 2022, 2:47 AM

squash commits and rebase

gchatelet marked an inline comment as not done.Feb 8 2022, 5:56 AM

FYI I also had the following constness warning

/redacted/git/llvm-project/libc/src/fenv/fesetexceptflag.cpp: In function ‘int __llvm_libc::__fesetexceptflag_impl__(const fexcept_t*, int)’:
/redacted/git/llvm-project/libc/src/fenv/fesetexceptflag.cpp:23:24: warning: type qualifiers ignored on cast result type [-Wignored-qualifiers]
   23 |   int excepts_to_set = static_cast<const int>(*flagp) & excepts;
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gchatelet updated this revision to Diff 406804.Feb 8 2022, 6:51 AM
  • Add missing change
lntue accepted this revision.Feb 8 2022, 7:29 AM
This revision is now accepted and ready to land.Feb 8 2022, 7:29 AM
sivachandra accepted this revision.Feb 8 2022, 8:49 AM

I am not an expert, but normally the word "illegal" in code/commit logs is not preferred as it can imply non-technical errors. So, may be replace it with "incorrect".

gchatelet updated this revision to Diff 406864.Feb 8 2022, 8:55 AM

rebase and change commit message

gchatelet retitled this revision from [libc] Fix illegal type punning to [libc] Replace type punning with bit_cast.Feb 8 2022, 8:56 AM
gchatelet edited the summary of this revision. (Show Details)

I am not an expert, but normally the word "illegal" in code/commit logs is not preferred as it can imply non-technical errors. So, may be replace it with "incorrect".

Thx, I reworded it.

This revision was landed with ongoing or failed builds.Feb 8 2022, 12:46 PM
This revision was automatically updated to reflect the committed changes.