From 69710af200a0254262c92ddfb96f8bbf4fb6636d Mon Sep 17 00:00:00 2001 From: div72 Date: Sat, 2 Mar 2024 12:14:54 +0300 Subject: [PATCH 1/2] build: enforce SSE2 on x86 targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit targets the floating point errors caused by x87 floating point calculations breaking floating point equality on the testing suite. Let's take the disassembly from `project.m_rac == 123.45` check at src/test/gridcoin/researcher_tests.cpp#L272 for example: ``` │ 0x5911c7c5 fldl -0xa4(%ebp) ... │ 0x5911c7e0 fldt -0x6fa844(%esi) ... │ 0x5911c81d fucompp │ 0x5911c81f fnstsw %ax ``` The fldl instruction loads a double to the floating point registers, while the fldt instruction loads a long double(80-bits) to registers. Combining that with the fact that since ASLR is enabled, the one with the massive offset against the stack is probably the 123.45 float literal while the first instruction is for the project.m_rac. Before the fucompp instruction(which compares two floating points), the floating point registers look like this: ``` st0 123.449999999999999997 (raw 0x4005f6e6666666666666) st1 123.450000000000002842 (raw 0x4005f6e6666666666800) ``` The x87 floating point registers seem to work like a stack, since the first fldl instruction loads the 123.450...2842 and the second fldt instruction loads the 123.449...97 value. From the raw value, it seems the first 8 bytes of the values are identical, but the last two bytes (corresponding to the extra 16 bits granted by the x87 extension, which should map to the fraction part of the float) differ slightly, which seems to cause the fucompp instruction to think that these two values are different. This is normally not a problem, as floating point equality comparisons are expected to be not stable. The problem however arises from the fact that it is the **literal 123.45** whose value is off. When a basic C program is compiled(with -m32 option or in a 32-bit environment), the loaded value for 123.45 is identical to the computed value in the st1 register; which means something is going wrong in either during runtime loading of the value or compile-time storing of the value. GDB is unable to actually read that memory address weirdly, so I'm unable to exactly pinpoint which. Because of the issue lying on the extra bits of the fp register, I assumed that the -ffloat-store(which should've ensured registers to not have more precision than a double) or the -fexcess-precision=standard(an option which should be a superset of the former) or the -mpc64(an option which rounds the significand of the results of FP ops to 53-bits) should have fixed the issue, but they didn't and I will not bother to re-examine the disassembly to figure out why. Rather than that, this commit enforces the SSE2 instructions for the FP operations, which are already used on x86_64 hosts and operate under double precision instead of long double. This should be fine compatibility wise, as SSE2 is supported on CPUs after Pentium 4 where I assume the prior CPUs don't have enough computing power to run the client in the first place. --- configure.ac | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/configure.ac b/configure.ac index 8fa47af3b0..d0b56fb0e3 100755 --- a/configure.ac +++ b/configure.ac @@ -278,6 +278,16 @@ if test "$CXXFLAGS_overridden" = "no"; then AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[CXXFLAGS="$CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]]) fi + +dnl x87 FP operations on x86 hosts can break assumptions made about the floating point values. +dnl See the commit message which introduced this change for more details. +case $host in + i?86-*|x86_64-*) + AX_CHECK_COMPILE_FLAG([-msse2],[CXXFLAGS="$CXXFLAGS -msse2 -mfpmath=sse"],[AC_MSG_ERROR([SSE2 support is required on x86 targets.])],[[$CXXFLAG_WERROR]]) + ;; + *) ;; +esac + enable_sse42=no enable_sse41=no enable_avx2=no From 6bc0c711a27b951b845581ce882e2c92cd227fbc Mon Sep 17 00:00:00 2001 From: div72 Date: Sat, 2 Mar 2024 12:15:20 +0300 Subject: [PATCH 2/2] Revert "Implement comp_double comparison function in certain tests" This reverts commit 7bfd1b4a094c4211a84b00d3aff76f7bb9c50db2. --- src/test/gridcoin/claim_tests.cpp | 34 +++---------- src/test/gridcoin/researcher_tests.cpp | 68 +++++++++----------------- 2 files changed, 31 insertions(+), 71 deletions(-) diff --git a/src/test/gridcoin/claim_tests.cpp b/src/test/gridcoin/claim_tests.cpp index 33d1e204ad..0a9b5f1af4 100644 --- a/src/test/gridcoin/claim_tests.cpp +++ b/src/test/gridcoin/claim_tests.cpp @@ -97,26 +97,6 @@ static CKey GetTestPrivateKey() return key; } - -// Unfortunately, GCC 13 on openSUSE i386 is misbehaving and exhibiting weird errors in the last decimal places for things -// even as straightforward as -// -// double foo = 0.0; -// text >> foo. -// -// This comparison function works around that by allowing a small error band to pass the tests, but not enough to invalidate -// the tests on compilers that work. -bool comp_double(double lhs, double rhs) -{ - // Require exact match if 0=0. - if (std::min(lhs, rhs) == 0.0) { - return (lhs == rhs); - } else { - double unsigned_rel_error = std::abs(lhs - rhs) / std::min(lhs, rhs); - - return (unsigned_rel_error <= double {1e-8}); - } -} } // anonymous namespace // ----------------------------------------------------------------------------- @@ -138,7 +118,7 @@ BOOST_AUTO_TEST_CASE(it_initializes_to_an_empty_claim) BOOST_CHECK(claim.m_magnitude == 0); BOOST_CHECK(claim.m_research_subsidy == 0); - BOOST_CHECK(comp_double(claim.m_magnitude_unit, 0.0)); + BOOST_CHECK(claim.m_magnitude_unit == 0.0); BOOST_CHECK(claim.m_signature.empty() == true); @@ -161,7 +141,7 @@ BOOST_AUTO_TEST_CASE(it_initializes_to_the_specified_version) BOOST_CHECK(claim.m_magnitude == 0); BOOST_CHECK(claim.m_research_subsidy == 0); - BOOST_CHECK(comp_double(claim.m_magnitude_unit, 0.0)); + BOOST_CHECK(claim.m_magnitude_unit == 0.0); BOOST_CHECK(claim.m_signature.empty() == true); @@ -240,7 +220,7 @@ BOOST_AUTO_TEST_CASE(it_parses_a_legacy_boincblock_string_for_researcher) BOOST_CHECK(claim.m_magnitude == 123); BOOST_CHECK(claim.m_research_subsidy == 47.25 * COIN); - BOOST_CHECK(comp_double(claim.m_magnitude_unit, 0.123456)); + BOOST_CHECK(claim.m_magnitude_unit == 0.123456); BOOST_CHECK(claim.m_signature == signature); @@ -523,7 +503,7 @@ BOOST_AUTO_TEST_CASE(it_deserializes_from_a_stream_for_investor) BOOST_CHECK(claim.m_research_subsidy == 0); BOOST_CHECK(claim.m_magnitude == 0.0); - BOOST_CHECK(comp_double(claim.m_magnitude_unit, 0.0)); + BOOST_CHECK(claim.m_magnitude_unit == 0.0); BOOST_CHECK(claim.m_signature.empty() == true); BOOST_CHECK(claim.m_quorum_address.empty() == true); BOOST_CHECK(claim.m_superblock->WellFormed() == false); @@ -565,7 +545,7 @@ BOOST_AUTO_TEST_CASE(it_deserializes_from_a_stream_for_investor_with_superblock) BOOST_CHECK(claim.m_research_subsidy == 0); BOOST_CHECK(claim.m_magnitude == 0.0); - BOOST_CHECK(comp_double(claim.m_magnitude_unit, 0.0)); + BOOST_CHECK(claim.m_magnitude_unit == 0.0); BOOST_CHECK(claim.m_signature.empty() == true); } @@ -650,7 +630,7 @@ BOOST_AUTO_TEST_CASE(it_deserializes_from_a_stream_for_researcher) BOOST_CHECK(claim.m_research_subsidy == expected.m_research_subsidy); BOOST_CHECK(claim.m_magnitude == 0.0); - BOOST_CHECK(comp_double(claim.m_magnitude_unit, 0.0)); + BOOST_CHECK(claim.m_magnitude_unit == 0.0); BOOST_CHECK(claim.m_signature == expected.m_signature); BOOST_CHECK(claim.m_quorum_hash == expected.m_quorum_hash); @@ -690,7 +670,7 @@ BOOST_AUTO_TEST_CASE(it_deserializes_from_a_stream_for_researcher_with_superbloc BOOST_CHECK(claim.m_research_subsidy == expected.m_research_subsidy); BOOST_CHECK(claim.m_magnitude == 0.0); - BOOST_CHECK(comp_double(claim.m_magnitude_unit, 0.0)); + BOOST_CHECK(claim.m_magnitude_unit == 0.0); BOOST_CHECK(claim.m_signature == expected.m_signature); BOOST_CHECK(claim.m_quorum_hash == expected.m_quorum_hash); diff --git a/src/test/gridcoin/researcher_tests.cpp b/src/test/gridcoin/researcher_tests.cpp index e163fb5862..352c2b464f 100644 --- a/src/test/gridcoin/researcher_tests.cpp +++ b/src/test/gridcoin/researcher_tests.cpp @@ -183,26 +183,6 @@ void AddProtocolEntry(const uint32_t& payload_version, const std::string& key, c registry.Add({contract, dummy_tx, &dummy_index}); } - -// Unfortunately, GCC 13 on openSUSE i386 is misbehaving and exhibiting weird errors in the last decimal places for things -// even as straightforward as -// -// double foo = 0.0; -// text >> foo. -// -// This comparison function works around that by allowing a small error band to pass the tests, but not enough to invalidate -// the tests on compilers that work. -bool comp_double(double lhs, double rhs) -{ - // Require exact match if 0=0. - if (std::min(lhs, rhs) == 0.0) { - return (lhs == rhs); - } else { - double unsigned_rel_error = std::abs(lhs - rhs) / std::min(lhs, rhs); - - return (unsigned_rel_error <= double {1e-8}); - } -} } // anonymous namespace // ----------------------------------------------------------------------------- @@ -225,7 +205,7 @@ BOOST_AUTO_TEST_CASE(it_initializes_with_project_data) BOOST_CHECK(project.m_cpid == expected); BOOST_CHECK(project.m_team == "team name"); BOOST_CHECK(project.m_url == "url"); - BOOST_CHECK(comp_double(project.m_rac, 0.0)); + BOOST_CHECK(project.m_rac == 0.0); BOOST_CHECK(project.m_error == GRC::MiningProject::Error::NONE); } @@ -254,7 +234,7 @@ BOOST_AUTO_TEST_CASE(it_parses_a_project_xml_string) BOOST_CHECK(project.m_cpid == cpid); BOOST_CHECK(project.m_team == "team name"); BOOST_CHECK(project.m_url == "https://example.com/"); - BOOST_CHECK(comp_double(project.m_rac, 123.45)); + BOOST_CHECK(project.m_rac == 123.45); BOOST_CHECK(project.m_error == GRC::MiningProject::Error::NONE); // Clean up: @@ -289,7 +269,7 @@ BOOST_AUTO_TEST_CASE(it_falls_back_to_compute_a_missing_external_cpid) BOOST_CHECK(project.m_cpid == cpid); BOOST_CHECK(project.m_team == "team name"); BOOST_CHECK(project.m_url == "https://example.com/"); - BOOST_CHECK(comp_double(project.m_rac, 123.45)); + BOOST_CHECK(project.m_rac == 123.45); BOOST_CHECK(project.m_error == GRC::MiningProject::Error::NONE); // Clean up: @@ -508,7 +488,7 @@ BOOST_AUTO_TEST_CASE(it_parses_a_set_of_project_xml_sections) BOOST_CHECK(project1->m_cpid == cpid_1); BOOST_CHECK(project1->m_team == "gridcoin"); BOOST_CHECK(project1->m_url == "https://example.com/1"); - BOOST_CHECK(comp_double(project1->m_rac, 123.45)); + BOOST_CHECK(project1->m_rac == 123.45); BOOST_CHECK(project1->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project1->Eligible() == true); } else { @@ -520,7 +500,7 @@ BOOST_AUTO_TEST_CASE(it_parses_a_set_of_project_xml_sections) BOOST_CHECK(project2->m_cpid == cpid_2); BOOST_CHECK(project2->m_team == "gridcoin"); BOOST_CHECK(project2->m_url == "https://example.com/2"); - BOOST_CHECK(comp_double(project2->m_rac, 567.89)); + BOOST_CHECK(project2->m_rac == 567.89); BOOST_CHECK(project2->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project2->Eligible() == true); } else { @@ -875,7 +855,7 @@ BOOST_AUTO_TEST_CASE(it_parses_project_xml_to_a_global_researcher_singleton) BOOST_CHECK(project1->m_cpid == cpid_1); BOOST_CHECK(project1->m_team == "gridcoin"); BOOST_CHECK(project1->m_url == "https://example.com/1"); - BOOST_CHECK(comp_double(project1->m_rac, 1.1)); + BOOST_CHECK(project1->m_rac == 1.1); BOOST_CHECK(project1->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project1->Eligible() == true); } else { @@ -887,7 +867,7 @@ BOOST_AUTO_TEST_CASE(it_parses_project_xml_to_a_global_researcher_singleton) BOOST_CHECK(project2->m_cpid == cpid_2); BOOST_CHECK(project2->m_team == "gridcoin"); BOOST_CHECK(project2->m_url == "https://example.com/2"); - BOOST_CHECK(comp_double(project2->m_rac, 2.2)); + BOOST_CHECK(project2->m_rac == 2.2); BOOST_CHECK(project2->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project2->Eligible() == true); } else { @@ -929,7 +909,7 @@ BOOST_AUTO_TEST_CASE(it_looks_up_loaded_boinc_projects_by_name) BOOST_CHECK(project->m_cpid == cpid); BOOST_CHECK(project->m_team == "gridcoin"); BOOST_CHECK(project->m_url == "https://example.com/"); - BOOST_CHECK(comp_double(project->m_rac, 1.1)); + BOOST_CHECK(project->m_rac == 1.1); BOOST_CHECK(project->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project->Eligible() == true); } else { @@ -1068,7 +1048,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) BOOST_CHECK(project1->m_name == "project name 1"); BOOST_CHECK(project1->m_cpid == cpid); BOOST_CHECK(project1->m_team == "not gridcoin"); - BOOST_CHECK(comp_double(project1->m_rac, 1.1)); + BOOST_CHECK(project1->m_rac == 1.1); BOOST_CHECK(project1->m_error == GRC::MiningProject::Error::INVALID_TEAM); BOOST_CHECK(project1->Eligible() == false); } else { @@ -1079,7 +1059,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) BOOST_CHECK(project2->m_name == "project name 2"); BOOST_CHECK(project2->m_cpid == cpid); BOOST_CHECK(project2->m_team.empty() == true); - BOOST_CHECK(comp_double(project2->m_rac, 2.2)); + BOOST_CHECK(project2->m_rac == 2.2); BOOST_CHECK(project2->m_error == GRC::MiningProject::Error::INVALID_TEAM); BOOST_CHECK(project2->Eligible() == false); } else { @@ -1090,7 +1070,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) BOOST_CHECK(project3->m_name == "project name 3"); BOOST_CHECK(project3->m_cpid == GRC::Cpid()); BOOST_CHECK(project3->m_team == "gridcoin"); - BOOST_CHECK(comp_double(project3->m_rac, 3.3)); + BOOST_CHECK(project3->m_rac == 3.3); BOOST_CHECK(project3->m_error == GRC::MiningProject::Error::MALFORMED_CPID); BOOST_CHECK(project3->Eligible() == false); } else { @@ -1101,7 +1081,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) BOOST_CHECK(project4->m_name == "project name 4"); BOOST_CHECK(project4->m_cpid == GRC::Cpid()); BOOST_CHECK(project4->m_team == "gridcoin"); - BOOST_CHECK(comp_double(project4->m_rac, 4.4)); + BOOST_CHECK(project4->m_rac == 4.4); BOOST_CHECK(project4->m_error == GRC::MiningProject::Error::MALFORMED_CPID); BOOST_CHECK(project4->Eligible() == false); } else { @@ -1112,7 +1092,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) BOOST_CHECK(project5->m_name == "project name 5"); BOOST_CHECK(project5->m_cpid == cpid); BOOST_CHECK(project5->m_team == "gridcoin"); - BOOST_CHECK(comp_double(project5->m_rac, 5.5)); + BOOST_CHECK(project5->m_rac == 5.5); BOOST_CHECK(project5->m_error == GRC::MiningProject::Error::MISMATCHED_CPID); BOOST_CHECK(project5->Eligible() == false); } else { @@ -1123,7 +1103,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) BOOST_CHECK(project6->m_name == "project name 6"); BOOST_CHECK(project6->m_cpid == cpid); BOOST_CHECK(project6->m_team == "gridcoin"); - BOOST_CHECK(comp_double(project6->m_rac, 6.6)); + BOOST_CHECK(project6->m_rac == 6.6); BOOST_CHECK(project6->m_error == GRC::MiningProject::Error::MISMATCHED_CPID); BOOST_CHECK(project6->Eligible() == false); } else { @@ -1132,7 +1112,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) if (const GRC::ProjectOption project7 = projects.Try("project name 7")) { BOOST_CHECK(project7->m_name == "project name 7"); - BOOST_CHECK(comp_double(project7->m_rac, 7.7)); + BOOST_CHECK(project7->m_rac == 7.7); BOOST_CHECK(project7->m_error == GRC::MiningProject::Error::POOL); BOOST_CHECK(project7->Eligible() == false); } else { @@ -1142,7 +1122,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) if (const GRC::ProjectOption project8 = projects.Try("project name 8")) { BOOST_CHECK(project8->m_name == "project name 8"); BOOST_CHECK(project8->m_cpid.IsZero() == true); - BOOST_CHECK(comp_double(project8->m_rac, 8.8)); + BOOST_CHECK(project8->m_rac == 8.8); BOOST_CHECK(project8->m_error == GRC::MiningProject::Error::POOL); BOOST_CHECK(project8->Eligible() == false); } else { @@ -1153,7 +1133,7 @@ BOOST_AUTO_TEST_CASE(it_tags_invalid_projects_with_errors_when_parsing_xml) BOOST_CHECK(project9->m_name == "project name 9"); BOOST_CHECK(project9->m_cpid == cpid); BOOST_CHECK(project9->m_team == "not gridcoin"); - BOOST_CHECK(comp_double(project9->m_rac, 0.0)); + BOOST_CHECK(project9->m_rac == 0.0); BOOST_CHECK(project9->m_error == GRC::MiningProject::Error::INVALID_TEAM); BOOST_CHECK(project9->Eligible() == false); } else { @@ -1232,7 +1212,7 @@ BOOST_AUTO_TEST_CASE(it_skips_the_team_requirement_when_set_by_protocol) BOOST_CHECK(project1->m_name == "project name 1"); BOOST_CHECK(project1->m_cpid == cpid); BOOST_CHECK(project1->m_team == "! not gridcoin !"); - BOOST_CHECK(comp_double(project1->m_rac, 1.1)); + BOOST_CHECK(project1->m_rac == 1.1); BOOST_CHECK(project1->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project1->Eligible() == true); } else { @@ -1302,7 +1282,7 @@ BOOST_AUTO_TEST_CASE(it_applies_the_team_whitelist_when_set_by_the_protocol) BOOST_CHECK(project1->m_name == "project name 1"); BOOST_CHECK(project1->m_cpid == cpid); BOOST_CHECK(project1->m_team == "! not gridcoin !"); - BOOST_CHECK(comp_double(project1->m_rac, 1.1)); + BOOST_CHECK(project1->m_rac == 1.1); BOOST_CHECK(project1->m_error == GRC::MiningProject::Error::INVALID_TEAM); BOOST_CHECK(project1->Eligible() == false); } else { @@ -1313,7 +1293,7 @@ BOOST_AUTO_TEST_CASE(it_applies_the_team_whitelist_when_set_by_the_protocol) BOOST_CHECK(project2->m_name == "project name 2"); BOOST_CHECK(project2->m_cpid == cpid); BOOST_CHECK(project2->m_team == "team 1"); - BOOST_CHECK(comp_double(project2->m_rac, 0)); + BOOST_CHECK(project2->m_rac == 0); BOOST_CHECK(project2->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project2->Eligible() == true); } else { @@ -1324,7 +1304,7 @@ BOOST_AUTO_TEST_CASE(it_applies_the_team_whitelist_when_set_by_the_protocol) BOOST_CHECK(project3->m_name == "project name 3"); BOOST_CHECK(project3->m_cpid == cpid); BOOST_CHECK(project3->m_team == "team 2"); - BOOST_CHECK(comp_double(project3->m_rac, 0)); + BOOST_CHECK(project3->m_rac == 0); BOOST_CHECK(project3->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project3->Eligible() == true); } else { @@ -1649,7 +1629,7 @@ void it_parses_project_xml_from_a_client_state_xml_file() BOOST_CHECK(project1->m_name == "valid project 1"); BOOST_CHECK(project1->m_cpid == cpid_1); BOOST_CHECK(project1->m_team == "gridcoin"); - BOOST_CHECK(comp_double(project1->m_rac, 1.1)); + BOOST_CHECK(project1->m_rac == 1.1); BOOST_CHECK(project1->m_url == "https://project1.example.com/boinc/"); BOOST_CHECK(project1->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project1->Eligible() == true); @@ -1661,7 +1641,7 @@ void it_parses_project_xml_from_a_client_state_xml_file() BOOST_CHECK(project2->m_name == "valid project 2"); BOOST_CHECK(project2->m_cpid == cpid_2); BOOST_CHECK(project2->m_team == "gridcoin"); - BOOST_CHECK(comp_double(project2->m_rac, 2.2)); + BOOST_CHECK(project2->m_rac == 2.2); BOOST_CHECK(project2->m_url == "https://project2.example.com/boinc/"); BOOST_CHECK(project2->m_error == GRC::MiningProject::Error::NONE); BOOST_CHECK(project2->Eligible() == true); @@ -1674,7 +1654,7 @@ void it_parses_project_xml_from_a_client_state_xml_file() BOOST_CHECK(project3->m_name == "invalid project 3"); BOOST_CHECK(project3->m_cpid == cpid_2); BOOST_CHECK(project3->m_team == "gridcoin"); - BOOST_CHECK(comp_double(project3->m_rac, 3.3)); + BOOST_CHECK(project3->m_rac == 3.3); BOOST_CHECK(project3->m_url == "https://project3.example.com/boinc/"); BOOST_CHECK(project3->m_error == GRC::MiningProject::Error::MISMATCHED_CPID); BOOST_CHECK(project3->Eligible() == false);