[libc][test] make str_to_float_comparison_test independent of C++ headers.

[bassiounix]

This is an attempt to move away from C++ headers to be able to run the test without libcxx.
closes #129838

cc @lntue @RossComputerGuy

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Muhammad Bassiouni (bassiounix)

Changes

This is an attempt to move away from C++ headers to be able to run the test without libcxx.
closes #129838

cc @lntue @RossComputerGuy

---

Full diff: https://github.com/llvm/llvm-project/pull/133978.diff

2 Files Affected:

• (modified) libc/test/src/__support/CMakeLists.txt (+14-23)
• (modified) libc/test/src/__support/str_to_float_comparison_test.cpp (+46-48)

diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index d056969034d69..6513e534607b9 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -249,31 +249,22 @@ add_libc_test(
     libc.src.__support.memory_size
 )
 
-# FIXME: We shouldn't have regular executables created because we could be
-#        cross-compiling the tests and running through an emulator.
 if(NOT LIBC_TARGET_OS_IS_GPU)
-  add_executable(
-    libc_str_to_float_comparison_test
-    str_to_float_comparison_test.cpp
-  )
-
-  target_link_libraries(libc_str_to_float_comparison_test
-    PRIVATE
-      "${LIBC_TARGET}"
-  )
-
-  add_executable(
-    libc_system_str_to_float_comparison_test
-    str_to_float_comparison_test.cpp
+  add_libc_test(
+    str_to_float_comparison_test
+    SUITE
+      libc-support-tests
+    SRCS
+      str_to_float_comparison_test.cpp
+    DEPENDS
+      libc.src.stdio.printf
+      libc.src.stdio.fopen
+      libc.src.stdio.fclose
+      libc.src.stdio.fgets
+      libc.src.stdlib.strtof
+      libc.src.stdlib.strtod
+    NO_RUN_POSTBUILD
   )
-
-  set(float_test_file ${CMAKE_CURRENT_SOURCE_DIR}/str_to_float_comparison_data.txt)
-
-  add_custom_command(TARGET libc_str_to_float_comparison_test
-                     POST_BUILD
-                     COMMAND ${CMAKE_CROSSCOMPILING_EMULATOR} $<TARGET_FILE:libc_str_to_float_comparison_test> ${float_test_file}
-                     COMMENT "Test the strtof and strtod implementations against precomputed results."
-                     VERBATIM)
 endif()
 
 add_subdirectory(CPP)
diff --git a/libc/test/src/__support/str_to_float_comparison_test.cpp b/libc/test/src/__support/str_to_float_comparison_test.cpp
index 61bfc3cd0903a..5f3100d3b5946 100644
--- a/libc/test/src/__support/str_to_float_comparison_test.cpp
+++ b/libc/test/src/__support/str_to_float_comparison_test.cpp
@@ -6,16 +6,14 @@
 //
 //===----------------------------------------------------------------------===//
 
-// #include "src/__support/str_float_conv_utils.h"
-
-#include <stdlib.h> // For string to float functions
-
-// #include "src/__support/FPUtil/FPBits.h"
-
-#include <cstdint>
-#include <fstream>
-#include <iostream>
-#include <string>
+#include "src/stdio/fclose.h"
+#include "src/stdio/fgets.h"
+#include "src/stdio/fopen.h"
+#include "src/stdio/printf.h"
+#include "src/stdlib/strtod.h"
+#include "src/stdlib/strtof.h"
+#include "test/UnitTest/Test.h"
+#include <stdint.h>
 
 // The intent of this test is to read in files in the format used in this test
 // dataset: https://github.com/nigeltao/parse-number-fxx-test-data
@@ -59,16 +57,18 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
   int32_t curFails = 0;    // Only counts actual failures, not bitdiffs.
   int32_t curBitDiffs = 0; // A bitdiff is when the expected result and actual
                            // result are off by +/- 1 bit.
-  std::string line;
-  std::string num;
+  char *line = nullptr;
+  char *num = nullptr;
 
-  std::ifstream fileStream(inputFileName, std::ifstream::in);
+  auto *fileHandle = LIBC_NAMESPACE::fopen(inputFileName, "r");
 
-  if (!fileStream.is_open()) {
-    std::cout << "file '" << inputFileName << "' failed to open. Exiting.\n";
+  if (!fileHandle) {
+    LIBC_NAMESPACE::printf("file '%s' failed to open. Exiting.\n",
+                           inputFileName);
     return 1;
   }
-  while (getline(fileStream, line)) {
+
+  while (LIBC_NAMESPACE::fgets(line, 100, fileHandle)) {
     if (line[0] == '#') {
       continue;
     }
@@ -76,13 +76,13 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
     uint32_t expectedFloatRaw;
     uint64_t expectedDoubleRaw;
 
-    expectedFloatRaw = fastHexToU32(line.c_str() + 5);
-    expectedDoubleRaw = fastHexToU64(line.c_str() + 14);
-    num = line.substr(31);
+    expectedFloatRaw = fastHexToU32(line + 5);
+    expectedDoubleRaw = fastHexToU64(line + 14);
+    num = line + 31;
 
-    float floatResult = strtof(num.c_str(), nullptr);
+    float floatResult = LIBC_NAMESPACE::strtof(num, nullptr);
 
-    double doubleResult = strtod(num.c_str(), nullptr);
+    double doubleResult = LIBC_NAMESPACE::strtod(num, nullptr);
 
     uint32_t floatRaw = *(uint32_t *)(&floatResult);
 
@@ -101,9 +101,8 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
         curFails++;
       }
       if (curFails + curBitDiffs < 10) {
-        std::cout << "Float fail for '" << num << "'. Expected " << std::hex
-                  << expectedFloatRaw << " but got " << floatRaw << "\n"
-                  << std::dec;
+        LIBC_NAMESPACE::printf("Float fail for '%s'. Expected %x but got %x\n",
+                               num, expectedFloatRaw, floatRaw);
       }
     }
 
@@ -120,14 +119,14 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
         curFails++;
       }
       if (curFails + curBitDiffs < 10) {
-        std::cout << "Double fail for '" << num << "'. Expected " << std::hex
-                  << expectedDoubleRaw << " but got " << doubleRaw << "\n"
-                  << std::dec;
+        LIBC_NAMESPACE::printf(
+            "Double fail for '%s'. Expected %lx but got %lx\n", num,
+            expectedDoubleRaw, doubleRaw);
       }
     }
   }
 
