Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc] Fix typo in long double negative block #68243

Merged

Conversation

michaelrj-google
Copy link
Contributor

The long double version of float to string's get_negative_block had a
bug in table mode. In table mode, one of the tables is named
"MIN_BLOCK_2" and it stores the number of blocks that are all zeroes
before the digits start for a given index. The check for long doubles
was incorrectly "block_index <= MIN_BLOCK_2[idx]" when it should be
"block_index < MIN_BLOCK_2[idx]" (without the equal sign). This bug
caused an off-by-one error for some long double values. This patch fixes
the bug and adds tests to ensure it doesn't regress.

The long double version of float to string's get_negative_block had a
bug in table mode. In table mode, one of the tables is named
"MIN_BLOCK_2" and it stores the number of blocks that are all zeroes
before the digits start for a given index. The check for long doubles
was incorrectly "block_index <= MIN_BLOCK_2[idx]" when it should be
"block_index < MIN_BLOCK_2[idx]" (without the equal sign). This bug
caused an off-by-one error for some long double values. This patch fixes
the bug and adds tests to ensure it doesn't regress.
@llvmbot
Copy link
Member

llvmbot commented Oct 4, 2023

@llvm/pr-subscribers-libc

Changes

The long double version of float to string's get_negative_block had a
bug in table mode. In table mode, one of the tables is named
"MIN_BLOCK_2" and it stores the number of blocks that are all zeroes
before the digits start for a given index. The check for long doubles
was incorrectly "block_index <= MIN_BLOCK_2[idx]" when it should be
"block_index < MIN_BLOCK_2[idx]" (without the equal sign). This bug
caused an off-by-one error for some long double values. This patch fixes
the bug and adds tests to ensure it doesn't regress.


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

2 Files Affected:

  • (modified) libc/src/__support/float_to_string.h (+1-1)
  • (modified) libc/test/src/stdio/sprintf_test.cpp (+11)
diff --git a/libc/src/__support/float_to_string.h b/libc/src/__support/float_to_string.h
index d3fcec7a652faba..eb06cd9c08af287 100644
--- a/libc/src/__support/float_to_string.h
+++ b/libc/src/__support/float_to_string.h
@@ -708,7 +708,7 @@ FloatToString<long double>::get_negative_block(int block_index) {
     const int32_t SHIFT_CONST = TABLE_SHIFT_CONST;
 
     // if the requested block is zero
-    if (block_index <= MIN_BLOCK_2[idx]) {
+    if (block_index < MIN_BLOCK_2[idx]) {
       return 0;
     }
     const uint32_t p = POW10_OFFSET_2[idx] + block_index - MIN_BLOCK_2[idx];
diff --git a/libc/test/src/stdio/sprintf_test.cpp b/libc/test/src/stdio/sprintf_test.cpp
index 8b9c919bed203d4..63de6f94dc1ab5e 100644
--- a/libc/test/src/stdio/sprintf_test.cpp
+++ b/libc/test/src/stdio/sprintf_test.cpp
@@ -2757,6 +2757,17 @@ TEST_F(LlvmLibcSPrintfTest, FloatAutoConv) {
   written = LIBC_NAMESPACE::sprintf(buff, "%.10g", 0x1.0p-1074);
   ASSERT_STREQ_LEN(written, buff, "4.940656458e-324");
 
+  written = LIBC_NAMESPACE::sprintf(buff, "%g", 0xa.aaaaaaaaaaaaaabp-7);
+  ASSERT_STREQ_LEN(written, buff, "0.0833333");
+
+  written = LIBC_NAMESPACE::sprintf(buff, "%Lg", 0xa.aaaaaaaaaaaaaabp-7L);
+  ASSERT_STREQ_LEN(written, buff, "0.0833333");
+
+  written = LIBC_NAMESPACE::sprintf(buff, "%.60Lg", 0xa.aaaaaaaaaaaaaabp-7L);
+  ASSERT_STREQ_LEN(
+      written, buff,
+      "0.0833333333333333333355920878593448009041821933351457118988037");
+
   // Long double precision tests.
   // These are currently commented out because they require long double support
   // that isn't ready yet.

@@ -2757,6 +2757,17 @@ TEST_F(LlvmLibcSPrintfTest, FloatAutoConv) {
written = LIBC_NAMESPACE::sprintf(buff, "%.10g", 0x1.0p-1074);
ASSERT_STREQ_LEN(written, buff, "4.940656458e-324");

written = LIBC_NAMESPACE::sprintf(buff, "%g", 0xa.aaaaaaaaaaaaaabp-7);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these tests be guarded in the ifdefs below?

@michaelrj-google michaelrj-google merged commit bfcfc2a into llvm:main Oct 4, 2023
@michaelrj-google michaelrj-google deleted the libcPrintfAutoFormatLongDoubleFix branch October 4, 2023 20:45
@tnv01 tnv01 mentioned this pull request Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants