Revert "Revert "[Support] Validate number of arguments passed to formatv()"" #106592

jurahul · 2024-08-29T17:39:22Z

Reverts #106589
The fix for bot failures caused by the reverted commit was committed already, so this revert is not needed.

…atv()" (…" This reverts commit ed37b5f.

llvmbot · 2024-08-29T17:39:57Z

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Rahul Joshi (jurahul)

Changes

Reverts llvm/llvm-project#106589
The fix for bot failures caused by the reverted commit was committed already, so this revert is not needed.

Patch is 23.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106592.diff

7 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+4-1)
(modified) llvm/benchmarks/CMakeLists.txt (+1)
(added) llvm/benchmarks/FormatVariadicBM.cpp (+63)
(modified) llvm/include/llvm/Support/FormatVariadic.h (+22-17)
(modified) llvm/lib/Support/FormatVariadic.cpp (+72-13)
(modified) llvm/unittests/Support/FormatVariadicTest.cpp (+31-25)
(modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+2-2)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 8f4bd17afc8581..4f30b2a0e7e7da 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1401,7 +1401,10 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
         ErrnoNote =
             llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote);
     } else {
-      CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName);
+      // Disable formatv() validation as the case note may not always have the
+      // {0} placeholder for function name.
+      CaseNote =
+          llvm::formatv(false, Case.getNote().str().c_str(), FunctionName);
     }
     const SVal RV = Call.getReturnValue();
 
