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

[Michael Jones via Phabricator via libc-commits]

michaelrj created this revision.
michaelrj added reviewers: sivachandra, lntue.
Herald added subscribers: libc-commits, ecnelises, tschuett.
Herald added projects: libc-project, All.
michaelrj requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

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[20];
+
+  // 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
@@ -106,6 +106,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 I 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.499989.patch
Type: text/x-patch
Size: 2357 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libc-commits/attachments/20230223/7cbbb654/attachment.bin>