Page MenuHomePhabricator

[libc++] Implement C++20's P0476r2: std::bit_cast
Needs ReviewPublic

Authored by ldionne on Mar 10 2020, 1:43 PM.

Details

Reviewers
jfb
EricWF
mclow.lists
Group Reviewers
Restricted Project

Diff Detail

Unit TestsFailed

TimeTest
890 mslibcxx CI -fno-exceptions > libc++.libcxx/diagnostics::nodiscard_extensions.verify.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/libcxx/diagnostics/nodiscard_extensions.verify.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -fno-exceptions -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/test/libcxx/diagnostics/Output/nodiscard_extensions.verify.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_ENABLE_NODISCARD -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
980 mslibcxx CI -fno-exceptions > libc++.libcxx/thread/thread_lock/thread_lock_guard::nodiscard.verify.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -fno-exceptions -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/Output/nodiscard.verify.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_ENABLE_NODISCARD -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
1,310 mslibcxx CI -fno-exceptions > libc++.std/numerics/bit/bit_cast::bit_cast.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -fno-exceptions -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/projects/libcxx/test/std/numerics/bit/bit.cast/Output/bit_cast.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/74c88f32001e-1/llvm-project/libcxx-ci/build/generic-noexceptions/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -lc++experimental…
1,060 mslibcxx CI ASAN > libc++.libcxx/diagnostics::nodiscard_extensions.verify.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/test/libcxx/diagnostics/nodiscard_extensions.verify.cpp -v --target=x86_64-unknown-linux-gnu -g -fno-omit-frame-pointer -fsanitize=address -include /home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -O1 -std=c++2a -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/test/libcxx/diagnostics/Output/nodiscard_extensions.verify.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_ENABLE_NODISCARD -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
1,280 mslibcxx CI ASAN > libc++.libcxx/thread/thread_lock/thread_lock_guard::nodiscard.verify.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp -v --target=x86_64-unknown-linux-gnu -g -fno-omit-frame-pointer -fsanitize=address -include /home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -O1 -std=c++2a -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/bb0a4c3a76df-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/Output/nodiscard.verify.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_ENABLE_NODISCARD -fsyntax-only -Wno-error -Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0
View Full Test Results (46 Failed)

Event Timeline

ldionne created this revision.Mar 10 2020, 1:43 PM
Herald added a project: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Mar 11 2020, 8:12 PM

LGTM other than the lack of tests.

We should also figure out what GCC's plan is here, so we can implement a GCC version in future.

libcxx/include/bit
366

We should probably mark this nodiscard.

371

Gosh. __builtin_bit_cast is a weird builtin -- taking a type as the first parameter.

373

#endif // !defined(_LIBCPP_HAS_NO_BUILTIN_BIT_CAST)

