Skip to content

[libc] Fix incorrect printing for alt mode ints #70252

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
merged 1 commit into from
Oct 27, 2023

Conversation

michaelrj-google
Copy link
Contributor

Previously, our printf would incorrectly handle conversions like
("%#x",0) and ("%#o",0). This patch corrects the behavior to match what
is described in the standard.

Previously, our printf would incorrectly handle conversions like
("%#x",0) and ("%#o",0). This patch corrects the behavior to match what
is described in the standard.
@llvmbot llvmbot added the libc label Oct 25, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 25, 2023

@llvm/pr-subscribers-libc

Author: None (michaelrj-google)

Changes

Previously, our printf would incorrectly handle conversions like
("%#x",0) and ("%#o",0). This patch corrects the behavior to match what
is described in the standard.


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

2 Files Affected:

  • (modified) libc/src/stdio/printf_core/int_converter.h (+14-2)
  • (modified) libc/test/src/stdio/sprintf_test.cpp (+16)
diff --git a/libc/src/stdio/printf_core/int_converter.h b/libc/src/stdio/printf_core/int_converter.h
index 348e81a1fa536bf..13fcf3f1aa2edac 100644
--- a/libc/src/stdio/printf_core/int_converter.h
+++ b/libc/src/stdio/printf_core/int_converter.h
@@ -113,7 +113,7 @@ LIBC_INLINE int convert_int(Writer *writer, const FormatSection &to_conv) {
   size_t prefix_len;
   char prefix[2];
   if ((to_lower(to_conv.conv_name) == 'x') &&
-      ((flags & FormatFlags::ALTERNATE_FORM) != 0)) {
+      ((flags & FormatFlags::ALTERNATE_FORM) != 0) && num != 0) {
     prefix_len = 2;
     prefix[0] = '0';
     prefix[1] = a + ('x' - 'a');
@@ -158,8 +158,20 @@ LIBC_INLINE int convert_int(Writer *writer, const FormatSection &to_conv) {
                               prefix_len);
   }
 
+  // The standard says that alternate form for the o conversion "increases
+  // the precision, if and only if necessary, to force the first digit of the
+  // result to be a zero (if the value and precision are both 0, a single 0 is
+  // printed)"
+  // This if checks the following conditions:
+  // 1) is this an o conversion in alternate form?
+  // 2) does this number has a leading zero?
+  //    2a) ... because there are additional leading zeroes?
+  //    2b) ... because it is just "0", unless it will not write any digits.
+  const bool has_leading_zero =
+      (zeroes > 0) || ((num == 0) && (digits_written != 0));
   if ((to_conv.conv_name == 'o') &&
-      ((to_conv.flags & FormatFlags::ALTERNATE_FORM) != 0) && zeroes < 1) {
+      ((to_conv.flags & FormatFlags::ALTERNATE_FORM) != 0) &&
+      !has_leading_zero) {
     zeroes = 1;
     --spaces;
   }
diff --git a/libc/test/src/stdio/sprintf_test.cpp b/libc/test/src/stdio/sprintf_test.cpp
index a68ee97d6d6edc4..b2c321c0b15c9dc 100644
--- a/libc/test/src/stdio/sprintf_test.cpp
+++ b/libc/test/src/stdio/sprintf_test.cpp
@@ -329,6 +329,10 @@ TEST(LlvmLibcSPrintfTest, HexConv) {
   EXPECT_EQ(written, 5);
   ASSERT_STREQ(buff, "0xd3f");
 
+  written = LIBC_NAMESPACE::sprintf(buff, "%#x", 0);
+  EXPECT_EQ(written, 1);
+  ASSERT_STREQ(buff, "0");
+
   written = LIBC_NAMESPACE::sprintf(buff, "%#X", 0xE40);
   EXPECT_EQ(written, 5);
   ASSERT_STREQ(buff, "0XE40");
@@ -359,6 +363,10 @@ TEST(LlvmLibcSPrintfTest, HexConv) {
   EXPECT_EQ(written, 9);
   ASSERT_STREQ(buff, "  0X009D4");
 
+  written = LIBC_NAMESPACE::sprintf(buff, "%#.x", 0);
+  EXPECT_EQ(written, 0);
+  ASSERT_STREQ(buff, "");
+
   written = LIBC_NAMESPACE::sprintf(buff, "%-7.5x", 0x260);
   EXPECT_EQ(written, 7);
   ASSERT_STREQ(buff, "00260  ");
@@ -516,6 +524,10 @@ TEST(LlvmLibcSPrintfTest, OctConv) {
   EXPECT_EQ(written, 4);
   ASSERT_STREQ(buff, "0234");
 
+  written = LIBC_NAMESPACE::sprintf(buff, "%#o", 0);
+  EXPECT_EQ(written, 1);
+  ASSERT_STREQ(buff, "0");
+
   written = LIBC_NAMESPACE::sprintf(buff, "%05o", 0470);
   EXPECT_EQ(written, 5);
   ASSERT_STREQ(buff, "00470");
@@ -534,6 +546,10 @@ TEST(LlvmLibcSPrintfTest, OctConv) {
   EXPECT_EQ(written, 7);
   ASSERT_STREQ(buff, "0703   ");
 
+  written = LIBC_NAMESPACE::sprintf(buff, "%#.o", 0);
+  EXPECT_EQ(written, 1);
+  ASSERT_STREQ(buff, "0");
+
   written = LIBC_NAMESPACE::sprintf(buff, "%7.5o", 0314);
   EXPECT_EQ(written, 7);
   ASSERT_STREQ(buff, "  00314");

@michaelrj-google michaelrj-google merged commit 6e863c4 into llvm:main Oct 27, 2023
@michaelrj-google michaelrj-google deleted the libcPrintfIntAltFix branch October 27, 2023 18: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