diff --git a/llvm/benchmarks/CMakeLists.txt b/llvm/benchmarks/CMakeLists.txt
index 713d4ccd3c5975..e3366e6f3ffe19 100644
--- a/llvm/benchmarks/CMakeLists.txt
+++ b/llvm/benchmarks/CMakeLists.txt
@@ -5,3 +5,4 @@ set(LLVM_LINK_COMPONENTS
 add_benchmark(DummyYAML DummyYAML.cpp PARTIAL_SOURCES_INTENDED)
 add_benchmark(xxhash xxhash.cpp PARTIAL_SOURCES_INTENDED)
 add_benchmark(GetIntrinsicForClangBuiltin GetIntrinsicForClangBuiltin.cpp PARTIAL_SOURCES_INTENDED)
+add_benchmark(FormatVariadicBM FormatVariadicBM.cpp PARTIAL_SOURCES_INTENDED)
diff --git a/llvm/benchmarks/FormatVariadicBM.cpp b/llvm/benchmarks/FormatVariadicBM.cpp
new file mode 100644
index 00000000000000..c03ead400d0d5c
--- /dev/null
+++ b/llvm/benchmarks/FormatVariadicBM.cpp
@@ -0,0 +1,63 @@
+//===- FormatVariadicBM.cpp - formatv() benchmark ---------- --------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "benchmark/benchmark.h"
+#include "llvm/Support/FormatVariadic.h"
+#include <algorithm>
+#include <string>
+#include <vector>
+
+using namespace llvm;
+using namespace std;
+
+// Generate a list of format strings that have `NumReplacements` replacements
+// by permuting the replacements and some literal text.
+static vector<string> getFormatStrings(int NumReplacements) {
+  vector<string> Components;
+  for (int I = 0; I < NumReplacements; I++)
+    Components.push_back("{" + to_string(I) + "}");
+  // Intersperse these with some other literal text (_).
+  const string_view Literal = "____";
+  for (char C : Literal)
+    Components.push_back(string(1, C));
+
+  vector<string> Formats;
+  do {
+    string Concat;
+    for (const string &C : Components)
+      Concat += C;
+    Formats.emplace_back(Concat);
+  } while (next_permutation(Components.begin(), Components.end()));
+  return Formats;
+}
+
+// Generate the set of formats to exercise outside the benchmark code.
+static const vector<vector<string>> Formats = {
+    getFormatStrings(1), getFormatStrings(2), getFormatStrings(3),
+    getFormatStrings(4), getFormatStrings(5),
+};
+
+// Benchmark formatv() for a variety of format strings and 1-5 replacements.
+static void BM_FormatVariadic(benchmark::State &state) {
+  for (auto _ : state) {
+    for (const string &Fmt : Formats[0])
+      formatv(Fmt.c_str(), 1).str();
+    for (const string &Fmt : Formats[1])
+      formatv(Fmt.c_str(), 1, 2).str();
+    for (const string &Fmt : Formats[2])
+      formatv(Fmt.c_str(), 1, 2, 3).str();
+    for (const string &Fmt : Formats[3])
+      formatv(Fmt.c_str(), 1, 2, 3, 4).str();
+    for (const string &Fmt : Formats[4])
+      formatv(Fmt.c_str(), 1, 2, 3, 4, 5).str();
+  }
+}
+
+BENCHMARK(BM_FormatVariadic);
+
+BENCHMARK_MAIN();
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index 595f2cf559a428..f31ad70021579e 100644
--- a/llvm/include/llvm/Support/FormatVariadic.h
+++ b/llvm/include/llvm/Support/FormatVariadic.h
@@ -67,23 +67,20 @@ class formatv_object_base {
 protected:
   StringRef Fmt;
   ArrayRef<support::detail::format_adapter *> Adapters;
-
-  static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                                 size_t &Align, char &Pad);
-
-  static std::pair<ReplacementItem, StringRef>
-  splitLiteralAndReplacement(StringRef Fmt);
+  bool Validate;
 
   formatv_object_base(StringRef Fmt,
-                      ArrayRef<support::detail::format_adapter *> Adapters)
-      : Fmt(Fmt), Adapters(Adapters) {}
+                      ArrayRef<support::detail::format_adapter *> Adapters,
+                      bool Validate)
+      : Fmt(Fmt), Adapters(Adapters), Validate(Validate) {}
 
   formatv_object_base(formatv_object_base const &rhs) = delete;
   formatv_object_base(formatv_object_base &&rhs) = default;
 
 public:
   void format(raw_ostream &S) const {
-    for (auto &R : parseFormatString(Fmt)) {
+    const auto Replacements = parseFormatString(Fmt, Adapters.size(), Validate);
+    for (const auto &R : Replacements) {
       if (R.Type == ReplacementType::Empty)
         continue;
       if (R.Type == ReplacementType::Literal) {
@@ -101,9 +98,10 @@ class formatv_object_base {
       Align.format(S, R.Options);
     }
   }
-  static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt);
 
-  static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec);
+  // Parse and optionally validate format string (in debug builds).
+  static SmallVector<ReplacementItem, 2>
+  parseFormatString(StringRef Fmt, size_t NumArgs, bool Validate);
 
   std::string str() const {
     std::string Result;
@@ -149,8 +147,8 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
   };
 
 public:
-  formatv_object(StringRef Fmt, Tuple &&Params)
-      : formatv_object_base(Fmt, ParameterPointers),
+  formatv_object(StringRef Fmt, Tuple &&Params, bool Validate)
+      : formatv_object_base(Fmt, ParameterPointers, Validate),
         Parameters(std::move(Params)) {
     ParameterPointers = std::apply(create_adapters(), Parameters);
   }
@@ -247,15 +245,22 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
 // assertion.  Otherwise, it will try to do something reasonable, but in general
 // the details of what that is are undefined.
 //
+
+// formatv() with validation enable/disable controlled by the first argument.
 template <typename... Ts>
-inline auto formatv(const char *Fmt, Ts &&...Vals)
+inline auto formatv(bool Validate, const char *Fmt, Ts &&...Vals)
     -> formatv_object<decltype(std::make_tuple(
         support::detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
   using ParamTuple = decltype(std::make_tuple(
       support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
-  return formatv_object<ParamTuple>(
-      Fmt, std::make_tuple(support::detail::build_format_adapter(
-               std::forward<Ts>(Vals))...));
+  auto Params = std::make_tuple(
+      support::detail::build_format_adapter(std::forward<Ts>(Vals))...);
+  return formatv_object<ParamTuple>(Fmt, std::move(Params), Validate);
+}
+
+// formatv() with validation enabled.
+template <typename... Ts> inline auto formatv(const char *Fmt, Ts &&...Vals) {
+  return formatv<Ts...>(true, Fmt, std::forward<Ts>(Vals)...);
 }
 
 } // end namespace llvm
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index e25d036cdf1e8c..26d2b549136e43 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -25,8 +25,8 @@ static std::optional<AlignStyle> translateLocChar(char C) {
   LLVM_BUILTIN_UNREACHABLE;
 }
 
-bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                                             size_t &Align, char &Pad) {
+static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+                               size_t &Align, char &Pad) {
   Where = AlignStyle::Right;
   Align = 0;
   Pad = ' ';
@@ -35,8 +35,7 @@ bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
 
   if (Spec.size() > 1) {
     // A maximum of 2 characters at the beginning can be used for something
-    // other
-    // than the width.
+    // other than the width.
     // If Spec[1] is a loc char, then Spec[0] is a pad char and Spec[2:...]
     // contains the width.
     // Otherwise, if Spec[0] is a loc char, then Spec[1:...] contains the width.
@@ -55,8 +54,7 @@ bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
   return !Failed;
 }
 
-std::optional<ReplacementItem>
-formatv_object_base::parseReplacementItem(StringRef Spec) {
+static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
   StringRef RepString = Spec.trim("{}");
 
   // If the replacement sequence does not start with a non-negative integer,
@@ -82,15 +80,14 @@ formatv_object_base::parseReplacementItem(StringRef Spec) {
     RepString = StringRef();
   }
   RepString = RepString.trim();
-  if (!RepString.empty()) {
-    assert(false && "Unexpected characters found in replacement string!");
-  }
+  assert(RepString.empty() &&
+         "Unexpected characters found in replacement string!");
 
   return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
 }
 
-std::pair<ReplacementItem, StringRef>
-formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
+static std::pair<ReplacementItem, StringRef>
+splitLiteralAndReplacement(StringRef Fmt) {
   while (!Fmt.empty()) {
     // Everything up until the first brace is a literal.
     if (Fmt.front() != '{') {
@@ -143,15 +140,77 @@ formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
   return std::make_pair(ReplacementItem{Fmt}, StringRef());
 }
 
+#ifndef NDEBUG
+#define ENABLE_VALIDATION 1
+#else
+#define ENABLE_VALIDATION 0 // Conveniently enable validation in release mode.
+#endif
+
 SmallVector<ReplacementItem, 2>
-formatv_object_base::parseFormatString(StringRef Fmt) {
+formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
+                                       bool Validate) {
   SmallVector<ReplacementItem, 2> Replacements;
-  ReplacementItem I;
+
+#if ENABLE_VALIDATION
+  const StringRef SavedFmtStr = Fmt;
+  size_t NumExpectedArgs = 0;
+#endif
+
   while (!Fmt.empty()) {
+    ReplacementItem I;
     std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
     if (I.Type != ReplacementType::Empty)
       Replacements.push_back(I);
+#if ENABLE_VALIDATION
+    if (I.Type == ReplacementType::Format)
+      NumExpectedArgs = std::max(NumExpectedArgs, I.Index + 1);
+#endif
+  }
+
+#if ENABLE_VALIDATION
+  if (!Validate)
+    return Replacements;
+
+  // Perform additional validation. Verify that the number of arguments matches
+  // the number of replacement indices and that there are no holes in the
+  // replacement indices.
+
+  // When validation fails, return an array of replacement items that
+  // will print an error message as the outout of this formatv() (used when
+  // validation is enabled in release mode).
+  auto getErrorReplacements = [SavedFmtStr](StringLiteral ErrorMsg) {
+    return SmallVector<ReplacementItem, 2>{
+        ReplacementItem("Invalid formatv() call: "), ReplacementItem(ErrorMsg),
+        ReplacementItem(" for format string: "), ReplacementItem(SavedFmtStr)};
+  };
+
+  if (NumExpectedArgs != NumArgs) {
+    errs() << formatv(
+        "Expected {0} Args, but got {1} for format string '{2}'\n",
+        NumExpectedArgs, NumArgs, SavedFmtStr);
+    assert(0 && "Invalid formatv() call");
+    return getErrorReplacements("Unexpected number of arguments");
+  }
+
+  // Find the number of unique indices seen. All replacement indices
+  // are < NumExpectedArgs.
+  SmallVector<bool> Indices(NumExpectedArgs);
+  size_t Count = 0;
+  for (const ReplacementItem &I : Replacements) {
+    if (I.Type != ReplacementType::Format || Indices[I.Index])
+      continue;
+    Indices[I.Index] = true;
+    ++Count;
+  }
+
+  if (Count != NumExpectedArgs) {
+    errs() << formatv(
+        "Replacement field indices cannot have holes for format string '{0}'\n",
+        SavedFmtStr);
+    assert(0 && "Invalid format string");
+    return getErrorReplacements("Replacement indices have holes");
   }
+#endif // ENABLE_VALIDATION
   return Replacements;
 }
 
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index a78b25c53d7e43..6ee0d924867419 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -9,9 +9,11 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatAdapters.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
+using ::testing::HasSubstr;
 
 // Compile-time tests templates in the detail namespace.
 namespace {
@@ -35,14 +37,19 @@ struct NoFormat {};
 static_assert(uses_missing_provider<NoFormat>::value, "");
 }
 
+// Helper to parse format string with no validation.
+static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt) {
+  return formatv_object_base::parseFormatString(Fmt, 0, false);
+}
+
 TEST(FormatVariadicTest, EmptyFormatString) {
-  auto Replacements = formatv_object_base::parseFormatString("");
+  auto Replacements = parseFormatString("");
   EXPECT_EQ(0U, Replacements.size());
 }
 
 TEST(FormatVariadicTest, NoReplacements) {
   const StringRef kFormatString = "This is a test";
-  auto Replacements = formatv_object_base::parseFormatString(kFormatString);
+  auto Replacements = parseFormatString(kFormatString);
   ASSERT_EQ(1U, Replacements.size());
   EXPECT_EQ(kFormatString, Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -50,25 +57,25 @@ TEST(FormatVariadicTest, NoReplacements) {
 
 TEST(FormatVariadicTest, EscapedBrace) {
   // {{ should be replaced with {
-  auto Replacements = formatv_object_base::parseFormatString("{{");
+  auto Replacements = parseFormatString("{{");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("{", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // An even number N of braces should be replaced with N/2 braces.
-  Replacements = formatv_object_base::parseFormatString("{{{{{{");
+  Replacements = parseFormatString("{{{{{{");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("{{{", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // } does not require doubling up.
-  Replacements = formatv_object_base::parseFormatString("}");
+  Replacements = parseFormatString("}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("}", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // } does not require doubling up.
-  Replacements = formatv_object_base::parseFormatString("}}}");
+  Replacements = parseFormatString("}}}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("}}}", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -76,14 +83,14 @@ TEST(FormatVariadicTest, EscapedBrace) {
 
 TEST(FormatVariadicTest, ValidReplacementSequence) {
   // 1. Simple replacement - parameter index only
-  auto Replacements = formatv_object_base::parseFormatString("{0}");
+  auto Replacements = parseFormatString("{0}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
   EXPECT_EQ(0u, Replacements[0].Align);
   EXPECT_EQ("", Replacements[0].Options);
 
-  Replacements = formatv_object_base::parseFormatString("{1}");
+  Replacements = parseFormatString("{1}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(1u, Replacements[0].Index);
@@ -92,7 +99,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 2. Parameter index with right alignment
-  Replacements = formatv_object_base::parseFormatString("{0,3}");
+  Replacements = parseFormatString("{0,3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -101,7 +108,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 3. And left alignment
-  Replacements = formatv_object_base::parseFormatString("{0,-3}");
+  Replacements = parseFormatString("{0,-3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -110,7 +117,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 4. And center alignment
-  Replacements = formatv_object_base::parseFormatString("{0,=3}");
+  Replacements = parseFormatString("{0,=3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -119,7 +126,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 4. Parameter index with option string
-  Replacements = formatv_object_base::parseFormatString("{0:foo}");
+  Replacements = parseFormatString("{0:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -128,7 +135,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // 5. Parameter index with alignment before option string
-  Replacements = formatv_object_base::parseFormatString("{0,-3:foo}");
+  Replacements = parseFormatString("{0,-3:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -137,7 +144,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // 7. Parameter indices, options, and alignment can all have whitespace.
-  Replacements = formatv_object_base::parseFormatString("{ 0, -3 : foo }");
+  Replacements = parseFormatString("{ 0, -3 : foo }");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -147,7 +154,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
 
   // 8. Everything after the first option specifier is part of the style, even
   // if it contains another option specifier.
-  Replacements = formatv_object_base::parseFormatString("{0:0:1}");
+  Replacements = parseFormatString("{0:0:1}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0:0:1", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -157,7 +164,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("0:1", Replacements[0].Options);
 
   // 9. Custom padding character
-  Replacements = formatv_object_base::parseFormatString("{0,p+4:foo}");
+  Replacements = parseFormatString("{0,p+4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,p+4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -168,7 +175,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // Format string special characters are allowed as padding character
-  Replacements = formatv_object_base::parseFormatString("{0,-+4:foo}");
+  Replacements = parseFormatString("{0,-+4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,-+4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -178,7 +185,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ('-', Replacements[0].Pad);
   EXPECT_EQ("foo", Replacements[0].Options);
 
-  Replacements = formatv_object_base::parseFormatString("{0,+-4:foo}");
+  Replacements = parseFormatString("{0,+-4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,+-4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -188,7 +195,7 @@ TEST(FormatVa...
[truncated]

llvmbot · 2024-08-29T17:39:58Z

@llvm/pr-subscribers-mlir

Author: Rahul Joshi (jurahul)

Changes

Reverts llvm/llvm-project#106589
The fix for bot failures caused by the reverted commit was committed already, so this revert is not needed.

Patch is 23.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106592.diff

7 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+4-1)
(modified) llvm/benchmarks/CMakeLists.txt (+1)
(added) llvm/benchmarks/FormatVariadicBM.cpp (+63)
(modified) llvm/include/llvm/Support/FormatVariadic.h (+22-17)
(modified) llvm/lib/Support/FormatVariadic.cpp (+72-13)
(modified) llvm/unittests/Support/FormatVariadicTest.cpp (+31-25)
(modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+2-2)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 8f4bd17afc8581..4f30b2a0e7e7da 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1401,7 +1401,10 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
         ErrnoNote =
             llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote);
     } else {
-      CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName);
+      // Disable formatv() validation as the case note may not always have the
+      // {0} placeholder for function name.
+      CaseNote =
+          llvm::formatv(false, Case.getNote().str().c_str(), FunctionName);
     }
     const SVal RV = Call.getReturnValue();
 
diff --git a/llvm/benchmarks/CMakeLists.txt b/llvm/benchmarks/CMakeLists.txt
index 713d4ccd3c5975..e3366e6f3ffe19 100644
--- a/llvm/benchmarks/CMakeLists.txt
+++ b/llvm/benchmarks/CMakeLists.txt
@@ -5,3 +5,4 @@ set(LLVM_LINK_COMPONENTS
 add_benchmark(DummyYAML DummyYAML.cpp PARTIAL_SOURCES_INTENDED)
 add_benchmark(xxhash xxhash.cpp PARTIAL_SOURCES_INTENDED)
 add_benchmark(GetIntrinsicForClangBuiltin GetIntrinsicForClangBuiltin.cpp PARTIAL_SOURCES_INTENDED)
+add_benchmark(FormatVariadicBM FormatVariadicBM.cpp PARTIAL_SOURCES_INTENDED)
diff --git a/llvm/benchmarks/FormatVariadicBM.cpp b/llvm/benchmarks/FormatVariadicBM.cpp
new file mode 100644
index 00000000000000..c03ead400d0d5c
--- /dev/null
+++ b/llvm/benchmarks/FormatVariadicBM.cpp
@@ -0,0 +1,63 @@
+//===- FormatVariadicBM.cpp - formatv() benchmark ---------- --------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "benchmark/benchmark.h"
+#include "llvm/Support/FormatVariadic.h"
+#include <algorithm>
+#include <string>
+#include <vector>
+
+using namespace llvm;
+using namespace std;
+
+// Generate a list of format strings that have `NumReplacements` replacements
+// by permuting the replacements and some literal text.
+static vector<string> getFormatStrings(int NumReplacements) {
+  vector<string> Components;
+  for (int I = 0; I < NumReplacements; I++)
+    Components.push_back("{" + to_string(I) + "}");
+  // Intersperse these with some other literal text (_).
+  const string_view Literal = "____";
+  for (char C : Literal)
+    Components.push_back(string(1, C));
+
+  vector<string> Formats;
+  do {
+    string Concat;
+    for (const string &C : Components)
+      Concat += C;
+    Formats.emplace_back(Concat);
+  } while (next_permutation(Components.begin(), Components.end()));
+  return Formats;
+}
+
+// Generate the set of formats to exercise outside the benchmark code.
+static const vector<vector<string>> Formats = {
+    getFormatStrings(1), getFormatStrings(2), getFormatStrings(3),
+    getFormatStrings(4), getFormatStrings(5),
+};
+
+// Benchmark formatv() for a variety of format strings and 1-5 replacements.
+static void BM_FormatVariadic(benchmark::State &state) {
+  for (auto _ : state) {
+    for (const string &Fmt : Formats[0])
+      formatv(Fmt.c_str(), 1).str();
+    for (const string &Fmt : Formats[1])
+      formatv(Fmt.c_str(), 1, 2).str();
+    for (const string &Fmt : Formats[2])
+      formatv(Fmt.c_str(), 1, 2, 3).str();
+    for (const string &Fmt : Formats[3])
+      formatv(Fmt.c_str(), 1, 2, 3, 4).str();
+    for (const string &Fmt : Formats[4])
+      formatv(Fmt.c_str(), 1, 2, 3, 4, 5).str();
+  }
+}
+
+BENCHMARK(BM_FormatVariadic);
+
+BENCHMARK_MAIN();
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index 595f2cf559a428..f31ad70021579e 100644
--- a/llvm/include/llvm/Support/FormatVariadic.h
+++ b/llvm/include/llvm/Support/FormatVariadic.h
@@ -67,23 +67,20 @@ class formatv_object_base {
 protected:
   StringRef Fmt;
   ArrayRef<support::detail::format_adapter *> Adapters;
-
-  static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                                 size_t &Align, char &Pad);
-
-  static std::pair<ReplacementItem, StringRef>
-  splitLiteralAndReplacement(StringRef Fmt);
+  bool Validate;
 
   formatv_object_base(StringRef Fmt,
-                      ArrayRef<support::detail::format_adapter *> Adapters)
-      : Fmt(Fmt), Adapters(Adapters) {}
+                      ArrayRef<support::detail::format_adapter *> Adapters,
+                      bool Validate)
+      : Fmt(Fmt), Adapters(Adapters), Validate(Validate) {}
 
   formatv_object_base(formatv_object_base const &rhs) = delete;
   formatv_object_base(formatv_object_base &&rhs) = default;
 
 public:
   void format(raw_ostream &S) const {
-    for (auto &R : parseFormatString(Fmt)) {
+    const auto Replacements = parseFormatString(Fmt, Adapters.size(), Validate);
+    for (const auto &R : Replacements) {
       if (R.Type == ReplacementType::Empty)
         continue;
       if (R.Type == ReplacementType::Literal) {
@@ -101,9 +98,10 @@ class formatv_object_base {
       Align.format(S, R.Options);
     }
   }
-  static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt);
 
-  static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec);
+  // Parse and optionally validate format string (in debug builds).
+  static SmallVector<ReplacementItem, 2>
+  parseFormatString(StringRef Fmt, size_t NumArgs, bool Validate);
 
   std::string str() const {
     std::string Result;
@@ -149,8 +147,8 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
   };
 
 public:
-  formatv_object(StringRef Fmt, Tuple &&Params)
-      : formatv_object_base(Fmt, ParameterPointers),
+  formatv_object(StringRef Fmt, Tuple &&Params, bool Validate)
+      : formatv_object_base(Fmt, ParameterPointers, Validate),
         Parameters(std::move(Params)) {
     ParameterPointers = std::apply(create_adapters(), Parameters);
   }
@@ -247,15 +245,22 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
 // assertion.  Otherwise, it will try to do something reasonable, but in general
 // the details of what that is are undefined.
 //
+
+// formatv() with validation enable/disable controlled by the first argument.
 template <typename... Ts>
-inline auto formatv(const char *Fmt, Ts &&...Vals)
+inline auto formatv(bool Validate, const char *Fmt, Ts &&...Vals)
     -> formatv_object<decltype(std::make_tuple(
         support::detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
   using ParamTuple = decltype(std::make_tuple(
       support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
-  return formatv_object<ParamTuple>(
-      Fmt, std::make_tuple(support::detail::build_format_adapter(
-               std::forward<Ts>(Vals))...));
+  auto Params = std::make_tuple(
+      support::detail::build_format_adapter(std::forward<Ts>(Vals))...);
+  return formatv_object<ParamTuple>(Fmt, std::move(Params), Validate);
+}
+
+// formatv() with validation enabled.
+template <typename... Ts> inline auto formatv(const char *Fmt, Ts &&...Vals) {
+  return formatv<Ts...>(true, Fmt, std::forward<Ts>(Vals)...);
 }
 
 } // end namespace llvm
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index e25d036cdf1e8c..26d2b549136e43 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -25,8 +25,8 @@ static std::optional<AlignStyle> translateLocChar(char C) {
   LLVM_BUILTIN_UNREACHABLE;
 }
 
-bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                                             size_t &Align, char &Pad) {
+static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+                               size_t &Align, char &Pad) {
   Where = AlignStyle::Right;
   Align = 0;
   Pad = ' ';
@@ -35,8 +35,7 @@ bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
 
   if (Spec.size() > 1) {
     // A maximum of 2 characters at the beginning can be used for something
-    // other
-    // than the width.
+    // other than the width.
     // If Spec[1] is a loc char, then Spec[0] is a pad char and Spec[2:...]
     // contains the width.
     // Otherwise, if Spec[0] is a loc char, then Spec[1:...] contains the width.
@@ -55,8 +54,7 @@ bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
   return !Failed;
 }
 
-std::optional<ReplacementItem>
-formatv_object_base::parseReplacementItem(StringRef Spec) {
+static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
   StringRef RepString = Spec.trim("{}");
 
   // If the replacement sequence does not start with a non-negative integer,
@@ -82,15 +80,14 @@ formatv_object_base::parseReplacementItem(StringRef Spec) {
     RepString = StringRef();
   }
   RepString = RepString.trim();
-  if (!RepString.empty()) {
-    assert(false && "Unexpected characters found in replacement string!");
-  }
+  assert(RepString.empty() &&
+         "Unexpected characters found in replacement string!");
 
   return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
 }
 
-std::pair<ReplacementItem, StringRef>
-formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
+static std::pair<ReplacementItem, StringRef>
+splitLiteralAndReplacement(StringRef Fmt) {
   while (!Fmt.empty()) {
     // Everything up until the first brace is a literal.
     if (Fmt.front() != '{') {
@@ -143,15 +140,77 @@ formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
   return std::make_pair(ReplacementItem{Fmt}, StringRef());
 }
 
+#ifndef NDEBUG
+#define ENABLE_VALIDATION 1
+#else
+#define ENABLE_VALIDATION 0 // Conveniently enable validation in release mode.
+#endif
+
 SmallVector<ReplacementItem, 2>
-formatv_object_base::parseFormatString(StringRef Fmt) {
+formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
+                                       bool Validate) {
   SmallVector<ReplacementItem, 2> Replacements;
-  ReplacementItem I;
+
+#if ENABLE_VALIDATION
+  const StringRef SavedFmtStr = Fmt;
+  size_t NumExpectedArgs = 0;
+#endif
+
   while (!Fmt.empty()) {
+    ReplacementItem I;
     std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
     if (I.Type != ReplacementType::Empty)
       Replacements.push_back(I);
+#if ENABLE_VALIDATION
+    if (I.Type == ReplacementType::Format)
+      NumExpectedArgs = std::max(NumExpectedArgs, I.Index + 1);
+#endif
+  }
+
+#if ENABLE_VALIDATION
+  if (!Validate)
+    return Replacements;
+
+  // Perform additional validation. Verify that the number of arguments matches
+  // the number of replacement indices and that there are no holes in the
+  // replacement indices.
+
+  // When validation fails, return an array of replacement items that
+  // will print an error message as the outout of this formatv() (used when
+  // validation is enabled in release mode).
+  auto getErrorReplacements = [SavedFmtStr](StringLiteral ErrorMsg) {
+    return SmallVector<ReplacementItem, 2>{
+        ReplacementItem("Invalid formatv() call: "), ReplacementItem(ErrorMsg),
+        ReplacementItem(" for format string: "), ReplacementItem(SavedFmtStr)};
+  };
+
+  if (NumExpectedArgs != NumArgs) {
+    errs() << formatv(
+        "Expected {0} Args, but got {1} for format string '{2}'\n",
+        NumExpectedArgs, NumArgs, SavedFmtStr);
+    assert(0 && "Invalid formatv() call");
+    return getErrorReplacements("Unexpected number of arguments");
+  }
+
+  // Find the number of unique indices seen. All replacement indices
+  // are < NumExpectedArgs.
+  SmallVector<bool> Indices(NumExpectedArgs);
+  size_t Count = 0;
+  for (const ReplacementItem &I : Replacements) {
+    if (I.Type != ReplacementType::Format || Indices[I.Index])
+      continue;
+    Indices[I.Index] = true;
+    ++Count;
+  }
+
+  if (Count != NumExpectedArgs) {
+    errs() << formatv(
+        "Replacement field indices cannot have holes for format string '{0}'\n",
+        SavedFmtStr);
+    assert(0 && "Invalid format string");
+    return getErrorReplacements("Replacement indices have holes");
   }
+#endif // ENABLE_VALIDATION
   return Replacements;
 }
 
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index a78b25c53d7e43..6ee0d924867419 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -9,9 +9,11 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatAdapters.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
+using ::testing::HasSubstr;
 
 // Compile-time tests templates in the detail namespace.
 namespace {
@@ -35,14 +37,19 @@ struct NoFormat {};
 static_assert(uses_missing_provider<NoFormat>::value, "");
 }
 
+// Helper to parse format string with no validation.
+static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt) {
+  return formatv_object_base::parseFormatString(Fmt, 0, false);
+}
+
 TEST(FormatVariadicTest, EmptyFormatString) {
-  auto Replacements = formatv_object_base::parseFormatString("");
+  auto Replacements = parseFormatString("");
   EXPECT_EQ(0U, Replacements.size());
 }
 
 TEST(FormatVariadicTest, NoReplacements) {
   const StringRef kFormatString = "This is a test";
-  auto Replacements = formatv_object_base::parseFormatString(kFormatString);
+  auto Replacements = parseFormatString(kFormatString);
   ASSERT_EQ(1U, Replacements.size());
   EXPECT_EQ(kFormatString, Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -50,25 +57,25 @@ TEST(FormatVariadicTest, NoReplacements) {
 
 TEST(FormatVariadicTest, EscapedBrace) {
   // {{ should be replaced with {
-  auto Replacements = formatv_object_base::parseFormatString("{{");
+  auto Replacements = parseFormatString("{{");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("{", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // An even number N of braces should be replaced with N/2 braces.
-  Replacements = formatv_object_base::parseFormatString("{{{{{{");
+  Replacements = parseFormatString("{{{{{{");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("{{{", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // } does not require doubling up.
-  Replacements = formatv_object_base::parseFormatString("}");
+  Replacements = parseFormatString("}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("}", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // } does not require doubling up.
-  Replacements = formatv_object_base::parseFormatString("}}}");
+  Replacements = parseFormatString("}}}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("}}}", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -76,14 +83,14 @@ TEST(FormatVariadicTest, EscapedBrace) {
 
 TEST(FormatVariadicTest, ValidReplacementSequence) {
   // 1. Simple replacement - parameter index only
-  auto Replacements = formatv_object_base::parseFormatString("{0}");
+  auto Replacements = parseFormatString("{0}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
   EXPECT_EQ(0u, Replacements[0].Align);
   EXPECT_EQ("", Replacements[0].Options);
 
-  Replacements = formatv_object_base::parseFormatString("{1}");
+  Replacements = parseFormatString("{1}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(1u, Replacements[0].Index);
@@ -92,7 +99,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 2. Parameter index with right alignment
-  Replacements = formatv_object_base::parseFormatString("{0,3}");
+  Replacements = parseFormatString("{0,3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -101,7 +108,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 3. And left alignment
-  Replacements = formatv_object_base::parseFormatString("{0,-3}");
+  Replacements = parseFormatString("{0,-3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -110,7 +117,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 4. And center alignment
-  Replacements = formatv_object_base::parseFormatString("{0,=3}");
+  Replacements = parseFormatString("{0,=3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -119,7 +126,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 4. Parameter index with option string
-  Replacements = formatv_object_base::parseFormatString("{0:foo}");
+  Replacements = parseFormatString("{0:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -128,7 +135,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // 5. Parameter index with alignment before option string
-  Replacements = formatv_object_base::parseFormatString("{0,-3:foo}");
+  Replacements = parseFormatString("{0,-3:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -137,7 +144,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // 7. Parameter indices, options, and alignment can all have whitespace.
-  Replacements = formatv_object_base::parseFormatString("{ 0, -3 : foo }");
+  Replacements = parseFormatString("{ 0, -3 : foo }");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -147,7 +154,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
 
   // 8. Everything after the first option specifier is part of the style, even
   // if it contains another option specifier.
-  Replacements = formatv_object_base::parseFormatString("{0:0:1}");
+  Replacements = parseFormatString("{0:0:1}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0:0:1", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -157,7 +164,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("0:1", Replacements[0].Options);
 
   // 9. Custom padding character
-  Replacements = formatv_object_base::parseFormatString("{0,p+4:foo}");
+  Replacements = parseFormatString("{0,p+4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,p+4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -168,7 +175,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // Format string special characters are allowed as padding character
-  Replacements = formatv_object_base::parseFormatString("{0,-+4:foo}");
+  Replacements = parseFormatString("{0,-+4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,-+4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -178,7 +185,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ('-', Replacements[0].Pad);
   EXPECT_EQ("foo", Replacements[0].Options);
 
-  Replacements = formatv_object_base::parseFormatString("{0,+-4:foo}");
+  Replacements = parseFormatString("{0,+-4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,+-4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -188,7 +195,7 @@ TEST(FormatVa...
[truncated]

llvmbot · 2024-08-29T17:39:58Z

@llvm/pr-subscribers-clang

Author: Rahul Joshi (jurahul)

Changes

Reverts llvm/llvm-project#106589
The fix for bot failures caused by the reverted commit was committed already, so this revert is not needed.

Patch is 23.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106592.diff

7 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+4-1)
(modified) llvm/benchmarks/CMakeLists.txt (+1)
(added) llvm/benchmarks/FormatVariadicBM.cpp (+63)
(modified) llvm/include/llvm/Support/FormatVariadic.h (+22-17)
(modified) llvm/lib/Support/FormatVariadic.cpp (+72-13)
(modified) llvm/unittests/Support/FormatVariadicTest.cpp (+31-25)
(modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+2-2)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 8f4bd17afc8581..4f30b2a0e7e7da 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1401,7 +1401,10 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
         ErrnoNote =
             llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote);
     } else {
-      CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName);
+      // Disable formatv() validation as the case note may not always have the
+      // {0} placeholder for function name.
+      CaseNote =
+          llvm::formatv(false, Case.getNote().str().c_str(), FunctionName);
     }
     const SVal RV = Call.getReturnValue();
 
diff --git a/llvm/benchmarks/CMakeLists.txt b/llvm/benchmarks/CMakeLists.txt
index 713d4ccd3c5975..e3366e6f3ffe19 100644
--- a/llvm/benchmarks/CMakeLists.txt
+++ b/llvm/benchmarks/CMakeLists.txt
@@ -5,3 +5,4 @@ set(LLVM_LINK_COMPONENTS
 add_benchmark(DummyYAML DummyYAML.cpp PARTIAL_SOURCES_INTENDED)
 add_benchmark(xxhash xxhash.cpp PARTIAL_SOURCES_INTENDED)
 add_benchmark(GetIntrinsicForClangBuiltin GetIntrinsicForClangBuiltin.cpp PARTIAL_SOURCES_INTENDED)
+add_benchmark(FormatVariadicBM FormatVariadicBM.cpp PARTIAL_SOURCES_INTENDED)
diff --git a/llvm/benchmarks/FormatVariadicBM.cpp b/llvm/benchmarks/FormatVariadicBM.cpp
new file mode 100644
index 00000000000000..c03ead400d0d5c
--- /dev/null
+++ b/llvm/benchmarks/FormatVariadicBM.cpp
@@ -0,0 +1,63 @@
+//===- FormatVariadicBM.cpp - formatv() benchmark ---------- --------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "benchmark/benchmark.h"
+#include "llvm/Support/FormatVariadic.h"
+#include <algorithm>
+#include <string>
+#include <vector>
+
+using namespace llvm;
+using namespace std;
+
+// Generate a list of format strings that have `NumReplacements` replacements
+// by permuting the replacements and some literal text.
+static vector<string> getFormatStrings(int NumReplacements) {
+  vector<string> Components;
+  for (int I = 0; I < NumReplacements; I++)
+    Components.push_back("{" + to_string(I) + "}");
+  // Intersperse these with some other literal text (_).
+  const string_view Literal = "____";
+  for (char C : Literal)
+    Components.push_back(string(1, C));
+
+  vector<string> Formats;
+  do {
+    string Concat;
+    for (const string &C : Components)
+      Concat += C;
+    Formats.emplace_back(Concat);
+  } while (next_permutation(Components.begin(), Components.end()));
+  return Formats;
+}
+
+// Generate the set of formats to exercise outside the benchmark code.
+static const vector<vector<string>> Formats = {
+    getFormatStrings(1), getFormatStrings(2), getFormatStrings(3),
+    getFormatStrings(4), getFormatStrings(5),
+};
+
+// Benchmark formatv() for a variety of format strings and 1-5 replacements.
+static void BM_FormatVariadic(benchmark::State &state) {
+  for (auto _ : state) {
+    for (const string &Fmt : Formats[0])
+      formatv(Fmt.c_str(), 1).str();
+    for (const string &Fmt : Formats[1])
+      formatv(Fmt.c_str(), 1, 2).str();
+    for (const string &Fmt : Formats[2])
+      formatv(Fmt.c_str(), 1, 2, 3).str();
+    for (const string &Fmt : Formats[3])
+      formatv(Fmt.c_str(), 1, 2, 3, 4).str();
+    for (const string &Fmt : Formats[4])
+      formatv(Fmt.c_str(), 1, 2, 3, 4, 5).str();
+  }
+}
+
+BENCHMARK(BM_FormatVariadic);
+
+BENCHMARK_MAIN();
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index 595f2cf559a428..f31ad70021579e 100644
--- a/llvm/include/llvm/Support/FormatVariadic.h
+++ b/llvm/include/llvm/Support/FormatVariadic.h
@@ -67,23 +67,20 @@ class formatv_object_base {
 protected:
   StringRef Fmt;
   ArrayRef<support::detail::format_adapter *> Adapters;
-
-  static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                                 size_t &Align, char &Pad);
-
-  static std::pair<ReplacementItem, StringRef>
-  splitLiteralAndReplacement(StringRef Fmt);
+  bool Validate;
 
   formatv_object_base(StringRef Fmt,
-                      ArrayRef<support::detail::format_adapter *> Adapters)
-      : Fmt(Fmt), Adapters(Adapters) {}
+                      ArrayRef<support::detail::format_adapter *> Adapters,
+                      bool Validate)
+      : Fmt(Fmt), Adapters(Adapters), Validate(Validate) {}
 
   formatv_object_base(formatv_object_base const &rhs) = delete;
   formatv_object_base(formatv_object_base &&rhs) = default;
 
 public:
   void format(raw_ostream &S) const {
-    for (auto &R : parseFormatString(Fmt)) {
+    const auto Replacements = parseFormatString(Fmt, Adapters.size(), Validate);
+    for (const auto &R : Replacements) {
       if (R.Type == ReplacementType::Empty)
         continue;
       if (R.Type == ReplacementType::Literal) {
@@ -101,9 +98,10 @@ class formatv_object_base {
       Align.format(S, R.Options);
     }
   }
-  static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt);
 
-  static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec);
+  // Parse and optionally validate format string (in debug builds).
+  static SmallVector<ReplacementItem, 2>
+  parseFormatString(StringRef Fmt, size_t NumArgs, bool Validate);
 
   std::string str() const {
     std::string Result;
@@ -149,8 +147,8 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
   };
 
 public:
-  formatv_object(StringRef Fmt, Tuple &&Params)
-      : formatv_object_base(Fmt, ParameterPointers),
+  formatv_object(StringRef Fmt, Tuple &&Params, bool Validate)
+      : formatv_object_base(Fmt, ParameterPointers, Validate),
         Parameters(std::move(Params)) {
     ParameterPointers = std::apply(create_adapters(), Parameters);
   }
@@ -247,15 +245,22 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
 // assertion.  Otherwise, it will try to do something reasonable, but in general
 // the details of what that is are undefined.
 //
+
+// formatv() with validation enable/disable controlled by the first argument.
 template <typename... Ts>
-inline auto formatv(const char *Fmt, Ts &&...Vals)
+inline auto formatv(bool Validate, const char *Fmt, Ts &&...Vals)
     -> formatv_object<decltype(std::make_tuple(
         support::detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
   using ParamTuple = decltype(std::make_tuple(
       support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
-  return formatv_object<ParamTuple>(
-      Fmt, std::make_tuple(support::detail::build_format_adapter(
-               std::forward<Ts>(Vals))...));
+  auto Params = std::make_tuple(
+      support::detail::build_format_adapter(std::forward<Ts>(Vals))...);
+  return formatv_object<ParamTuple>(Fmt, std::move(Params), Validate);
+}
+
+// formatv() with validation enabled.
+template <typename... Ts> inline auto formatv(const char *Fmt, Ts &&...Vals) {
+  return formatv<Ts...>(true, Fmt, std::forward<Ts>(Vals)...);
 }
 
 } // end namespace llvm
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index e25d036cdf1e8c..26d2b549136e43 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -25,8 +25,8 @@ static std::optional<AlignStyle> translateLocChar(char C) {
   LLVM_BUILTIN_UNREACHABLE;
 }
 
-bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                                             size_t &Align, char &Pad) {
+static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+                               size_t &Align, char &Pad) {
   Where = AlignStyle::Right;
   Align = 0;
   Pad = ' ';
@@ -35,8 +35,7 @@ bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
 
   if (Spec.size() > 1) {
     // A maximum of 2 characters at the beginning can be used for something
-    // other
-    // than the width.
+    // other than the width.
     // If Spec[1] is a loc char, then Spec[0] is a pad char and Spec[2:...]
     // contains the width.
     // Otherwise, if Spec[0] is a loc char, then Spec[1:...] contains the width.
@@ -55,8 +54,7 @@ bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
   return !Failed;
 }
 
-std::optional<ReplacementItem>
-formatv_object_base::parseReplacementItem(StringRef Spec) {
+static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
   StringRef RepString = Spec.trim("{}");
 
   // If the replacement sequence does not start with a non-negative integer,
@@ -82,15 +80,14 @@ formatv_object_base::parseReplacementItem(StringRef Spec) {
     RepString = StringRef();
   }
   RepString = RepString.trim();
-  if (!RepString.empty()) {
-    assert(false && "Unexpected characters found in replacement string!");
-  }
+  assert(RepString.empty() &&
+         "Unexpected characters found in replacement string!");
 
   return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
 }
 
-std::pair<ReplacementItem, StringRef>
-formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
+static std::pair<ReplacementItem, StringRef>
+splitLiteralAndReplacement(StringRef Fmt) {
   while (!Fmt.empty()) {
     // Everything up until the first brace is a literal.
     if (Fmt.front() != '{') {
@@ -143,15 +140,77 @@ formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
   return std::make_pair(ReplacementItem{Fmt}, StringRef());
 }
 
+#ifndef NDEBUG
+#define ENABLE_VALIDATION 1
+#else
+#define ENABLE_VALIDATION 0 // Conveniently enable validation in release mode.
+#endif
+
 SmallVector<ReplacementItem, 2>
-formatv_object_base::parseFormatString(StringRef Fmt) {
+formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
+                                       bool Validate) {
   SmallVector<ReplacementItem, 2> Replacements;
-  ReplacementItem I;
+
+#if ENABLE_VALIDATION
+  const StringRef SavedFmtStr = Fmt;
+  size_t NumExpectedArgs = 0;
+#endif
+
   while (!Fmt.empty()) {
+    ReplacementItem I;
     std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
     if (I.Type != ReplacementType::Empty)
       Replacements.push_back(I);
+#if ENABLE_VALIDATION
+    if (I.Type == ReplacementType::Format)
+      NumExpectedArgs = std::max(NumExpectedArgs, I.Index + 1);
+#endif
+  }
+
+#if ENABLE_VALIDATION
+  if (!Validate)
+    return Replacements;
+
+  // Perform additional validation. Verify that the number of arguments matches
+  // the number of replacement indices and that there are no holes in the
+  // replacement indices.
+
+  // When validation fails, return an array of replacement items that
+  // will print an error message as the outout of this formatv() (used when
+  // validation is enabled in release mode).
+  auto getErrorReplacements = [SavedFmtStr](StringLiteral ErrorMsg) {
+    return SmallVector<ReplacementItem, 2>{
+        ReplacementItem("Invalid formatv() call: "), ReplacementItem(ErrorMsg),
+        ReplacementItem(" for format string: "), ReplacementItem(SavedFmtStr)};
+  };
+
+  if (NumExpectedArgs != NumArgs) {
+    errs() << formatv(
+        "Expected {0} Args, but got {1} for format string '{2}'\n",
+        NumExpectedArgs, NumArgs, SavedFmtStr);
+    assert(0 && "Invalid formatv() call");
+    return getErrorReplacements("Unexpected number of arguments");
+  }
+
+  // Find the number of unique indices seen. All replacement indices
+  // are < NumExpectedArgs.
+  SmallVector<bool> Indices(NumExpectedArgs);
+  size_t Count = 0;
+  for (const ReplacementItem &I : Replacements) {
+    if (I.Type != ReplacementType::Format || Indices[I.Index])
+      continue;
+    Indices[I.Index] = true;
+    ++Count;
+  }
+
+  if (Count != NumExpectedArgs) {
+    errs() << formatv(
+        "Replacement field indices cannot have holes for format string '{0}'\n",
+        SavedFmtStr);
+    assert(0 && "Invalid format string");
+    return getErrorReplacements("Replacement indices have holes");
   }
+#endif // ENABLE_VALIDATION
   return Replacements;
 }
 
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index a78b25c53d7e43..6ee0d924867419 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -9,9 +9,11 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatAdapters.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
+using ::testing::HasSubstr;
 
 // Compile-time tests templates in the detail namespace.
 namespace {
@@ -35,14 +37,19 @@ struct NoFormat {};
 static_assert(uses_missing_provider<NoFormat>::value, "");
 }
 
+// Helper to parse format string with no validation.
+static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt) {
+  return formatv_object_base::parseFormatString(Fmt, 0, false);
+}
+
 TEST(FormatVariadicTest, EmptyFormatString) {
-  auto Replacements = formatv_object_base::parseFormatString("");
+  auto Replacements = parseFormatString("");
   EXPECT_EQ(0U, Replacements.size());
 }
 
 TEST(FormatVariadicTest, NoReplacements) {
   const StringRef kFormatString = "This is a test";
-  auto Replacements = formatv_object_base::parseFormatString(kFormatString);
+  auto Replacements = parseFormatString(kFormatString);
   ASSERT_EQ(1U, Replacements.size());
   EXPECT_EQ(kFormatString, Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -50,25 +57,25 @@ TEST(FormatVariadicTest, NoReplacements) {
 
 TEST(FormatVariadicTest, EscapedBrace) {
   // {{ should be replaced with {
-  auto Replacements = formatv_object_base::parseFormatString("{{");
+  auto Replacements = parseFormatString("{{");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("{", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // An even number N of braces should be replaced with N/2 braces.
-  Replacements = formatv_object_base::parseFormatString("{{{{{{");
+  Replacements = parseFormatString("{{{{{{");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("{{{", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // } does not require doubling up.
-  Replacements = formatv_object_base::parseFormatString("}");
+  Replacements = parseFormatString("}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("}", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 
   // } does not require doubling up.
-  Replacements = formatv_object_base::parseFormatString("}}}");
+  Replacements = parseFormatString("}}}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("}}}", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -76,14 +83,14 @@ TEST(FormatVariadicTest, EscapedBrace) {
 
 TEST(FormatVariadicTest, ValidReplacementSequence) {
   // 1. Simple replacement - parameter index only
-  auto Replacements = formatv_object_base::parseFormatString("{0}");
+  auto Replacements = parseFormatString("{0}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
   EXPECT_EQ(0u, Replacements[0].Align);
   EXPECT_EQ("", Replacements[0].Options);
 
-  Replacements = formatv_object_base::parseFormatString("{1}");
+  Replacements = parseFormatString("{1}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(1u, Replacements[0].Index);
@@ -92,7 +99,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 2. Parameter index with right alignment
-  Replacements = formatv_object_base::parseFormatString("{0,3}");
+  Replacements = parseFormatString("{0,3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -101,7 +108,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 3. And left alignment
-  Replacements = formatv_object_base::parseFormatString("{0,-3}");
+  Replacements = parseFormatString("{0,-3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -110,7 +117,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 4. And center alignment
-  Replacements = formatv_object_base::parseFormatString("{0,=3}");
+  Replacements = parseFormatString("{0,=3}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -119,7 +126,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("", Replacements[0].Options);
 
   // 4. Parameter index with option string
-  Replacements = formatv_object_base::parseFormatString("{0:foo}");
+  Replacements = parseFormatString("{0:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -128,7 +135,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // 5. Parameter index with alignment before option string
-  Replacements = formatv_object_base::parseFormatString("{0,-3:foo}");
+  Replacements = parseFormatString("{0,-3:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -137,7 +144,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // 7. Parameter indices, options, and alignment can all have whitespace.
-  Replacements = formatv_object_base::parseFormatString("{ 0, -3 : foo }");
+  Replacements = parseFormatString("{ 0, -3 : foo }");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
@@ -147,7 +154,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
 
   // 8. Everything after the first option specifier is part of the style, even
   // if it contains another option specifier.
-  Replacements = formatv_object_base::parseFormatString("{0:0:1}");
+  Replacements = parseFormatString("{0:0:1}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0:0:1", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -157,7 +164,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("0:1", Replacements[0].Options);
 
   // 9. Custom padding character
-  Replacements = formatv_object_base::parseFormatString("{0,p+4:foo}");
+  Replacements = parseFormatString("{0,p+4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,p+4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -168,7 +175,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ("foo", Replacements[0].Options);
 
   // Format string special characters are allowed as padding character
-  Replacements = formatv_object_base::parseFormatString("{0,-+4:foo}");
+  Replacements = parseFormatString("{0,-+4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,-+4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -178,7 +185,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ('-', Replacements[0].Pad);
   EXPECT_EQ("foo", Replacements[0].Options);
 
-  Replacements = formatv_object_base::parseFormatString("{0,+-4:foo}");
+  Replacements = parseFormatString("{0,+-4:foo}");
   ASSERT_EQ(1u, Replacements.size());
   EXPECT_EQ("0,+-4:foo", Replacements[0].Spec);
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -188,7 +195,7 @@ TEST(FormatVa...
[truncated]

Revert "Revert "[Support] Validate number of arguments passed to form…

002d9ef

…atv()" (…" This reverts commit ed37b5f.

llvmbot added clang Clang issues not falling into any other category clang:static analyzer mlir:core MLIR Core Infrastructure mlir llvm:support labels Aug 29, 2024

jurahul merged commit 9ce4af5 into main Aug 29, 2024
6 of 8 checks passed

jurahul deleted the revert-106589-revert-105745-formatv_errorcheck branch August 29, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Revert "Revert "[Support] Validate number of arguments passed to formatv()"" #106592

Revert "Revert "[Support] Validate number of arguments passed to formatv()"" #106592

Uh oh!

jurahul commented Aug 29, 2024

Uh oh!

Uh oh!

llvmbot commented Aug 29, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

Uh oh!

Revert "Revert "[Support] Validate number of arguments passed to formatv()"" #106592

Revert "Revert "[Support] Validate number of arguments passed to formatv()"" #106592

Uh oh!

Conversation

jurahul commented Aug 29, 2024

Uh oh!

Uh oh!

llvmbot commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

llvmbot commented Aug 29, 2024

Uh oh!

Uh oh!

llvmbot commented Aug 29, 2024 •

edited

Loading