Skip to content

Commit

Permalink
Fix test for double truncation
Browse files Browse the repository at this point in the history
Summary:
Work around Clang 13 treating the truncation fast path as UB and
optimising it to not work on compile time constants.

This appears to be a bug in clang, since adding the
`-fno-strict-float-cast-overflow` flag does not fix it. Nonetheless,
this diff also adds that flag, since it is generally desirable for
Hermes code.

Reviewed By: tmikov

Differential Revision: D31939409

fbshipit-source-id: b86830d129e6bf1b872e502294ab9ea1893ce242
  • Loading branch information
neildhar authored and facebook-github-bot committed Oct 27, 2021
1 parent f60423a commit 01f70aa
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 18 deletions.
3 changes: 3 additions & 0 deletions cmake/modules/Hermes.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,9 @@ if (GCC_COMPATIBLE)
# Enable -Wdelete-non-virtual-dtor if available.
add_flag_if_supported("-Wdelete-non-virtual-dtor" DELETE_NON_VIRTUAL_DTOR_FLAG)

# Avoid triggering arbitrary UB when converting doubles to ints.
add_flag_if_supported("-fno-strict-float-cast-overflow" NO_STRICT_FLOAT_CAST_OVERFLOW_FLAG)

# Disable range loop analysis warnings.
check_cxx_compiler_flag("-Wrange-loop-analysis" RANGE_ANALYSIS_FLAG)
append_if(RANGE_ANALYSIS_FLAG "-Wno-range-loop-analysis" CMAKE_CXX_FLAGS)
Expand Down
41 changes: 23 additions & 18 deletions unittests/Support/ConversionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,30 @@ using namespace hermes;

namespace {

// Work around new versions of clang treating the truncation fast path as UB
// and optimising it to not work on compile time constants.
#ifdef __clang__
LLVM_ATTRIBUTE_NOINLINE
#endif
int32_t toInt32Wrapper(double d) {
return hermes::truncateToInt32(d);
}

TEST(ConversionsTest, toInt32Test) {
EXPECT_EQ(0, hermes::truncateToInt32(0));
EXPECT_EQ(0, hermes::truncateToInt32(-0.1));
EXPECT_EQ(0, hermes::truncateToInt32(0.1));
EXPECT_EQ(1, hermes::truncateToInt32(1));
EXPECT_EQ(-1, hermes::truncateToInt32(-1.5));

EXPECT_EQ(1661992960, hermes::truncateToInt32(1e20));
EXPECT_EQ(-1661992960, hermes::truncateToInt32(-1e20));
EXPECT_EQ(-2147483648, hermes::truncateToInt32(-2147483648));

EXPECT_EQ(
0, hermes::truncateToInt32(std::numeric_limits<double>::infinity()));
EXPECT_EQ(
0, hermes::truncateToInt32(-std::numeric_limits<double>::infinity()));
EXPECT_EQ(
0, hermes::truncateToInt32(-std::numeric_limits<double>::quiet_NaN()));
EXPECT_EQ(
0, hermes::truncateToInt32(-std::numeric_limits<double>::denorm_min()));
EXPECT_EQ(0, toInt32Wrapper(0));
EXPECT_EQ(0, toInt32Wrapper(-0.1));
EXPECT_EQ(0, toInt32Wrapper(0.1));
EXPECT_EQ(1, toInt32Wrapper(1));
EXPECT_EQ(-1, toInt32Wrapper(-1.5));

EXPECT_EQ(1661992960, toInt32Wrapper(1e20));
EXPECT_EQ(-1661992960, toInt32Wrapper(-1e20));
EXPECT_EQ(-2147483648, toInt32Wrapper(-2147483648));

EXPECT_EQ(0, toInt32Wrapper(std::numeric_limits<double>::infinity()));
EXPECT_EQ(0, toInt32Wrapper(-std::numeric_limits<double>::infinity()));
EXPECT_EQ(0, toInt32Wrapper(-std::numeric_limits<double>::quiet_NaN()));
EXPECT_EQ(0, toInt32Wrapper(-std::numeric_limits<double>::denorm_min()));
}

TEST(ConversionsTest, toArrayIndexTest) {
Expand Down

0 comments on commit 01f70aa

Please sign in to comment.