[libc] add printf oct conversion

[michaelrj]

Summary

The oct converter handles the %o conversion.

[lntue]

libc/src/stdio/printf_core/oct_converter.h

27

static is not needed here.

38

Optional: technically you can use buffer[--buff_cur] = ... and remove the --buff_cur in the line above. But I'm not sure if it makes this less readable.

91

Look like you don't need to reset spaces and zeros to 0 when they are negatives here and above, since you already check to make sure they are positive before using in the if-else's below.

[michaelrj]

address comments

libc/src/stdio/printf_core/oct_converter.h

38

In this case, I'd say keeping them separate makes it more clear that we're writing to buff_cur - 1 and not bufcur, so I'm going to leave it.

91

spaces doesn't need to be reset, but zeroes does when it's being used in later calculations (see lines 73-76).

[sivachandra]

libc/src/stdio/printf_core/oct_converter.h

33–34

Nitty nit: ISTM like this sentence is written in the oppositte way. You probably want:

Since the buffer is size to sized to be able fit the entire number, buf_cur can never reach 0.
So, we do not need bounds checking on buf_cur.

40

Nit: The name digits_written can be confusing in the presence of a writer. You probably want to name it digits_in_val, or just num_digits.

42

Nit: Please follow the convention of starting comments with upper case first letters. Here and elsewhere.

93

Are writes guaranteed to succeed?

[michaelrj]

address comments and rebase. I will commit this once the presubmit checks clear.

libc/src/stdio/printf_core/CMakeLists.txt

HDRS
     converter.h
     converter_atlas.h
     converter_utils.h
     string_converter.h
     char_converter.h
     int_converter.h
     hex_converter.h
     ptr_converter.h
