[libc-commits] [PATCH] D144679: [libc] Clarify printf percent conversion behavior.

[Michael Jones via Phabricator via libc-commits]

michaelrj updated this revision to Diff 500252.
michaelrj added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144679/new/

https://reviews.llvm.org/D144679

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


Index: libc/test/src/stdio/printf_core/parser_test.cpp
===================================================================
--- libc/test/src/stdio/printf_core/parser_test.cpp
+++ libc/test/src/stdio/printf_core/parser_test.cpp
@@ -523,4 +523,29 @@
   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
Index: libc/src/stdio/printf_core/parser.cpp
===================================================================
--- libc/src/stdio/printf_core/parser.cpp
+++ libc/src/stdio/printf_core/parser.cpp
@@ -41,8 +41,9 @@
     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 @@
     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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144679.500252.patch
Type: text/x-patch
Size: 3044 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libc-commits/attachments/20230224/f29451a1/attachment.bin>