[libc] Clarify printf percent conversion behavior.

[michaelrj]

Summary

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.

[lntue]

libc/src/stdio/printf_core/parser.cpp

113

Probably we should have a section in the main doc page about implementation-defined behaviors and rationale, with links to the implementations and tests.

[sivachandra]

libc/src/stdio/printf_core/parser.cpp

113

Address this before landing: Please use "we" instead of "I".

Can be done as a follow up step: +1 for adding a doc page titled "Implementation Defined Behavior".

[michaelrj]

libc/src/stdio/printf_core/parser.cpp

113

I agree that we should have a page on implementation defined behavior and will do that in another patch.

[michaelrj]

fix comment phrasing before landing

[michaelrj]

Fix a minor bug found in final testing.

Setting the variable from the result of a failed index parsing was setting
unexpected pieces of the FormatSection struct. This wouldn't have affected
any actual conversions, since the only conversion that regains its has_conv
flag is %% which ignores all options, but it was causing the test to fail.

libc/src/stdio/printf_core/parser.cpp

 template <typename T> using int_type_of_v = typename int_type_of<T>::type;
 
 #ifndef LIBC_COPT_PRINTF_DISABLE_INDEX_MODE
 #define WRITE_ARG_VAL_SIMPLEST(dst, arg_type, index)                           \
   {                                                                            \
     auto temp = get_arg_value<arg_type>(index);                                \
     if (!temp.has_value()) {                                                   \
       section.has_conv = false;                                                \
-    }                                                                          \
-    dst = cpp::bit_cast<int_type_of_v<arg_type>>(temp.value());                \
+    } else {                                                                   \
+      dst = cpp::bit_cast<int_type_of_v<arg_type>>(temp.value());              \
+    }                                                                          \
   }
 #else
 #define WRITE_ARG_VAL_SIMPLEST(dst, arg_type, _)                               \
   dst = cpp::bit_cast<int_type_of_v<arg_type>>(get_next_arg_value<arg_type>())
 #endif // LIBC_COPT_PRINTF_DISABLE_INDEX_MODE
 
 FormatSection Parser::get_next_section() {
   FormatSection section;
#endif // LIBC_COPT_PRINTF_DISABLE_INDEX_MODE
     }
 
     LengthModifier lm = parse_length_modifier(&cur_pos);
 
     section.length_modifier = lm;
     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);
       break;
     case ('d'):
     case ('i'):
     case ('o'):
     case ('x'):

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

TEST(LlvmLibcPrintfParserTest, IndexModeTrailingPercentCrash) {
   EXPECT_PFORMAT_EQ(expected0, format_arr[0]);
 
   expected1.has_conv = false;
 
   expected1.raw_string = {str + 4, 1};
   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