[libc-commits] [libc] 8f0814f - [libc] Clarify printf percent conversion behavior.

[Michael Jones via libc-commits]

Author: Michael Jones
Date: 2023-02-24T13:29:30-08:00
New Revision: 8f0814f5dcdc29a01d0d3e87d07227f553472a51

URL: https://github.com/llvm/llvm-project/commit/8f0814f5dcdc29a01d0d3e87d07227f553472a51
DIFF: https://github.com/llvm/llvm-project/commit/8f0814f5dcdc29a01d0d3e87d07227f553472a51.diff

LOG: [libc] Clarify printf percent conversion behavior.

Almost all printf conversions ending in '%' are undefined, but they're
traditionally treated as if the complete conversion specifier is "%%".
This patch modifies the parser to more closely match that behavior.

Reviewed By: sivachandra

Differential Revision: https://reviews.llvm.org/D144679

Added: 
    

Modified: 
    libc/src/stdio/printf_core/parser.cpp
    libc/test/src/stdio/printf_core/parser_test.cpp

Removed: 
    


################################################################################
diff  --git a/libc/src/stdio/printf_core/parser.cpp b/libc/src/stdio/printf_core/parser.cpp
index 85b38da3db8bf..6b2c174c3f233 100644
--- a/libc/src/stdio/printf_core/parser.cpp
+++ b/libc/src/stdio/printf_core/parser.cpp
@@ -41,8 +41,9 @@ template <typename T> using int_type_of_v = typename int_type_of<T>::type;
     auto temp = get_arg_value<arg_type>(index);                                \
     if (!temp.has_value()) {                                                   \
       section.has_conv = false;                                                \
+    } else {                                                                   \
+      dst = cpp::bit_cast<int_type_of_v<arg_type>>(temp.value());              \
     }                                                                          \
-    dst = cpp::bit_cast<int_type_of_v<arg_type>>(temp.value());                \
   }
 #else
 #define WRITE_ARG_VAL_SIMPLEST(dst, arg_type, _)                               \
@@ -106,6 +107,13 @@ FormatSection Parser::get_next_section() {
     section.conv_name = str[cur_pos];
     switch (str[cur_pos]) {
     case ('%'):
+      // Regardless of options, a % conversion is always safe. The standard says
+      // that "The complete conversion specification shall be %%" but it also
+      // says that "If a conversion specification is invalid, the behavior is
+      // undefined." Based on that we define that any conversion specification
+      // ending in '%' shall display as '%' regardless of any valid or invalid
+      // options.
+      section.has_conv = true;
       break;
     case ('c'):
       WRITE_ARG_VAL_SIMPLEST(section.conv_val_raw, int, conv_index);

diff  --git a/libc/test/src/stdio/printf_core/parser_test.cpp b/libc/test/src/stdio/printf_core/parser_test.cpp
index b0a1cfc5b8a00..61f8c7cbe580e 100644
--- a/libc/test/src/stdio/printf_core/parser_test.cpp
+++ b/libc/test/src/stdio/printf_core/parser_test.cpp
@@ -523,4 +523,29 @@ TEST(LlvmLibcPrintfParserTest, IndexModeTrailingPercentCrash) {
   EXPECT_PFORMAT_EQ(expected1, format_arr[1]);
 }
 
+TEST(LlvmLibcPrintfParserTest, DoublePercentIsAllowedInvalidIndex) {
+  __llvm_libc::printf_core::FormatSection format_arr[10];
+
+  // Normally this conversion specifier would be raw (due to having a width
+  // defined as an invalid argument) but since it's a % conversion it's allowed
+  // by this specific parser. Any % conversion that is not just "%%" is
+  // undefined, so this is implementation-specific behavior.
+  // The goal is to be consistent. A conversion specifier of "%L%" is also
+  // undefined, but is traditionally displayed as just "%". "%2%" is also
+  // displayed as "%", even though if the width was respected it should be " %".
+  // Finally, "%*%" traditionally is displayed as "%" but also traditionally
+  // consumes an argument, since the * consumes an integer. Therefore, having
+  // "%*2$%" display as "%" is consistent with other printf behavior.
+  const char *str = "%*2$%";
+
+  evaluate(format_arr, str, 1, 2);
+
+  __llvm_libc::printf_core::FormatSection expected0;
+  expected0.has_conv = true;
+
+  expected0.raw_string = str;
+  expected0.conv_name = '%';
+  EXPECT_PFORMAT_EQ(expected0, format_arr[0]);
+}
+
 #endif // LIBC_COPT_PRINTF_DISABLE_INDEX_MODE