From 223e11a52ea11638d9e70b14d8a69c9d35265dc5 Mon Sep 17 00:00:00 2001 From: Harold Pratt Date: Mon, 18 Jul 2022 15:59:11 -0700 Subject: [PATCH 1/2] Rewrite CompactValue to avoid undefined C++ behavior. C++ does not, pedantically, allow the use of unions for type-punning in the way that C does. Most compilers, in practice, do support it; however, recent versions of MSVC appear to have a bug that cause bad code to be generated due to this U.B. (see: https://developercommunity.visualstudio.com/t/Bad-code-generated-for-std::isnan-compil/10082631). This led to a series of issues in the react-native-windows project, see: * https://github.com/microsoft/react-native-windows/issues/4122 * https://github.com/microsoft/react-native-windows/issues/8675 In C++20, the `` header and `bit_cast` function provide a pleasant API for type-punning. Since C++20 is not universally available, if the feature-test macro for `bit_cast` is not defined, memcpy is used instead. --- yoga/CompactValue.h | 90 ++++++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 37 deletions(-) diff --git a/yoga/CompactValue.h b/yoga/CompactValue.h index 1f03cf72..510e993a 100644 --- a/yoga/CompactValue.h +++ b/yoga/CompactValue.h @@ -9,6 +9,9 @@ #ifdef __cplusplus +#ifdef __cpp_lib_bit_cast +#include +#endif #include "YGValue.h" #include "YGMacros.h" #include @@ -54,31 +57,29 @@ public: static CompactValue of(float value) noexcept { if (value == 0.0f || (value < LOWER_BOUND && value > -LOWER_BOUND)) { constexpr auto zero = - Unit == YGUnitPercent ? ZERO_BITS_PERCENT : ZERO_BITS_POINT; - return {Payload{zero}}; + Unit == YGUnitPercent ? ZERO_BITS_PERCENT : ZERO_BITS_POINT; + return {zero}; } - constexpr auto upperBound = - Unit == YGUnitPercent ? UPPER_BOUND_PERCENT : UPPER_BOUND_POINT; + constexpr auto upperBound = Unit == YGUnitPercent ? UPPER_BOUND_PERCENT : UPPER_BOUND_POINT; if (value > upperBound || value < -upperBound) { value = copysignf(upperBound, value); } uint32_t unitBit = Unit == YGUnitPercent ? PERCENT_BIT : 0; - auto data = Payload{value}; - data.repr -= BIAS; - data.repr |= unitBit; + auto data = asU32(value); + data -= BIAS; + data |= unitBit; return {data}; } template static CompactValue ofMaybe(float value) noexcept { - return std::isnan(value) || std::isinf(value) ? ofUndefined() - : of(value); + return std::isnan(value) || std::isinf(value) ? ofUndefined() : of(value); } static constexpr CompactValue ofZero() noexcept { - return CompactValue{Payload{ZERO_BITS_POINT}}; + return CompactValue{ZERO_BITS_POINT}; } static constexpr CompactValue ofUndefined() noexcept { @@ -86,13 +87,12 @@ public: } static constexpr CompactValue ofAuto() noexcept { - return CompactValue{Payload{AUTO_BITS}}; + return CompactValue{AUTO_BITS}; } - constexpr CompactValue() noexcept - : payload_(std::numeric_limits::quiet_NaN()) {} + constexpr CompactValue() noexcept : repr_(0x7FC00000) {} - CompactValue(const YGValue& x) noexcept : payload_(uint32_t{0}) { + CompactValue(const YGValue &x) noexcept : repr_(uint32_t{0}) { switch (x.unit) { case YGUnitUndefined: *this = ofUndefined(); @@ -110,7 +110,7 @@ public: } operator YGValue() const noexcept { - switch (payload_.repr) { + switch (repr_) { case AUTO_BITS: return YGValueAuto; case ZERO_BITS_POINT: @@ -119,35 +119,28 @@ public: return YGValue{0.0f, YGUnitPercent}; } - if (std::isnan(payload_.value)) { + if (std::isnan(asFloat(repr_))) { return YGValueUndefined; } - auto data = payload_; - data.repr &= ~PERCENT_BIT; - data.repr += BIAS; + auto data = repr_; + data &= ~PERCENT_BIT; + data += BIAS; - return YGValue{ - data.value, payload_.repr & 0x40000000 ? YGUnitPercent : YGUnitPoint}; + return YGValue{asFloat(data), repr_ & 0x40000000 ? YGUnitPercent : YGUnitPoint}; } bool isUndefined() const noexcept { - return ( - payload_.repr != AUTO_BITS && payload_.repr != ZERO_BITS_POINT && - payload_.repr != ZERO_BITS_PERCENT && std::isnan(payload_.value)); + return (repr_ != AUTO_BITS && repr_ != ZERO_BITS_POINT && repr_ != ZERO_BITS_PERCENT && std::isnan(asFloat(repr_))); } - bool isAuto() const noexcept { return payload_.repr == AUTO_BITS; } + bool isAuto() const noexcept { + return repr_ == AUTO_BITS; + } private: - union Payload { - float value; - uint32_t repr; - Payload() = delete; - constexpr Payload(uint32_t r) : repr(r) {} - constexpr Payload(float v) : value(v) {} - }; - + uint32_t repr_; + static constexpr uint32_t BIAS = 0x20000000; static constexpr uint32_t PERCENT_BIT = 0x40000000; @@ -157,13 +150,36 @@ private: static constexpr uint32_t ZERO_BITS_POINT = 0x7f8f0f0f; static constexpr uint32_t ZERO_BITS_PERCENT = 0x7f80f0f0; - constexpr CompactValue(Payload data) noexcept : payload_(data) {} + constexpr CompactValue(uint32_t data) noexcept : repr_(data) {} - Payload payload_; + VISIBLE_FOR_TESTING uint32_t repr() { + return repr_; + } - VISIBLE_FOR_TESTING uint32_t repr() { return payload_.repr; } + static uint32_t asU32(float f) { +#ifdef __cpp_lib_bit_cast + return std::bit_cast(f); +#else + uint32_t u; + static_assert(sizeof(u) == sizeof(f), "uint32_t and float must have the same size"); + std::memcpy(&u, &f, sizeof(f)); + return u; +#endif + } + + static float asFloat(uint32_t u) { +#ifdef __cpp_lib_bit_cast + return std::bit_cast(data); +#else + float f; + static_assert(sizeof(f) == sizeof(u), "uint32_t and float must have the same size"); + std::memcpy(&f, &u, sizeof(u)); + return f; +#endif + } }; + template <> CompactValue CompactValue::of(float) noexcept = delete; template <> @@ -174,7 +190,7 @@ template <> CompactValue CompactValue::ofMaybe(float) noexcept = delete; constexpr bool operator==(CompactValue a, CompactValue b) noexcept { - return a.payload_.repr == b.payload_.repr; + return a.repr_ == b.repr_; } constexpr bool operator!=(CompactValue a, CompactValue b) noexcept { -- 2.50.1.windows.1 From 68b780fa37fd7b18f1442cc021f3e22679677c3b Mon Sep 17 00:00:00 2001 From: "Harold Pratt (HAL)" Date: Mon, 25 Jul 2022 10:34:20 -0700 Subject: [PATCH 2/2] Two small fixes: 1. memcpy is declared in ; make sure to include it if needed 2. asFloat's __cpp_lib_bit_cast was using the wrong name for its parameter (the param was renamed after testing and I never rebuilt) with `__cpp_lib_bit_cast` on. Oops :-( --- yoga/CompactValue.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/yoga/CompactValue.h b/yoga/CompactValue.h index 510e993a..1ed0a74b 100644 --- a/yoga/CompactValue.h +++ b/yoga/CompactValue.h @@ -11,6 +11,8 @@ #ifdef __cpp_lib_bit_cast #include +#else +#include #endif #include "YGValue.h" #include "YGMacros.h" @@ -169,7 +171,7 @@ private: static float asFloat(uint32_t u) { #ifdef __cpp_lib_bit_cast - return std::bit_cast(data); + return std::bit_cast(u); #else float f; static_assert(sizeof(f) == sizeof(u), "uint32_t and float must have the same size"); -- 2.50.1.windows.1