[libc] Properly fix printf long double subnormals

[michaelrj-google]

In a previous PR I fixed one case where subnormal long doubles would
cause an infinite loop in printf. It was an improper fix though. The
problem was that a shift on the fixed point representation would
sometimes go negative, since the effective exponent of a subnormal is
lower than the minimum allowed exponent value. This patch extends the
fixed point representation to have space for subnormals, and adds an
assert to check that lshifts are always positive. The previous fix of
sometimes shifting right instead of left caused a loss of precision
which also sometimes caused infinite loops in the %e code.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Michael Jones (michaelrj-google)

Changes

In a previous PR I fixed one case where subnormal long doubles would
cause an infinite loop in printf. It was an improper fix though. The
problem was that a shift on the fixed point representation would
sometimes go negative, since the effective exponent of a subnormal is
lower than the minimum allowed exponent value. This patch extends the
fixed point representation to have space for subnormals, and adds an
assert to check that lshifts are always positive. The previous fix of
sometimes shifting right instead of left caused a loss of precision
which also sometimes caused infinite loops in the %e code.

---

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

2 Files Affected:

• (modified) libc/src/__support/float_to_string.h (+10-9)
• (modified) libc/test/src/stdio/sprintf_test.cpp (+10)

diff --git a/libc/src/__support/float_to_string.h b/libc/src/__support/float_to_string.h
index 9115ed2856881..44853059a8da3 100644
--- a/libc/src/__support/float_to_string.h
+++ b/libc/src/__support/float_to_string.h
@@ -618,16 +618,18 @@ template <> class FloatToString<long double> {
   fputil::FPBits<long double> float_bits;
   bool is_negative = 0;
   int exponent = 0;
-  FPBits::StorageType mantissa = 0;
+  fputil::FPBits<long double>::StorageType mantissa = 0;
 
   static constexpr int FRACTION_LEN = fputil::FPBits<long double>::FRACTION_LEN;
   static constexpr int EXP_BIAS = fputil::FPBits<long double>::EXP_BIAS;
   static constexpr size_t UINT_WORD_SIZE = 64;
 
   static constexpr size_t FLOAT_AS_INT_WIDTH =
-      internal::div_ceil(fputil::FPBits<long double>::MAX_BIASED_EXPONENT -
-                             FPBits::EXP_BIAS,
-                         UINT_WORD_SIZE) *
+      internal::div_ceil(
+          fputil::FPBits<long double>::MAX_BIASED_EXPONENT -
+              fputil::FPBits<long double>::EXP_BIAS +
+              FRACTION_LEN, // Add fraction len to provide space for subnormals.
+          UINT_WORD_SIZE) *
       UINT_WORD_SIZE;
   static constexpr size_t EXTRA_INT_WIDTH =
       internal::div_ceil(sizeof(long double) * CHAR_BIT, UINT_WORD_SIZE) *
@@ -699,12 +701,11 @@ template <> class FloatToString<long double> {
       float_as_fixed = mantissa;
 
       const int SHIFT_AMOUNT = FLOAT_AS_INT_WIDTH + exponent;
+      // if the shift amount would be negative, then the shift would cause a
+      // loss of precision.
+      LIBC_ASSERT(SHIFT_AMOUNT >= 0);
       static_assert(EXTRA_INT_WIDTH >= sizeof(long double) * 8);
-      if (SHIFT_AMOUNT > 0) {
-        float_as_fixed <<= SHIFT_AMOUNT;
-      } else {
-        float_as_fixed >>= -SHIFT_AMOUNT;
-      }
+      float_as_fixed <<= SHIFT_AMOUNT;
 
       // If there are still digits above the decimal point, handle those.
       if (cpp::countl_zero(float_as_fixed) <
diff --git a/libc/test/src/stdio/sprintf_test.cpp b/libc/test/src/stdio/sprintf_test.cpp
index 42fdd59cf4d9c..b0dd4bb4634f9 100644
--- a/libc/test/src/stdio/sprintf_test.cpp
+++ b/libc/test/src/stdio/sprintf_test.cpp
@@ -2484,6 +2484,16 @@ TEST(LlvmLibcSPrintfTest, FloatExponentLongDoubleConv) {
 
   written = LIBC_NAMESPACE::sprintf(buff, "%Le", 1.2345678e4900L);
   ASSERT_STREQ_LEN(written, buff, "1.234568e+4900");
+
+#ifndef LIBC_COPT_FLOAT_TO_STR_REDUCED_PRECISION
+  // Minimum normal + epsilon
+  written = LIBC_NAMESPACE::sprintf(buff, "%.20Le", 3.36210314311209350663E-4932L);
+  ASSERT_STREQ_LEN(written, buff, "3.36210314311209350663e-4932");
+
+  // Minimum subnormal
+  written = LIBC_NAMESPACE::sprintf(buff, "%.20Le", 3.64519953188247460253E-4951L);
+  ASSERT_STREQ_LEN(written, buff, "3.64519953188247460253e-4951");
+#endif // LIBC_COPT_FLOAT_TO_STR_REDUCED_PRECISION
 #endif
 }
 

[github-actions]

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

[vonosmas]

LGTM after fixing clang-format