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

…105745)"

This reverts commit fc11020.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

@@ -1401,10 +1401,7 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
         ErrnoNote =
             llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote);
     } else {
-      // 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);
+      CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName);
     }
     const SVal RV = Call.getReturnValue();
 

llvm/benchmarks/CMakeLists.txt

@@ -5,4 +5,3 @@ 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)

llvm/include/llvm/Support/FormatVariadic.h

@@ -67,20 +67,23 @@ class formatv_object_base {
 protected:
   StringRef Fmt;
   ArrayRef<support::detail::format_adapter *> Adapters;
-  bool Validate;
+
+  static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+                                 size_t &Align, char &Pad);
+
+  static std::pair<ReplacementItem, StringRef>
+  splitLiteralAndReplacement(StringRef Fmt);
 
   formatv_object_base(StringRef Fmt,
-                      ArrayRef<support::detail::format_adapter *> Adapters,
-                      bool Validate)
-      : Fmt(Fmt), Adapters(Adapters), Validate(Validate) {}
+                      ArrayRef<support::detail::format_adapter *> Adapters)
+      : Fmt(Fmt), Adapters(Adapters) {}
 
   formatv_object_base(formatv_object_base const &rhs) = delete;
   formatv_object_base(formatv_object_base &&rhs) = default;
 
 public:
   void format(raw_ostream &S) const {
-    const auto Replacements = parseFormatString(Fmt, Adapters.size(), Validate);
-    for (const auto &R : Replacements) {
+    for (auto &R : parseFormatString(Fmt)) {
       if (R.Type == ReplacementType::Empty)
         continue;
       if (R.Type == ReplacementType::Literal) {
@@ -98,10 +101,9 @@ class formatv_object_base {
       Align.format(S, R.Options);
     }
   }
+  static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt);
 
-  // Parse and optionally validate format string (in debug builds).
-  static SmallVector<ReplacementItem, 2>
-  parseFormatString(StringRef Fmt, size_t NumArgs, bool Validate);
+  static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec);
 
   std::string str() const {
     std::string Result;
@@ -147,8 +149,8 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
   };
 
 public:
-  formatv_object(StringRef Fmt, Tuple &&Params, bool Validate)
-      : formatv_object_base(Fmt, ParameterPointers, Validate),
+  formatv_object(StringRef Fmt, Tuple &&Params)
+      : formatv_object_base(Fmt, ParameterPointers),
         Parameters(std::move(Params)) {
     ParameterPointers = std::apply(create_adapters(), Parameters);
   }
@@ -245,22 +247,15 @@ 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(bool Validate, const char *Fmt, Ts &&...Vals)
+inline auto formatv(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))...));
-  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)...);
+  return formatv_object<ParamTuple>(
+      Fmt, std::make_tuple(support::detail::build_format_adapter(
+               std::forward<Ts>(Vals))...));
 }
 
 } // end namespace llvm

llvm/lib/Support/FormatVariadic.cpp

@@ -25,8 +25,8 @@ static std::optional<AlignStyle> translateLocChar(char C) {
   LLVM_BUILTIN_UNREACHABLE;
 }
 
-static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                               size_t &Align, char &Pad) {
+bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+                                             size_t &Align, char &Pad) {
   Where = AlignStyle::Right;
   Align = 0;
   Pad = ' ';
@@ -35,7 +35,8 @@ static bool 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.
@@ -54,7 +55,8 @@ static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
   return !Failed;
 }
 
-static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
+std::optional<ReplacementItem>
+formatv_object_base::parseReplacementItem(StringRef Spec) {
   StringRef RepString = Spec.trim("{}");
 
   // If the replacement sequence does not start with a non-negative integer,
@@ -80,14 +82,15 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
     RepString = StringRef();
   }
   RepString = RepString.trim();
-  assert(RepString.empty() &&
-         "Unexpected characters found in replacement string!");
+  if (!RepString.empty()) {
+    assert(false && "Unexpected characters found in replacement string!");
+  }
 
   return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
 }
 
-static std::pair<ReplacementItem, StringRef>
-splitLiteralAndReplacement(StringRef Fmt) {
+std::pair<ReplacementItem, StringRef>
+formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
   while (!Fmt.empty()) {
     // Everything up until the first brace is a literal.
     if (Fmt.front() != '{') {
@@ -140,77 +143,15 @@ 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, size_t NumArgs,
-                                       bool Validate) {
+formatv_object_base::parseFormatString(StringRef Fmt) {
   SmallVector<ReplacementItem, 2> Replacements;
-
-#if ENABLE_VALIDATION
-  const StringRef SavedFmtStr = Fmt;
-  size_t NumExpectedArgs = 0;
-#endif
-
+  ReplacementItem I;
   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;
 }