-  fileStream.close();
+  LIBC_NAMESPACE::fclose(fileHandle);
 
   *totalBitDiffs += curBitDiffs;
   *totalFails += curFails;
@@ -138,7 +137,7 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
   return 0;
 }
 
-int main(int argc, char *argv[]) {
+TEST(LlvmLibcStrToFloatComparisonTest, CheckFile) {
   int result = 0;
   int fails = 0;
 
@@ -150,24 +149,23 @@ int main(int argc, char *argv[]) {
   int detailedBitDiffs[4] = {0, 0, 0, 0};
 
   int total = 0;
-  for (int i = 1; i < argc; i++) {
-    std::cout << "Starting file " << argv[i] << "\n";
-    int curResult =
-        checkFile(argv[i], &fails, &bitdiffs, detailedBitDiffs, &total);
-    if (curResult == 1) {
-      result = 1;
-      break;
-    } else if (curResult == 2) {
-      result = 2;
-    }
+
+  char filename[] = "str_to_float_comparison_data.txt";
+  LIBC_NAMESPACE::printf("Starting file %s\n", filename);
+  int curResult =
+      checkFile(filename, &fails, &bitdiffs, detailedBitDiffs, &total);
+  if (curResult == 1) {
+    result = 1;
+  } else if (curResult == 2) {
+    result = 2;
   }
-  std::cout << "Results:\n"
-            << "Total significant failed conversions: " << fails << "\n"
-            << "Total conversions off by +/- 1 bit: " << bitdiffs << "\n"
-            << "\t" << detailedBitDiffs[0] << "\tfloat low\n"
-            << "\t" << detailedBitDiffs[1] << "\tfloat high\n"
-            << "\t" << detailedBitDiffs[2] << "\tdouble low\n"
-            << "\t" << detailedBitDiffs[3] << "\tdouble high\n"
-            << "Total lines: " << total << "\n";
-  return result;
+
+  EXPECT_EQ(result, 0);
+  EXPECT_EQ(fails, 0);
+  EXPECT_EQ(bitdiffs, 0);
+  EXPECT_EQ(detailedBitDiffs[0], 0);
+  EXPECT_EQ(detailedBitDiffs[1], 0);
+  EXPECT_EQ(detailedBitDiffs[2], 0);
+  EXPECT_EQ(detailedBitDiffs[3], 0);
+  EXPECT_EQ(total, 0);
 }

[michaelrj-google]

Overall LGTM with some minor comments. Thanks for cleaning up this old test!

[michaelrj-google]

Overall LGTM, but wait for lntue to approve for the cmake.

[mikhailramalho]

can you make it constexpr?

[bassiounix]

No not really

libc/test/src/__support/str_to_float_comparison_test.cpp:120:31: warning: ISO C++11 does not allow conversion from string literal to 'char *const' [-Wwritable-strings]
  120 |   constexpr char *lines[6] = {"3C00 3F800000 3FF0000000000000 1",

[mikhailramalho]

Try constexpr const char*

[mikhailramalho]

Why do we need a const_cast? Can't you pass the const char to parseLine?

[bassiounix]

Cannot initialize a variable of type 'char *' with an rvalue of type 'const char *'. clang(init_conversion_failed)

[mikhailramalho]

Have you tried to change the parameter of parseLine to take a const char*?

[mikhailramalho]

you can get rid of this since the data will be available on the test now.

[mikhailramalho]

you can get rid of this since the data will be available on the test now.

[lntue]

@mikhailramalho @RossComputerGuy I think the purpose of this test according to @michaelrj-google is to manually run it against large files containing many interesting cases. Before this PR, the test build was set up manually, and didn't respect the dependency or entrypoint lists that we normally have. With this change, this test will be build using our normal testing infra as other tests, respecting the dependency (skipped if they are not available), and will not automatically run with ninja check-libc. I hope that would be enough to make it non-blocking when we try to build on new targets?

[mikhailramalho]

Got it, but if the goal is to run it manually on large files, can we remove the file from the repo and keep the static data for the automated test?

If so, you can ignore my comments to remove the file handling stuff @bassiounix

[lntue]

removing the file sounds good to me, but we should add more comments about the usage of this test, and maybe add the link to the other repo with a lot of tests for this one. I'll let @michaelrj-google decide what to do with the test file(s).