+    oct_converter.h
   DEPENDS
     .writer
     .core_structs
 )
 
 
 add_object_library(
   printf_main

libc/src/stdio/printf_core/converter.cpp

case 'c':
     return convert_char(writer, to_conv);
   case 's':
     return convert_string(writer, to_conv);
   case 'd':
   case 'i':
   case 'u':
     return convert_int(writer, to_conv);
   case 'o':
-    // return convert_oct(writer, to_conv);
+    return convert_oct(writer, to_conv);
   case 'x':
   case 'X':
     return convert_hex(writer, to_conv);
   // TODO(michaelrj): add a flag to disable float point values here
   case 'f':
   case 'F':
     // return convert_float_decimal(writer, to_conv);
   case 'e':

libc/src/stdio/printf_core/converter_atlas.h

 
 // defines convert_char
 #include "src/stdio/printf_core/char_converter.h"
 
 // defines convert_int
 #include "src/stdio/printf_core/int_converter.h"
 
 // defines convert_oct
+#include "src/stdio/printf_core/oct_converter.h"
 // defines convert_hex
 #include "src/stdio/printf_core/hex_converter.h"
 
 // TODO(michaelrj): add a flag to disable float point values here
 // defines convert_float_decimal
 // defines convert_float_dec_exp
 // defines convert_float_hex_exp
 // defines convert_float_mixed
 
 // TODO(michaelrj): add a flag to disable writing an int here
 // defines convert_write_int
 
 // defines convert_pointer
 #include "src/stdio/printf_core/ptr_converter.h"
 
 #endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_CONVERTER_ATLAS_H

libc/src/stdio/printf_core/oct_converter.h

+//===-- Octal Converter for printf ------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_OCT_CONVERTER_H
+#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_OCT_CONVERTER_H
+
+#include "src/stdio/printf_core/converter_utils.h"
+#include "src/stdio/printf_core/core_structs.h"
+#include "src/stdio/printf_core/writer.h"
+
+#include <inttypes.h>
+#include <stddef.h>
+
+namespace __llvm_libc {
+namespace printf_core {
+
+int inline convert_oct(Writer *writer, const FormatSection &to_conv) {
+  // This is the number of digits it takes to represent a octal value of a
+  // certain number of bits. Each oct digit represents 3 bits, so the value is
+  // ceil(number of bits / 3).
+  constexpr size_t BUFF_LEN = ((sizeof(uintmax_t) * 8) + 2) / 3;
+  uintmax_t num = to_conv.conv_val_raw;
+  char buffer[BUFF_LEN];
+
+  num = apply_length_modifier(num, to_conv.length_modifier);
+
+  // Since the buffer is size to sized to be able fit the entire number, buf_cur
+  // can never reach 0. So, we do not need bounds checking on buf_cur.
+  size_t buff_cur = BUFF_LEN;
+  for (; num > 0 /* && buff_cur > 0 */; --buff_cur, num /= 8)
+    buffer[buff_cur - 1] = (num % 8) + '0';
+
+  size_t num_digits = BUFF_LEN - buff_cur;
+
+  // These are signed to prevent underflow due to negative values. Negative
+  // values are treated the same as 0.
+  int zeroes;
+  int spaces;
+
+  // Negative precision indicates that it was not specified.
+  if (to_conv.precision < 0) {
+    if ((to_conv.flags &
+         (FormatFlags::LEADING_ZEROES | FormatFlags::LEFT_JUSTIFIED)) ==
+        FormatFlags::LEADING_ZEROES) {
+      // If this conv has flag 0 but not - and no specified precision, it's
+      // padded with 0's instead of spaces identically to if precision =
+      // min_width. For example: ("%04o", 15) -> "0017"
+      zeroes = to_conv.min_width - num_digits;
+      spaces = 0;
+    } else if (num_digits < 1) {
+      // If no precision is specified, precision defaults to 1. This means that
+      // if the integer passed to the conversion is 0, a 0 will be printed.
+      // Example: ("%3o", 0) -> "  0"
+      zeroes = 1;
+      spaces = to_conv.min_width - zeroes;
+    } else {
+      // If there are enough digits to pass over the precision, just write the
+      // number, padded by spaces.
+      zeroes = 0;
+      spaces = to_conv.min_width - num_digits;
+    }
+  } else {
+    // If precision was specified, possibly write zeroes, and possibly write
+    // spaces. Example: ("%5.4o", 010000) -> "10000"
+    // If the check for if zeroes is negative was not there, spaces would be
+    // incorrectly evaluated as 1.
+    zeroes = to_conv.precision - num_digits; // a negative value means 0
+    if (zeroes < 0)
+      zeroes = 0;
+    spaces = to_conv.min_width - zeroes - num_digits;
+  }
+
+  // The alternate form prefix is "0", so it's handled by increasing the number
+  // of zeroes if necessary.
+  if (((to_conv.flags & FormatFlags::ALTERNATE_FORM) ==
+       FormatFlags::ALTERNATE_FORM) &&
+      zeroes < 1) {
+    zeroes = 1;
+    --spaces;
+  }
+
+  if ((to_conv.flags & FormatFlags::LEFT_JUSTIFIED) ==
+      FormatFlags::LEFT_JUSTIFIED) {
+    // If left justified the pattern is zeroes digits spaces
+    if (zeroes > 0)
+      RET_IF_RESULT_NEGATIVE(writer->write_chars('0', zeroes));
+    if (num_digits > 0)
+      RET_IF_RESULT_NEGATIVE(writer->write(buffer + buff_cur, num_digits));
+    if (spaces > 0)
+      RET_IF_RESULT_NEGATIVE(writer->write_chars(' ', spaces));
+  } else {
+    // Else the pattern is spaces zeroes digits
+    if (spaces > 0)
+      RET_IF_RESULT_NEGATIVE(writer->write_chars(' ', spaces));
+    if (zeroes > 0)
+      RET_IF_RESULT_NEGATIVE(writer->write_chars('0', zeroes));
+    if (num_digits > 0)
+      RET_IF_RESULT_NEGATIVE(writer->write(buffer + buff_cur, num_digits));
+  }
+  return 0;
+}
+
+} // namespace printf_core
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_OCT_CONVERTER_H

libc/test/src/stdio/printf_core/converter_test.cpp

TEST_F(LlvmLibcPrintfConverterTest, IntConversionSimple) {
   __llvm_libc::printf_core::convert(&writer, section);
 
   str_writer.terminate();
 
   ASSERT_STREQ(str, "12345");
   ASSERT_EQ(writer.get_chars_written(), 5);
 }
 
-// This needs to be switched to the new testing layout, but that's still in the
-// int patch so I need to land that first. This is what I get for not keeping my
-// patches small and focused.
 TEST(LlvmLibcPrintfConverterTest, HexConversion) {
   char str[20];
   __llvm_libc::printf_core::StringWriter str_writer(str);
   __llvm_libc::printf_core::Writer writer(
       reinterpret_cast<void *>(&str_writer),
       __llvm_libc::printf_core::write_to_string);
 
-  __llvm_libc::printf_core::FormatSection left_justified_conv;
-  left_justified_conv.has_conv = true;
-  left_justified_conv.raw_string = "%#018x";
-  left_justified_conv.raw_len = 6;
-  left_justified_conv.conv_name = 'x';
-  left_justified_conv.flags =
-      static_cast<__llvm_libc::printf_core::FormatFlags>(
-          __llvm_libc::printf_core::FormatFlags::ALTERNATE_FORM |
-          __llvm_libc::printf_core::FormatFlags::LEADING_ZEROES);
-  left_justified_conv.min_width = 18;
-  left_justified_conv.conv_val_raw = 0x123456ab;
-  __llvm_libc::printf_core::convert(&writer, left_justified_conv);
+  __llvm_libc::printf_core::FormatSection section;
+  section.has_conv = true;
+  section.raw_string = "%#018x";
+  section.raw_len = 6;
+  section.conv_name = 'x';
+  section.flags = static_cast<__llvm_libc::printf_core::FormatFlags>(
+  section.min_width = 18;
+  section.conv_val_raw = 0x123456ab;
+  __llvm_libc::printf_core::convert(&writer, section);
 
   str_writer.terminate();
   ASSERT_STREQ(str, "0x00000000123456ab");
   ASSERT_EQ(writer.get_chars_written(), 18);
 }
 
 TEST(LlvmLibcPrintfConverterTest, PointerConversion) {
   char str[20];
TEST(LlvmLibcPrintfConverterTest, PointerConversion) {
   section.conv_name = 'p';
   section.conv_val_ptr = (void *)(0x123456ab);
   __llvm_libc::printf_core::convert(&writer, section);
 
   str_writer.terminate();
   ASSERT_STREQ(str, "0x123456ab");
   ASSERT_EQ(writer.get_chars_written(), 10);
 }
+
+TEST(LlvmLibcPrintfConverterTest, OctConversion) {
+  char str[20];
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+
+  __llvm_libc::printf_core::FormatSection section;
+  section.has_conv = true;
+  section.raw_string = "%o";
+  section.raw_len = 2;
+  section.conv_name = 'o';
+  section.conv_val_raw = 01234;
+  __llvm_libc::printf_core::convert(&writer, section);
+
+  str_writer.terminate();
+  ASSERT_STREQ(str, "1234");
+  ASSERT_EQ(writer.get_chars_written(), 4);
+}

libc/test/src/stdio/sprintf_test.cpp

TEST(LlvmLibcSPrintfTest, PointerConv) {
   written = __llvm_libc::sprintf(buff, "%p", 0x1a2b3c4d);
   EXPECT_EQ(written, 10);
   ASSERT_STREQ(buff, "0x1a2b3c4d");
 
   written = __llvm_libc::sprintf(buff, "%p", buff);
   EXPECT_GT(written, 0);
 }
 
+TEST(LlvmLibcSPrintfTest, OctConv) {
+  char buff[64];
+  int written;
+
+  // Basic Tests.
+
+  written = __llvm_libc::sprintf(buff, "%o", 01234);
+  EXPECT_EQ(written, 4);
+  ASSERT_STREQ(buff, "1234");
+
+  written = __llvm_libc::sprintf(buff, "%o", 04567);
+  EXPECT_EQ(written, 4);
+  ASSERT_STREQ(buff, "4567");
+
+  // Length Modifier Tests.
+
+  written = __llvm_libc::sprintf(buff, "%hho", 0401);
+  EXPECT_EQ(written, 1);
+  ASSERT_STREQ(buff, "1");
+
+  written = __llvm_libc::sprintf(buff, "%llo", 01777777777777777777777ull);
+  EXPECT_EQ(written, 22);
+  ASSERT_STREQ(buff, "1777777777777777777777"); // ull max
+
+  written = __llvm_libc::sprintf(buff, "%to", ~ptrdiff_t(0));
+  if (sizeof(ptrdiff_t) == 8) {
+    EXPECT_EQ(written, 22);
+    ASSERT_STREQ(buff, "1777777777777777777777");
+  } else if (sizeof(ptrdiff_t) == 4) {
+    EXPECT_EQ(written, 11);
+    ASSERT_STREQ(buff, "37777777777");
+  }
+
+  // Min Width Tests.
+
+  written = __llvm_libc::sprintf(buff, "%4o", 0701);
+  EXPECT_EQ(written, 4);
+  ASSERT_STREQ(buff, " 701");
+
+  written = __llvm_libc::sprintf(buff, "%2o", 0107);
+  EXPECT_EQ(written, 3);
+  ASSERT_STREQ(buff, "107");
+
+  // Precision Tests.
+
+  written = __llvm_libc::sprintf(buff, "%o", 0);
+  EXPECT_EQ(written, 1);
+  ASSERT_STREQ(buff, "0");
+
+  written = __llvm_libc::sprintf(buff, "%.0o", 0);
+  EXPECT_EQ(written, 0);
+  ASSERT_STREQ(buff, "");
+
+  written = __llvm_libc::sprintf(buff, "%.5o", 0153);
+  EXPECT_EQ(written, 5);
+  ASSERT_STREQ(buff, "00153");
+
+  written = __llvm_libc::sprintf(buff, "%.2o", 0135);
+  EXPECT_EQ(written, 3);
+  ASSERT_STREQ(buff, "135");
+
+  // Flag Tests.
+
+  written = __llvm_libc::sprintf(buff, "%-5o", 0246);
+  EXPECT_EQ(written, 5);
+  ASSERT_STREQ(buff, "246  ");
+
+  written = __llvm_libc::sprintf(buff, "%#o", 0234);
+  EXPECT_EQ(written, 4);
+  ASSERT_STREQ(buff, "0234");
+
+  written = __llvm_libc::sprintf(buff, "%05o", 0470);
+  EXPECT_EQ(written, 5);
+  ASSERT_STREQ(buff, "00470");
+
+  written = __llvm_libc::sprintf(buff, "%0#6o", 0753);
+  EXPECT_EQ(written, 6);
+  ASSERT_STREQ(buff, "000753");
+
+  written = __llvm_libc::sprintf(buff, "%-#6o", 0642);
+  EXPECT_EQ(written, 6);
+  ASSERT_STREQ(buff, "0642  ");
+
+  // Combined Tests.
+
+  written = __llvm_libc::sprintf(buff, "%#-07o", 0703);
+  EXPECT_EQ(written, 7);
+  ASSERT_STREQ(buff, "0703   ");
+
+  written = __llvm_libc::sprintf(buff, "%7.5o", 0314);
+  EXPECT_EQ(written, 7);
+  ASSERT_STREQ(buff, "  00314");
+
+  written = __llvm_libc::sprintf(buff, "%#9.5o", 0234);
+  EXPECT_EQ(written, 9);
+  ASSERT_STREQ(buff, "    00234");
+
+  written = __llvm_libc::sprintf(buff, "%-7.5o", 0260);
+  EXPECT_EQ(written, 7);
+  ASSERT_STREQ(buff, "00260  ");
+
+  written = __llvm_libc::sprintf(buff, "%5.4o", 010000);
+  EXPECT_EQ(written, 5);
+  ASSERT_STREQ(buff, "10000");
+
+  // Multiple Conversion Tests.
+
+  written = __llvm_libc::sprintf(buff, "%10o %-#10o", 0456, 0123);
+  EXPECT_EQ(written, 21);
+  ASSERT_STREQ(buff, "       456 0123      ");
+
+  written = __llvm_libc::sprintf(buff, "%-5.4o%#.4o", 075, 025);
+  EXPECT_EQ(written, 9);
+  ASSERT_STREQ(buff, "0075 0025");
+
+  written = __llvm_libc::sprintf(buff, "%04hho %#.5llo %-6.3zo", 256 + 077,
+                                 01000000000000ll, size_t(2));
+  EXPECT_EQ(written, 26);
+  ASSERT_STREQ(buff, "0077 01000000000000 002   ");
+}
+
 #ifndef LLVM_LIBC_PRINTF_DISABLE_INDEX_MODE
 TEST(LlvmLibcSPrintfTest, IndexModeParsing) {
   char buff[64];
   int written;
 
   written = __llvm_libc::sprintf(buff, "%1$s", "abcDEF123");
   EXPECT_EQ(written, 9);
   ASSERT_STREQ(buff, "abcDEF123");