libcxx/test/std/numerics/bit/bit.cast/bit_cast.fail.cpp
58 ↗(On Diff #249494)

Are these errors emitted inside <bit>? If so, we should specify that here.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
25

What's the fun stuff?

This revision now requires changes to proceed.Mar 11 2020, 8:12 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 11 2020, 8:12 PM
ldionne updated this revision to Diff 250243.Mar 13 2020, 9:54 AM
ldionne marked 5 inline comments as done.

Add tests

ldionne updated this revision to Diff 250245.Mar 13 2020, 9:58 AM
ldionne marked 3 inline comments as done and an inline comment as not done.

Specify that the errors from __builtin_bit_cast come from <bit>

ldionne added inline comments.
libcxx/include/bit
366

Marked it with _LIBCPP_NODISCARD_EXT, which we use for nodiscard-as-an-extension.

368

Question: the spec has these as Constraints: -- I think it means these should be used to SFINAE out, not hard-error -- right?

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
25

Writing the tests.

154

@erik.pilkington Any idea why those don't work inside constexpr?

158

@jfb Can you think of other interesting values to test?

197

@erik.pilkington All the tests for long double are failing (at runtime) -- any idea why? Did you implement support for long double in __builtin_bit_cast?

ldionne edited the summary of this revision. (Show Details)Mar 13 2020, 9:59 AM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
154

Its hitting the isInt() check here: https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ExprConstant.cpp#L11102. Seems like constexpr __builtin_memcmp was only implemented for integers.

197

Eek, it looks like this is asserting at compile time too. Let me take a closer look.

jfb added inline comments.Mar 13 2020, 10:52 AM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
165

Signaling NaN, and NaNs with payloads.
Infinities.
Denormals.

Check this out: https://babbage.cs.qc.cuny.edu/IEEE-754/index.xhtml

195

Some of these won't work on all platforms. You'll need to selectively disable some for platforms that don't have IEEE-754 with the expected sizes.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
197

I put a patch up to fix this here: https://reviews.llvm.org/D76323

tambre added a subscriber: tambre.Jun 20 2020, 4:19 AM
tambre added inline comments.Jun 20 2020, 4:29 AM
libcxx/include/bit
368

Correct.

From [structure.specifications] 3.1:

Constraints: the conditions for the function’s participation in overload resolution.

chfast added a subscriber: chfast.Dec 8 2020, 1:57 PM

This approach may be frowned upon because std::bit_cast is implemented with __builtin_bit_cast but, I think you could actually use __builtin_bit_cast to write the comparison function you want for the tests:

template<class T> struct Buff { char buff[sizeof(T)]; };

template<class T>
constexpr bool cmp(const T &a, const T &b) {
    auto c1 = __builtin_bit_cast(Buff<T>, a);
    auto c2 = __builtin_bit_cast(Buff<T>, b);
    for (unsigned i = 0; i < sizeof(T); ++i) {
        if (c1.buff[i] != c2.buff[i])
            return false;
    }
    return true;
}
libcxx/test/std/numerics/bit/bit.cast/bit_cast.fail.cpp
31 ↗(On Diff #250245)

Maybe a deleted copy constructor?

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
44

So, in non-constexpr, the only thing you're testing here is that you can cast a pointer to its current type?

ldionne updated this revision to Diff 316245.Jan 12 2021, 2:39 PM
ldionne marked 8 inline comments as done.

Rebase onto main and update tests + implementation.

The tests are still not passing, and I have to verify whether it's a Clang
bug or whether the tests are wrong.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.fail.cpp
31 ↗(On Diff #250245)

I think that's covered by the test for a type that isn't trivially copyable. If a type is not copyable, then it is not trivially copyable.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
44

I don't understand. In constexpr mode, the only thing being tested is that the roundtrip from T back to T (through std::bit_cast) works as expected. I'm only unable to test the intermediate results (the char arrays).

However, as of rebasing this patch, it appears that none of it works anymore. So I don't have a way to test bit_cast inside constexpr.

195

I'll see what CI complains about!

zoecarver added inline comments.Jan 12 2021, 3:19 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
44

This comment was referring to the old patch where there was an is_constant_evaluated condition in the test_buffer_roundtrip function.

208

I assume this means you're against using __builtin_bit_cast to test std::bit_cast? Fair enough.

ldionne marked an inline comment as done.Jan 13 2021, 7:59 AM
ldionne added inline comments.
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
208

Yes, because std::bit_cast is implemented as a single call to __builtin_bit_cast. I feel like what we're actually testing here is in fact __builtin_bit_cast directly, not just std::bit_cast.

curdeius added inline comments.
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
153

Nit: you should probably use long long instead of long so that this test doesn't break on LLP64 data model (Windows). You might use uint64_t as well.

curdeius added inline comments.Jan 13 2021, 2:54 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
49

Not sure if this adds much value, but can't you additionally test assert(from == middle.x);?
Similarly for middle2.

zoecarver added inline comments.Jan 13 2021, 9:37 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
208

Fair enough. Ideally, Clang's tests would be sufficient, but I see where you're coming from. I think that makes sense.

Would it make it better if the call to __builtin_bit_cast always casted it to a char array and compared the bytes? That would basically be re-implementing memcmp. If we can verify that the cast to char array always works, then we can use that to test other things, maybe.

zoecarver added inline comments.Jan 13 2021, 11:12 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
208

Here's a Godbolt. I think the issue is just when you try to compare different pointer types with __builtin_memcmp. I guess the earlier approach where you turned off certain asserts in constexpr mode actually makes sense.

https://godbolt.org/z/ozKn8W

Re [[nodiscard]]: hooray!
D99895 provides a Test Ready to Eat for the nodiscardness of bit_cast; once it's landed, you should be able to just rebase, uncomment that test, and add bit_cast to the appropriate list in UsingLibcxx.rst.