[libc] Fix printf %a padding issue

[michaelrj]

Summary

The trailing zeroes were previously not counted when calculating the
padding, which caused a high-precision number to get too much padding.

[sivachandra]

libc/test/src/stdio/sprintf_test.cpp

872

This comment talking about what was previously happening, and that it was found via fuzzing doesn't help build the context. Instead, can you describe what exact expected behavior is being tested here?

[michaelrj]

rewrite comment to clarify the bug being fixed

libc/test/src/stdio/sprintf_test.cpp

872

The fuzzing thing is a header, telling you what category this is. If you look earlier in the test there's a couple different categories of test, such as precision or width, that focus on a specific feature. Saying these are found by fuzzing clarifies that these aren't from a specific category, they're just random things found to have problems.

[sivachandra]

libc/test/src/stdio/sprintf_test.cpp

872

There is nothing wrong if you want to say that it was found by fuzzing. But, if you do say it, you should ideally provide a reference. If this patch was a fix to a user bug, you will likely put a link to the bug report to help build context for the reader. An year later, the comment "Found via fuzzing" means almost nothing without a reference.

That said, my comment was more about describing what is being tested here rather than describing what was happening previously. Once that is fixed, the note that the bug was found via fuzzing is up to you.

880

Another nit: Add a comment here saying that the output string has a single character padding to make it 58 characters long.

[sivachandra]

libc/test/src/stdio/sprintf_test.cpp

872

Just read the other patch and it seems like you want to treat "found by fuzzing" as a category of tests. I would instead call them "Padding tests" or something like that in this case and view that as a category.

[michaelrj]

adjust test comments

libc/test/src/stdio/sprintf_test.cpp

872

the category these would fall under is "combined tests" since these aren't strictly padding tests or strictly precision tests, both of which are listed above on lines 608 and 631 respectively. The combined tests category is at the end, the section these are already in. I've removed the note that these were found via fuzzing to avoid confusion.

libc/src/stdio/printf_core/float_hex_converter.h

bool has_hexadecimal_point =
                             FormatFlags::ALTERNATE_FORM);
   constexpr cpp::string_view HEXADECIMAL_POINT(".");
 
   // This is for the letter 'p' before the exponent.
   const char exp_seperator = a + ('p' - 'a');
   constexpr int EXP_SEPERATOR_LEN = 1;
 
   padding = static_cast<int>(to_conv.min_width - (sign_char > 0 ? 1 : 0) -
-                             PREFIX_LEN - mant_digits -
+                             PREFIX_LEN - mant_digits - trailing_zeroes -
                              static_cast<int>(has_hexadecimal_point) -
                              EXP_SEPERATOR_LEN - (EXP_LEN - exp_cur));
   if (padding < 0)
     padding = 0;
 
   if ((to_conv.flags & FormatFlags::LEFT_JUSTIFIED) ==
       FormatFlags::LEFT_JUSTIFIED) {
     // The pattern is (sign), 0x, digit, (.), (other digits), (zeroes), p,

libc/test/src/stdio/sprintf_test.cpp

#endif
 
   // Combined Tests.
 
   written = __llvm_libc::sprintf(buff, "%12.3a %-12.3A", 0.1, 256.0);
   ASSERT_STREQ_LEN(written, buff, "  0x1.99ap-4 0X1.000P+8  ");
 
   written = __llvm_libc::sprintf(buff, "%+-#12.3a % 012.3a", 0.1256, 1256.0);
   ASSERT_STREQ_LEN(written, buff, "+0x1.014p-3   0x1.3a0p+10");
+
+  // These tests check that the padding is properly calculated based on the
+  // min_width field. Specifically, they check that the extra zeroes added by
+  // the high precision are accounted for correctly.
+  written = __llvm_libc::sprintf(buff, "%50.50a", 0x1.0p0);
+  ASSERT_STREQ_LEN(written, buff,
+                   "0x1.00000000000000000000000000000000000000000000000000p+0");
+
+  // The difference with this test is that the formatted number is exactly 57
+  // characters, so padding to 58 adds a space.
+  written = __llvm_libc::sprintf(buff, "%58.50a", 0x1.0p0);
+  ASSERT_STREQ_LEN(
+      written, buff,
+      " 0x1.00000000000000000000000000000000000000000000000000p+0");
 }
 
 TEST_F(LlvmLibcSPrintfTest, FloatDecimalConv) {
   ForceRoundingMode r(RoundingMode::Nearest);
   double inf = __llvm_libc::fputil::FPBits<double>::inf().get_val();
   double nan = __llvm_libc::fputil::FPBits<double>::build_nan(1);
 
   written = __llvm_libc::sprintf(buff, "%f", 1.0);