[Support] Validate number of arguments passed to formatv() #105745
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zwuis
reviewed
Aug 23, 2024
jurahul
force-pushed
the
formatv_errorcheck
branch
5 times, most recently
from
d834ebb to
9ae8a03
Compare
jurahul
requested review from
antiagainst,
kuhar and
JDevlieghere
as code owners
llvmbot
added
clang
|
CI failed FYI. |
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-lldb Author: Rahul Joshi (jurahul) Changes
Patch is 39.55 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105745.diff 15 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
index 99e11a15c08dc2..1b89951397cfb1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -131,7 +131,7 @@ bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
"Storage provided to placement new is only {0} bytes, "
"whereas the allocated array type requires more space for "
"internal needs",
- SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+ SizeOfPlaceCI->getValue()));
else
Msg = std::string(llvm::formatv(
"Storage provided to placement new is only {0} bytes, "
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 8f4bd17afc8581..60c035612dcd44 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1401,7 +1401,7 @@ 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);
+ CaseNote = llvm::formatvv(Case.getNote().str().c_str(), FunctionName);
}
const SVal RV = Call.getReturnValue();
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
index feb72f6244a18c..cdb266f70f0e3e 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -655,7 +655,7 @@ DWARFUnit::GetDIE(dw_offset_t die_offset) {
if (!ContainsDIEOffset(die_offset)) {
GetSymbolFileDWARF().GetObjectFile()->GetModule()->ReportError(
- "GetDIE for DIE {0:x16} is outside of its CU {0:x16}", die_offset,
+ "GetDIE for DIE {0:x16} is outside of its CU {1:x16}", die_offset,
GetOffset());
return DWARFDIE(); // Not found
}
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index 595f2cf559a428..b42e24646b31b5 100644
--- a/llvm/include/llvm/Support/FormatVariadic.h
+++ b/llvm/include/llvm/Support/FormatVariadic.h
@@ -66,24 +66,24 @@ struct ReplacementItem {
class formatv_object_base {
protected:
StringRef Fmt;
+ bool Validate;
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);
-
- formatv_object_base(StringRef Fmt,
+ formatv_object_base(StringRef Fmt, bool Validate,
ArrayRef<support::detail::format_adapter *> Adapters)
- : Fmt(Fmt), Adapters(Adapters) {}
+ : Fmt(Fmt), Validate(Validate), 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 {
- for (auto &R : parseFormatString(Fmt)) {
+ const auto [Replacements, IsValid] =
+ parseFormatString(S, Fmt, Adapters.size(), Validate);
+ if (!IsValid)
+ return;
+
+ for (const auto &R : Replacements) {
if (R.Type == ReplacementType::Empty)
continue;
if (R.Type == ReplacementType::Literal) {
@@ -101,9 +101,13 @@ class formatv_object_base {
Align.format(S, R.Options);
}
}
- static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt);
- static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec);
+ // Parse format string and return the array of replacement items. If there is
+ // an error in the format string, return false for the second member of the
+ // pair, and print the error message to `S`.
+ static std::pair<SmallVector<ReplacementItem, 2>, bool>
+ parseFormatString(raw_ostream &S, StringRef Fmt, size_t NumArgs,
+ bool Validate);
std::string str() const {
std::string Result;
@@ -149,8 +153,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, bool ValidateNumArgs, Tuple &&Params)
+ : formatv_object_base(Fmt, ValidateNumArgs, ParameterPointers),
Parameters(std::move(Params)) {
ParameterPointers = std::apply(create_adapters(), Parameters);
}
@@ -174,7 +178,7 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
//
// rep_field ::= "{" index ["," layout] [":" format] "}"
// index ::= <non-negative integer>
-// layout ::= [[[char]loc]width]
+// layout ::= [[[pad]loc]width]
// format ::= <any string not containing "{" or "}">
// char ::= <any character except "{" or "}">
// loc ::= "-" | "=" | "+"
@@ -187,7 +191,7 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
// format - A type-dependent string used to provide additional options to
// the formatting operation. Refer to the documentation of the
// various individual format providers for per-type options.
-// char - The padding character. Defaults to ' ' (space). Only valid if
+// pad - The padding character. Defaults to ' ' (space). Only valid if
// `loc` is also specified.
// loc - Where to print the formatted text within the field. Only valid if
// `width` is also specified.
@@ -247,6 +251,8 @@ 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 enabled.
template <typename... Ts>
inline auto formatv(const char *Fmt, Ts &&...Vals)
-> formatv_object<decltype(std::make_tuple(
@@ -254,8 +260,22 @@ inline auto formatv(const char *Fmt, 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))...));
+ Fmt, /*Validate=*/true,
+ std::make_tuple(
+ support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
+}
+
+// formatvv() perform no argument/format string validation.
+template <typename... Ts>
+inline auto formatvv(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, /*Validate=*/false,
+ std::make_tuple(
+ support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
}
} // end namespace llvm
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
index 77e8ece9439cf9..eb2751ab30ac50 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
@@ -1518,8 +1518,7 @@ DWARFVerifier::verifyNameIndexAbbrevs(const DWARFDebugNames::NameIndex &NI) {
error() << formatv("NameIndex @ {0:x}: Indexing multiple compile units "
"and abbreviation {1:x} has no DW_IDX_compile_unit "
"or DW_IDX_type_unit attribute.\n",
- NI.getUnitOffset(), Abbrev.Code,
- dwarf::DW_IDX_compile_unit);
+ NI.getUnitOffset(), Abbrev.Code);
});
++NumErrors;
}
diff --git a/llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp b/llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp
index 6448adaa0ceb36..49f239b73e8a01 100644
--- a/llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp
@@ -348,10 +348,10 @@ void CompileOnDemandLayer::emitPartition(
HC = hash_combine(HC, hash_combine_range(GVName.begin(), GVName.end()));
}
raw_string_ostream(SubModuleName)
- << ".submodule."
- << formatv(sizeof(size_t) == 8 ? "{0:x16}" : "{0:x8}",
- static_cast<size_t>(HC))
- << ".ll";
+ << ".submodule."
+ << formatv(sizeof(size_t) == 8 ? "{0:x16}" : "{0:x8}",
+ static_cast<size_t>(HC))
+ << ".ll";
}
// Extract the requested partiton (plus any necessary aliases) and
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index e25d036cdf1e8c..f86c0d05b5d6b3 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -6,8 +6,10 @@
//===----------------------------------------------------------------------===//
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/ADT/SmallSet.h"
#include <cassert>
#include <optional>
+#include <variant>
using namespace llvm;
@@ -25,8 +27,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 +37,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 +56,8 @@ bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
return !Failed;
}
-std::optional<ReplacementItem>
-formatv_object_base::parseReplacementItem(StringRef Spec) {
+static std::variant<ReplacementItem, StringRef>
+parseReplacementItem(StringRef Spec) {
StringRef RepString = Spec.trim("{}");
// If the replacement sequence does not start with a non-negative integer,
@@ -67,14 +68,12 @@ formatv_object_base::parseReplacementItem(StringRef Spec) {
StringRef Options;
size_t Index = 0;
RepString = RepString.trim();
- if (RepString.consumeInteger(0, Index)) {
- assert(false && "Invalid replacement sequence index!");
- return ReplacementItem{};
- }
+ if (RepString.consumeInteger(0, Index))
+ return "Invalid replacement sequence index!";
RepString = RepString.trim();
if (RepString.consume_front(",")) {
if (!consumeFieldLayout(RepString, Where, Align, Pad))
- assert(false && "Invalid replacement field layout specification!");
+ return "Invalid replacement field layout specification!";
}
RepString = RepString.trim();
if (RepString.consume_front(":")) {
@@ -82,77 +81,110 @@ formatv_object_base::parseReplacementItem(StringRef Spec) {
RepString = StringRef();
}
RepString = RepString.trim();
- if (!RepString.empty()) {
- assert(false && "Unexpected characters found in replacement string!");
- }
-
+ if (!RepString.empty())
+ return "Unexpected character found in replacement string!";
return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
}
-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() != '{') {
- std::size_t BO = Fmt.find_first_of('{');
- return std::make_pair(ReplacementItem{Fmt.substr(0, BO)}, Fmt.substr(BO));
- }
-
- StringRef Braces = Fmt.take_while([](char C) { return C == '{'; });
- // If there is more than one brace, then some of them are escaped. Treat
- // these as replacements.
- if (Braces.size() > 1) {
- size_t NumEscapedBraces = Braces.size() / 2;
- StringRef Middle = Fmt.take_front(NumEscapedBraces);
- StringRef Right = Fmt.drop_front(NumEscapedBraces * 2);
- return std::make_pair(ReplacementItem{Middle}, Right);
- }
- // An unterminated open brace is undefined. Assert to indicate that this is
- // undefined and that we consider it an error. When asserts are disabled,
- // build a replacement item with an error message.
- std::size_t BC = Fmt.find_first_of('}');
- if (BC == StringRef::npos) {
- assert(
- false &&
- "Unterminated brace sequence. Escape with {{ for a literal brace.");
- return std::make_pair(
- ReplacementItem{"Unterminated brace sequence. Escape with {{ for a "
- "literal brace."},
- StringRef());
- }
-
- // Even if there is a closing brace, if there is another open brace before
- // this closing brace, treat this portion as literal, and try again with the
- // next one.
- std::size_t BO2 = Fmt.find_first_of('{', 1);
- if (BO2 < BC)
- return std::make_pair(ReplacementItem{Fmt.substr(0, BO2)},
- Fmt.substr(BO2));
-
- StringRef Spec = Fmt.slice(1, BC);
- StringRef Right = Fmt.substr(BC + 1);
-
- auto RI = parseReplacementItem(Spec);
- if (RI)
- return std::make_pair(*RI, Right);
+static std::variant<std::pair<ReplacementItem, StringRef>, StringRef>
+splitLiteralAndReplacement(StringRef Fmt) {
+ // Everything up until the first brace is a literal.
+ if (Fmt.front() != '{') {
+ std::size_t BO = Fmt.find_first_of('{');
+ return std::make_pair(ReplacementItem(Fmt.substr(0, BO)), Fmt.substr(BO));
+ }
- // If there was an error parsing the replacement item, treat it as an
- // invalid replacement spec, and just continue.
- Fmt = Fmt.drop_front(BC + 1);
+ StringRef Braces = Fmt.take_while([](char C) { return C == '{'; });
+ // If there is more than one brace, then some of them are escaped. Treat
+ // these as replacements.
+ if (Braces.size() > 1) {
+ size_t NumEscapedBraces = Braces.size() / 2;
+ StringRef Middle = Fmt.take_front(NumEscapedBraces);
+ StringRef Right = Fmt.drop_front(NumEscapedBraces * 2);
+ return std::make_pair(ReplacementItem(Middle), Right);
}
- return std::make_pair(ReplacementItem{Fmt}, StringRef());
+ // An unterminated open brace is undefined. Assert to indicate that this is
+ // undefined and that we consider it an error. When asserts are disabled,
+ // build a replacement item with an error message.
+ std::size_t BC = Fmt.find_first_of('}');
+ if (BC == StringRef::npos)
+ return "Unterminated brace sequence. Escape with {{ for a literal brace.";
+
+ // Even if there is a closing brace, if there is another open brace before
+ // this closing brace, treat this portion as literal, and try again with the
+ // next one.
+ std::size_t BO2 = Fmt.find_first_of('{', 1);
+ if (BO2 < BC)
+ return std::make_pair(ReplacementItem{Fmt.substr(0, BO2)}, Fmt.substr(BO2));
+
+ StringRef Spec = Fmt.slice(1, BC);
+ StringRef Right = Fmt.substr(BC + 1);
+
+ auto RI = parseReplacementItem(Spec);
+ if (const StringRef *ErrMsg = std::get_if<1>(&RI))
+ return *ErrMsg;
+
+ return std::make_pair(std::get<0>(RI), Right);
}
-SmallVector<ReplacementItem, 2>
-formatv_object_base::parseFormatString(StringRef Fmt) {
+std::pair<SmallVector<ReplacementItem, 2>, bool>
+formatv_object_base::parseFormatString(raw_ostream &S, const StringRef Fmt,
+ size_t NumArgs, bool Validate) {
SmallVector<ReplacementItem, 2> Replacements;
ReplacementItem I;
- while (!Fmt.empty()) {
- std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
+ size_t NumExpectedArgs = 0;
+
+ // Make a copy for pasring as it updates it.
+ StringRef ParseFmt = Fmt;
+ while (!ParseFmt.empty()) {
+ auto RI = splitLiteralAndReplacement(ParseFmt);
+ if (const StringRef *ErrMsg = std::get_if<1>(&RI)) {
+ // If there was an error parsing the format string, write the error to the
+ // stream, and return false as second member of the pair.
+ errs() << "Invalid format string: " << Fmt << "\n";
+ assert(0 && "Invalid format string");
+ S << *ErrMsg;
+ return {{}, false};
+ }
+ std::tie(I, ParseFmt) = std::get<0>(RI);
if (I.Type != ReplacementType::Empty)
Replacements.push_back(I);
+ if (I.Type == ReplacementType::Format)
+ NumExpectedArgs = std::max(NumExpectedArgs, I.Index + 1);
}
- return Replacements;
+ if (!Validate)
+ return {Replacements, true};
+
+ // Perform additional validation. Verify that the number of arguments matches
+ // the number of replacement indices and that there are no holes in the
+ // replacement indexes.
+ if (NumExpectedArgs != NumArgs) {
+ errs() << "Expected " << NumExpectedArgs << " Args, but got " << NumArgs
+ << " for format string '" << Fmt << "'\n";
+ assert(0 && "Invalid formatv() call");
+ S << "Expected " << NumExpectedArgs << " Args, but got " << NumArgs
+ << " for format string '" << Fmt << "'\n";
+ return {{}, false};
+ }
+
+ SmallSet<size_t, 2> Indices;
+ for (const ReplacementItem &R : Replacements) {
+ if (R.Type != ReplacementType::Format)
+ continue;
+ Indices.insert(R.Index);
+ }
+
+ if (Indices.size() != NumExpectedArgs) {
+ errs() << "Invalid format string: Replacement field indices "
+ "cannot have holes for format string '"
+ << Fmt << "'\n";
+ assert(0 && "Invalid format string");
+ S << "Replacement field indices cannot have holes for format string '"
+ << Fmt << "'\n";
+ return {{}, false};
+ }
+
+ return {Replacements, true};
}
void support::detail::format_adapter::anchor() {}
diff --git a/llvm/tools/llvm-pdbutil/ExplainOutputStyle.cpp b/llvm/tools/llvm-pdbutil/ExplainOutputStyle.cpp
index ce9d659335b30b..200e9037de3cbe 100644
--- a/llvm/tools/llvm-pdbutil/ExplainOutputStyle.cpp
+++ b/llvm/tools/llvm-pdbutil/ExplainOutputStyle.cpp
@@ -139,8 +139,8 @@ bool ExplainOutputStyle::explainPdbBlockStatus() {
FileOffset, File.pdb().getFileSize());
return false;
}
- P.formatLine("Block:Offset = {2:X-}:{1:X-4}.", FileOffset, pdbBlockOffset(),
- pdbBlockIndex());
+ P.formatLine("Block:Offset = {0:X-}:{1:X-4}.", pdbBlockIndex(),
+ pdbBlockOffset());
bool IsFree = File.pdb().getMsfLayout().FreePageMap[pdbBlockIndex()];
P.formatLine("Address is in block {0} ({1}allocated).", pdbBlockIndex(),
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index a78b25c53d7e43..23399babe7337e 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,38 @@ struct NoFormat {};
static_assert(uses_missing_provider<NoFormat>::value, "");
}
+// Helper to parse format string with no validation.
+static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt) {
+ std::string Error;
+ raw_string_ostream ErrorStream(Error);
+ auto [Replacements, IsValid] =
+ formatv_object_base::parseFormatString(ErrorStream, Fmt, 0, false);
+ ErrorStream.flush();
+ assert(IsValid && Error.empty());
+ return Replacements;
+}
+
+#if NDEBUG
+// Helper for parse format string with errors.
+static std::string parseFormatStringError(StringRef Fmt) {
+ std::string Error;
+ raw_string_ostream ErrorStream(Error);
+ auto [...
[truncated]
|
|
Hi all, this is related to https://discourse.llvm.org/t/adding-argument-count-validation-for-formatv/80876/1 I still have formatv() and formatvv() functions in the code, but only a handful instances. So, if this looks ok overall, I will go ahead and rename formatvv() to formatv_unchecked() (or if there are other better suggestions for that version). Also, let me know if it would make sense to split this into 2 changes, one NFC one that fixes formatv() current uses that are invalid, and then the next one that adds validation and changes the handful few that need to be to formatvv(). |
Yeah, it looked like an unrelated failure. Windows CI passed. |
joker-eph
reviewed
Aug 27, 2024
I can check. How do I run them? |
I am wondering if running mlir-tblgen is a better benchmark, given that formatv() is widely used there (as opposed to clang), in case this measurement has to be done manually. |
|
Possible, let's try it there then |
|
I did 2 sets of experiments, but data wise I am inconclusive if this causes a real compile time regression.
Baseline: This is ~3.2% regression in just formatv. The benchmark I added was: #include "benchmark/benchmark.h"
#include "llvm/Support/FormatVariadic.h"
using namespace llvm;
// Benchmark intrinsic lookup from a variety of targets.
static void BM_FormatVariadic(benchmark::State &state) {
for (auto _ : state) {
// Exercise formatv() with several valid replacement options.
formatv("{0}", 1).str();
formatv("{0}{1}", 1, 1).str();
formatv("{0}{1}{2}", 1, 1, 1).str();
formatv("{0}{1}{2}{3}", 1, 1, 1, 1).str();
formatv("{0}{1}{2}{3}{4}", 1, 1, 1, 1, 1).str();
}
}
BENCHMARK(BM_FormatVariadic);
BENCHMARK_MAIN();The compile time data collected from mlir-tblgen runs is quite noisy for individual targets, though the aggregated results seem stable, but I wonder if that means that its not really capturing small compile time delta correctly. As an example: So within the same run, for one target its +12% and for another its -12%. The other line of thinking is that this validation is an aid to developers, so enabling it just in Debug builds may be good enough to catch issues. I am attaching the script and the capture mlir-tblgen commands used in the script below |
Thanks. I'll split out an additional PR for the independent fixes, and then in this one I'll change ENABLE_VALIDATION back to 0 and also remove the |
|
Started #106454 for the independent fixes. |
jurahul
force-pushed
the
formatv_errorcheck
branch
from
d805dcd to
55abaa8
Compare
- Detect formatv() calls where the number of replacement parameters expected after parsing the format string does not match the number provides in the formatv() call. - assert() in debug builds, and fail the formatv() call in release builds by just emitting an error message in the stream.
jurahul
force-pushed
the
formatv_errorcheck
branch
from
55abaa8 to
432c425
Compare
|
The final version is good to go @joker-eph if you want to take another look. Nothing significant changed, just formatvv() is now gone and replaced with formatv(false,...). |
|
LG! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/5504 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/12/builds/4733 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/4140 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/1843 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/63/builds/1273 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/127/builds/667 Here is the relevant piece of the build log for the reference |
Mogball
pushed a commit
that referenced
this pull request
Sep 6, 2024
PR #105745 requires that `formatv` calls have the correct number of arguments. This call to `Logger::info` was incorrect.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/49/builds/431 Here is the relevant piece of the build log for the reference |
github-merge-queue bot
pushed a commit
to carbon-language/carbon-lang
that referenced
this pull request
Sep 9, 2024
llvm/llvm-project#105745 increased validation of formatv requirements, this fixes a couple issues. Note the CommandLine case was untested, and caught separately.
nirvedhmeshram
added a commit
to iree-org/iree
that referenced
this pull request
Sep 12, 2024
Bumps llvm to commit: iree-org/llvm-project@e268afb in branch https://github.com/iree-org/llvm-project/tree/shared/integrates_20240910 Following changes are made: 1. Fix formatv call to pass validation added by llvm/llvm-project#105745 2. API changes in DICompositeTypeAttr::get introduced by llvm/llvm-project#106571 3. Fix API call from llvm/llvm-project#100361 4. Fix chipset comparison in ROCMTarget.cpp There are two cherry-picks from upstream main as they contain fixes we need and one cheery-pick that is yet to land 1. iree-org/llvm-project@0d5d355 2. iree-org/llvm-project@650d852 3. iree-org/llvm-project@e268afb (the upstream PR for this one is llvm/llvm-project#108302 And a revert due to an outstanding torch-mlir issue iree-org/llvm-project@cf22797
josemonsalve2
pushed a commit
to josemonsalve2/iree
that referenced
this pull request
Sep 14, 2024
Bumps llvm to commit: iree-org/llvm-project@e268afb in branch https://github.com/iree-org/llvm-project/tree/shared/integrates_20240910 Following changes are made: 1. Fix formatv call to pass validation added by llvm/llvm-project#105745 2. API changes in DICompositeTypeAttr::get introduced by llvm/llvm-project#106571 3. Fix API call from llvm/llvm-project#100361 4. Fix chipset comparison in ROCMTarget.cpp There are two cherry-picks from upstream main as they contain fixes we need and one cheery-pick that is yet to land 1. iree-org/llvm-project@0d5d355 2. iree-org/llvm-project@650d852 3. iree-org/llvm-project@e268afb (the upstream PR for this one is llvm/llvm-project#108302 And a revert due to an outstanding torch-mlir issue iree-org/llvm-project@cf22797
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/98/builds/388 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change formatv() to validate that the number of arguments passed matches number of
replacement fields in the format string, and that the replacement indices do not contain
holes.
To support cases where this cannot be guaranteed, introduce a formatv() variant that does not
do this validation (called formatvv()).
Fixed existing issues found by this validation.