Skip to content

[libc] Fix printf long double truncation bound #70705

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

Merged

Conversation

michaelrj-google
Copy link
Contributor

The calculation for if a number being printed is truncated and should be
rounded up assumed a double for one of its constants, causing
occassional misrounding. This fixes that by making the constant based on
the mantissa width.

The calculation for if a number being printed is truncated and should be
rounded up assumed a double for one of its constants, causing
occassional misrounding. This fixes that by making the constant based on
the mantissa width.
@llvmbot llvmbot added the libc label Oct 30, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 30, 2023

@llvm/pr-subscribers-libc

Author: None (michaelrj-google)

Changes

The calculation for if a number being printed is truncated and should be
rounded up assumed a double for one of its constants, causing
occassional misrounding. This fixes that by making the constant based on
the mantissa width.


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

2 Files Affected:

  • (modified) libc/src/stdio/printf_core/float_dec_converter.h (+7-5)
  • (modified) libc/test/src/stdio/sprintf_test.cpp (+3)
diff --git a/libc/src/stdio/printf_core/float_dec_converter.h b/libc/src/stdio/printf_core/float_dec_converter.h
index 8c5ba0d241eda54..0e152a26025642d 100644
--- a/libc/src/stdio/printf_core/float_dec_converter.h
+++ b/libc/src/stdio/printf_core/float_dec_converter.h
@@ -85,11 +85,13 @@ LIBC_INLINE RoundDirection get_round_direction(int last_digit, bool truncated,
 
 template <typename T>
 LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_integral_v<T>, bool>
-zero_after_digits(int32_t base_2_exp, int32_t digits_after_point, T mantissa) {
+zero_after_digits(int32_t base_2_exp, int32_t digits_after_point, T mantissa,
+                  const int32_t mant_width) {
   const int32_t required_twos = -base_2_exp - digits_after_point - 1;
+  // Add 8 to mant width since this is a loose bound.
   const bool has_trailing_zeros =
       required_twos <= 0 ||
-      (required_twos < 60 &&
+      (required_twos < (mant_width + 8) &&
        multiple_of_power_of_2(mantissa, static_cast<uint32_t>(required_twos)));
   return has_trailing_zeros;
 }
@@ -568,7 +570,7 @@ LIBC_INLINE int convert_float_decimal_typed(Writer *writer,
         RoundDirection round;
         const bool truncated =
             !zero_after_digits(exponent - MANT_WIDTH, precision,
-                               float_bits.get_explicit_mantissa());
+                               float_bits.get_explicit_mantissa(), MANT_WIDTH);
         round = get_round_direction(last_digit, truncated, is_negative);
 
         RET_IF_RESULT_NEGATIVE(
@@ -733,7 +735,7 @@ LIBC_INLINE int convert_float_dec_exp_typed(Writer *writer,
       // Use the formula from %f.
       truncated =
           !zero_after_digits(exponent - MANT_WIDTH, precision - final_exponent,
-                             float_bits.get_explicit_mantissa());
+                             float_bits.get_explicit_mantissa(), MANT_WIDTH);
     }
   }
   round = get_round_direction(last_digit, truncated, is_negative);
@@ -979,7 +981,7 @@ LIBC_INLINE int convert_float_dec_auto_typed(Writer *writer,
       // Use the formula from %f.
       truncated =
           !zero_after_digits(exponent - MANT_WIDTH, exp_precision - base_10_exp,
-                             float_bits.get_explicit_mantissa());
+                             float_bits.get_explicit_mantissa(), MANT_WIDTH);
     }
   }
 
diff --git a/libc/test/src/stdio/sprintf_test.cpp b/libc/test/src/stdio/sprintf_test.cpp
index b2c321c0b15c9dc..a8fe8f2557c8ef9 100644
--- a/libc/test/src/stdio/sprintf_test.cpp
+++ b/libc/test/src/stdio/sprintf_test.cpp
@@ -1041,6 +1041,9 @@ TEST_F(LlvmLibcSPrintfTest, FloatDecimalConv) {
   written = LIBC_NAMESPACE::sprintf(buff, "%Lf", 1.0L);
   ASSERT_STREQ_LEN(written, buff, "1.000000");
 
+  written = LIBC_NAMESPACE::sprintf(buff, "%.Lf", -2.5L);
+  ASSERT_STREQ_LEN(written, buff, "-2");
+
 #if defined(SPECIAL_X86_LONG_DOUBLE)
 
   written = LIBC_NAMESPACE::sprintf(buff, "%Lf", 1e100L);

@michaelrj-google michaelrj-google merged commit 8ca565c into llvm:main Oct 30, 2023
@michaelrj-google michaelrj-google deleted the libcPrintfLongDoubleRounding branch October 30, 2023 21:04
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.

3 participants