[Lldb-commits] [clang] [clang-tools-extra] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

@@ -74,16 +75,31 @@ class formatv_object_base {
   static std::pair
   splitLiteralAndReplacement(StringRef Fmt);
 
-  formatv_object_base(StringRef Fmt,
+  formatv_object_base(StringRef Fmt, bool ValidateNumArgs,
   ArrayRef Adapters)
-  : Fmt(Fmt), Adapters(Adapters) {}
+  : Fmt(Fmt), ValidateNumArgs(ValidateNumArgs), 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, NumExpectedParams] = parseFormatString(Fmt);
+// Fail formatv() call if the number of replacement parameters provided
+// does not match the expected number after parsing the format string.
+// Assert in debug builds.
+if (ValidateNumArgs && NumExpectedParams != Adapters.size()) {
+  errs() << "Invalid format() in formatv: " << Fmt << "\n";
+  assert(0 &&
+ "Mismatch between replacement parameters expected and provided");
+
+  S << "formatv() error: " << NumExpectedParams
+<< " replacement parameters expected, but " << Adapters.size()
+<< " provided";

jurahul wrote:

Thanks. This is not ready for review yet. I wanted to run CI locally as I 
suspect I need to fix more issues before it runs clean. But yes, we can tweak 
what gets dumped into the stream in error case for release builds.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul edited 
https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul edited 
https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul updated 
https://github.com/llvm/llvm-project/pull/105745

>From 9ae8a0361b0d26cf29cc4547658baec0c2654c89 Mon Sep 17 00:00:00 2001
From: Rahul Joshi 
Date: Thu, 22 Aug 2024 08:47:02 -0700
Subject: [PATCH] [Support] Detect invalid formatv() calls

- 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.
---
 .../Checkers/CheckPlacementNew.cpp|   2 +-
 .../Checkers/StdLibraryFunctionsChecker.cpp   |   2 +-
 .../Plugins/SymbolFile/DWARF/DWARFUnit.cpp|   2 +-
 llvm/include/llvm/Support/FormatVariadic.h|  54 --
 llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp|   3 +-
 .../Orc/CompileOnDemandLayer.cpp  |   8 +-
 llvm/lib/Support/FormatVariadic.cpp   | 172 +++---
 .../tools/llvm-pdbutil/ExplainOutputStyle.cpp |   4 +-
 llvm/unittests/Support/FormatVariadicTest.cpp | 115 +---
 llvm/utils/TableGen/IntrinsicEmitter.cpp  |   4 +-
 .../mlir-linalg-ods-yaml-gen.cpp  |   4 +-
 .../tools/mlir-tblgen/LLVMIRConversionGen.cpp |   2 +-
 mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp   |  13 +-
 mlir/tools/mlir-tblgen/OpFormatGen.cpp|  14 +-
 mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp  |   3 +-
 15 files changed, 253 insertions(+), 149 deletions(-)

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 Adapters;
 
-  static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
- size_t &Align, char &Pad);
-
-  static std::pair
-  splitLiteralAndReplacement(StringRef Fmt);
-
-  formatv_object_base(StringRef Fmt,
+  formatv_object_base(StringRef Fmt, bool Validate,
   ArrayRef 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;
  

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul ready_for_review 
https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

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().

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

> CI failed FYI.

Yeah, it looked like an unrelated failure. Windows CI passed.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

@@ -67,92 +68,123 @@ 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(":")) {
 Options = RepString.trim();
 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
-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, 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
-format

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

> compile-time tests with clang for example

I can check. How do I run them?

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul edited 
https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul edited 
https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

> > compile-time tests with clang for example
> 
> 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.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

I did 2 sets of experiments, but data wise I am inconclusive if this causes a 
real compile time regression.

1. Build MLIR verbose and capture all mlir-gen command lines to a file:
   ninja -C build check-mlir --verbose | tee build_log.txt
   grep "NATIVE/bin/mlir-tblgen " build_log.txt  | cut -d ' ' -f 2- > 
mlir-tablegen-commands.txt

2. Build both baseline and new versions of LLVM/MLIR in 2 different paths 
"upstream_clean" and "upstream_llvm"

3. Use attached script to run these captured commands with --time-phases and 
measure total time.

4. Establish baseline variance, by running the script comparing baseline to 
itself.

Total time 4.2302 4.2573
 0.6406

   So baseline variance is 0.6%, with each command running 20 times. Note that 
for individual targets,
   the variance is quite high for some of them, upto 100%.

5. Establish "new" variance, by running script to compare new to itself
  Total time 4.2829 4.2531 
-0.6958
  Again, 0.6% variance.

6. Run baseline against new:
  Total time 4.1745 4.2864 
2.6806
  So this seems to give 2.6% regression. However, the individual data is quite 
noisy. For example, for individual samples,
  the variance can be quite high, upto 100%.

7. Add a FormatVariadic benchmark to test format() with 1-5 substitutions 
(which covers the common usage in LLVM), and run baseline and new:
   ./build/benchmarks/FormatVariadic --benchmark_repetitions=20

Baseline:
BM_FormatVariadic_mean 1063 ns 1063 ns   20
New:
BM_FormatVariadic_mean 1097 ns 1097 ns   20

This is ~3.2% regression in just formatv.  

The benchmark I added was:

```C++
#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:

```
lir/Dialect/MemRef/IR/MemRefOps.cpp.inc  0.0106 0.0119 12.2642%
mlir/include/mlir/IR/BuiltinOps.cpp.inc  0.0048 0.0042 -12.5000%
```

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 mlit-tblgen commands used in the script 
below

[mlir-tablegen-commands.txt](https://github.com/user-attachments/files/16770614/mlir-tablegen-commands.txt)
[ct_formatv.txt](https://github.com/user-attachments/files/16770618/ct_formatv.txt)
 

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Yeah. For the unbalanced {{ I actually committed a change to fail in
release builds as well by printing an error message as formatv() output.
The only issue with debug only is that many folks build tablegen in release
mode, so those bad uses may not get flagged. But as long as the CI debug
builds use a debug build of tablegen, that would be ok. Let me work on
making this debug only.

On Tue, Aug 27, 2024 at 5:20 PM Mehdi Amini ***@***.***>
wrote:

> Having this enabled guarded by NDEBUG seems like a good option. There is
> already some things already like this isn't there? I kind of remember that
> using imbalanced { / } pair of something like that was checked only in
> assert builds?
>
> —
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>


https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul updated 
https://github.com/llvm/llvm-project/pull/105745

>From d805dcdc44d22bb21d086b186cc7f644323f68ef Mon Sep 17 00:00:00 2001
From: Rahul Joshi 
Date: Thu, 22 Aug 2024 08:47:02 -0700
Subject: [PATCH] [Support] Detect invalid formatv() calls

- 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.
---
 .../Checkers/CheckPlacementNew.cpp|  2 +-
 .../Checkers/StdLibraryFunctionsChecker.cpp   |  2 +-
 .../Plugins/SymbolFile/DWARF/DWARFUnit.cpp|  2 +-
 llvm/benchmarks/CMakeLists.txt|  1 +
 llvm/benchmarks/FormatVariadicBM.cpp  | 53 
 llvm/include/llvm/Support/FormatVariadic.h| 44 ++
 llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp|  3 +-
 llvm/lib/Support/FormatVariadic.cpp   | 84 ---
 .../tools/llvm-pdbutil/ExplainOutputStyle.cpp |  4 +-
 llvm/unittests/Support/FormatVariadicTest.cpp | 68 +--
 llvm/utils/TableGen/IntrinsicEmitter.cpp  |  4 +-
 .../mlir-linalg-ods-yaml-gen.cpp  |  4 +-
 .../tools/mlir-tblgen/LLVMIRConversionGen.cpp |  2 +-
 mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp   | 13 ++-
 mlir/tools/mlir-tblgen/OpFormatGen.cpp| 14 ++--
 mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp  |  3 +-
 16 files changed, 218 insertions(+), 85 deletions(-)
 create mode 100644 llvm/benchmarks/FormatVariadicBM.cpp

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 d1d91134b6237c..0eb882b0e7d4f5 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/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 00..e9916344f9bb67
--- /dev/null
+++ b/llvm/benchmarks/FormatVariadicBM.cpp
@@ -0,0 +1,53 @@
+#include "benchmark/benchmark.h"
+#include "llvm/Support/FormatVariadic.h"
+#include 
+#include 
+#include 
+
+using namespace llvm;
+using namespace std;
+
+// Generate a list of format strings that have `NumReplacements` replacements.
+static vector getFormatStrings(int NumReplacements) {
+  vector Components;
+  for (int I = 0; I < NumReplacements; I++)
+Components.push_back("{" + to_string(I) + "}");
+  // I

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

I have uploaded a new version where the validation is enabled only in debug 
builds, and also added a benchmark for formatv(). On my machine, the benchmark 
results are as follows:

```
Old:
BM_FormatVariadic_mean  3427456 ns  3427426 ns   10

New
BM_FormatVariadic_mean  3360522 ns  3360236 ns   10

New with validation enabled in release builds:
BM_FormatVariadic_mean  3421434 ns  3421426 ns   10
```

So, the new mean is less than the old (with or without validation enabled). My 
takeaway is that the change in time is < measurement noise. In addition, since 
validation is now disabled in release builds, its relatively safe compile time 
perf wise.

I am currently running the CI with ENABLE_VALIDATION, buy @joker-eph can you 
start looking as well? Once the overall change is ok, I still need feedback 
around formatvv(). Given the new setup formatv(false, Fmt, ...) works, and 
since we just have 3 users of formatv() we could just switch to that and not 
invent a new name.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Looks like the CI builds are release but with LLVM_ENABLE_ASSERTION=ON, so this 
validation code will be enabled and potential bad usage will be flagged as well 
going forward as long as it's exercised during the build or test.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

@@ -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,

jurahul wrote:

Yeah, that was one of my plans, so split out independent fixes as a separate 
patch before.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

> LGTM

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 
`formatvv()` and use `formatv(false` instead so we just have one function name.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [NFC] Fix formatv() usage in preparation of validation (PR #106454)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul created 
https://github.com/llvm/llvm-project/pull/106454

Fix several uses of formatv() that would be flagged as invalid by an upcoming 
change that will add additional validation to formatv().

>From b97456af86580c7f2d4473fbc13d3e345f071486 Mon Sep 17 00:00:00 2001
From: Rahul Joshi 
Date: Wed, 28 Aug 2024 13:55:58 -0700
Subject: [PATCH] [NFC] Fix formatv() usage in preparation of validation

Fix several uses of formatv() that would be flagged as invalid by an upcoming
change that will add additional validation to formatv().
---
 .../StaticAnalyzer/Checkers/CheckPlacementNew.cpp   |  2 +-
 lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp  |  2 +-
 llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp  |  3 +--
 llvm/tools/llvm-pdbutil/ExplainOutputStyle.cpp  |  4 ++--
 llvm/utils/TableGen/IntrinsicEmitter.cpp|  4 ++--
 .../mlir-linalg-ods-yaml-gen.cpp|  4 ++--
 mlir/tools/mlir-tblgen/LLVMIRConversionGen.cpp  |  2 +-
 mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | 13 ++---
 mlir/tools/mlir-tblgen/OpFormatGen.cpp  | 10 --
 mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp|  3 +--
 10 files changed, 21 insertions(+), 26 deletions(-)

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/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
index d1d91134b6237c..0eb882b0e7d4f5 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/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/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/utils/TableGen/IntrinsicEmitter.cpp 
b/llvm/utils/TableGen/IntrinsicEmitter.cpp
index 4c211cdca84c5f..af8ff889918428 100644
--- a/llvm/utils/TableGen/IntrinsicEmitter.cpp
+++ b/llvm/utils/TableGen/IntrinsicEmitter.cpp
@@ -735,8 +735,8 @@ Intrinsic::getIntrinsicFor{1}Builtin(StringRef TargetPrefix,
 const auto &[Map, CommonPrefix] = Entry;
 if (TargetPrefix.empty())
   continue;
-OS << formatv(R"({{"{0}", {0}Names, "{2}"},)", TargetPrefix,
-  TargetPrefix, CommonPrefix)
+OS << formatv(R"({{"{0}", {0}Names, "{1}"},)", TargetPrefix,
+  CommonPrefix)
<< "\n";
   }
   OS << "  };\n";
diff --git a/mlir/tools/mlir-linalg-ods-gen/mlir-l

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [Support] Validate number of arguments passed to formatv() (PR #105745)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Started https://github.com/llvm/llvm-project/pull/106454 for the independent 
fixes.

https://github.com/llvm/llvm-project/pull/105745
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [NFC] Fix formatv() usage in preparation of validation (PR #106454)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul updated 
https://github.com/llvm/llvm-project/pull/106454

>From 581c47377f77a9d2819a5997b1c0a3c23b2c3346 Mon Sep 17 00:00:00 2001
From: Rahul Joshi 
Date: Wed, 28 Aug 2024 13:55:58 -0700
Subject: [PATCH] [NFC] Fix formatv() usage in preparation of validation

Fix several uses of formatv() that would be flagged as invalid by an upcoming
change that will add additional validation to formatv().
---
 .../StaticAnalyzer/Checkers/CheckPlacementNew.cpp   |  2 +-
 lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp  |  2 +-
 llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp  |  3 +--
 llvm/tools/llvm-pdbutil/ExplainOutputStyle.cpp  |  4 ++--
 llvm/utils/TableGen/IntrinsicEmitter.cpp|  4 ++--
 .../mlir-linalg-ods-yaml-gen.cpp|  4 ++--
 mlir/tools/mlir-tblgen/LLVMIRConversionGen.cpp  |  2 +-
 mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | 13 ++---
 mlir/tools/mlir-tblgen/OpFormatGen.cpp  |  6 ++
 mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp|  3 +--
 10 files changed, 19 insertions(+), 24 deletions(-)

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/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
index d1d91134b6237c..0eb882b0e7d4f5 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/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/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/utils/TableGen/IntrinsicEmitter.cpp 
b/llvm/utils/TableGen/IntrinsicEmitter.cpp
index 4c211cdca84c5f..af8ff889918428 100644
--- a/llvm/utils/TableGen/IntrinsicEmitter.cpp
+++ b/llvm/utils/TableGen/IntrinsicEmitter.cpp
@@ -735,8 +735,8 @@ Intrinsic::getIntrinsicFor{1}Builtin(StringRef TargetPrefix,
 const auto &[Map, CommonPrefix] = Entry;
 if (TargetPrefix.empty())
   continue;
-OS << formatv(R"({{"{0}", {0}Names, "{2}"},)", TargetPrefix,
-  TargetPrefix, CommonPrefix)
+OS << formatv(R"({{"{0}", {0}Names, "{1}"},)", TargetPrefix,
+  CommonPrefix)
<< "\n";
   }
   OS << "  };\n";
diff --git a/mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-yaml-gen.cpp 
b/mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-yaml-gen.cpp
index 7311cdd39d0755..a00f12661f7120 100644
--- a/mlir/

[Lldb-commits] [clang] [lldb] [llvm] [mlir] [NFC] Fix formatv() usage in preparation of validation (PR #106454)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul closed 
https://github.com/llvm/llvm-project/pull/106454
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper (PR #107536)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul ready_for_review 
https://github.com/llvm/llvm-project/pull/107536
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper (PR #107536)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Yes, the motivation is const correctness. Please see the discourse thread here: 
https://discourse.llvm.org/t/psa-planned-changes-to-tablegen-getallderiveddefinitions-api-potential-downstream-breakages/81089

https://github.com/llvm/llvm-project/pull/107536
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper (PR #107536)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul edited 
https://github.com/llvm/llvm-project/pull/107536
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB][TableGen] Migrate lldb-tblgen to use const RecordKeeper (PR #107536)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Done.

https://github.com/llvm/llvm-project/pull/107536
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[Rahul Joshi via lldb-commits]

jurahul wrote:

@JDevlieghere or @dwblaikie one last ping to comment if consistency of file 
names is a strong enough motivation. If not, I will drop this.

https://github.com/llvm/llvm-project/pull/110692
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Sounds good. thanks.

https://github.com/llvm/llvm-project/pull/110692
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul closed 
https://github.com/llvm/llvm-project/pull/110692
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[Rahul Joshi via lldb-commits]

jurahul wrote:

@JDevlieghere @cyndyishida @topperc gentle ping. It's mostly mechanical.

https://github.com/llvm/llvm-project/pull/110692
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[Rahul Joshi via lldb-commits]

jurahul wrote:

more of a hypothetical concern at this point. It's also making file names under 
Option consistent, in the sense that we have `Option.h` and `Option.cpp` but 
`OptTable.h` `OptSpecifier.h` and `OptTable.cpp` 

https://github.com/llvm/llvm-project/pull/110692
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul created 
https://github.com/llvm/llvm-project/pull/110692

Rename option parsing related files from OptXYZ -> OptionXYZ, since Opt could 
be confused with optimization and Opt as a short hand for Option is not really 
that smaller to warrant its use.

>From 7ee776230f113853ac39a03d13c13a6ca9480bc6 Mon Sep 17 00:00:00 2001
From: Rahul Joshi 
Date: Tue, 1 Oct 2024 09:03:33 -0700
Subject: [PATCH] [NFC] Rename Option parsing related files from OptXYZ ->
 OptionXYZ

Rename option parsing related files from OptXYZ -> OptionXYZ, since
Opt could be confused with optimization and Opt as a short hand
for Option is not really that smaller to warrant its use.
---
 clang-tools-extra/modularize/Modularize.cpp   |  2 +-
 clang-tools-extra/pp-trace/PPTrace.cpp|  2 +-
 clang/docs/InternalsManual.rst|  2 +-
 clang/include/clang/Driver/OptionUtils.h  |  2 +-
 clang/include/clang/Driver/Options.h  |  2 +-
 clang/include/clang/Driver/Options.td |  2 +-
 clang/lib/Driver/Compilation.cpp  |  2 +-
 clang/lib/Driver/Driver.cpp   |  4 +-
 clang/lib/Driver/DriverOptions.cpp|  2 +-
 clang/lib/Driver/ToolChain.cpp|  2 +-
 clang/lib/Frontend/CompilerInvocation.cpp |  4 +-
 .../ExecuteCompilerInvocation.cpp |  2 +-
 .../InterpolatingCompilationDatabase.cpp  |  2 +-
 clang/lib/Tooling/Tooling.cpp |  2 +-
 clang/tools/clang-check/ClangCheck.cpp|  2 +-
 .../tools/clang-installapi/InstallAPIOpts.td  |  2 +-
 .../ClangLinkerWrapper.cpp|  2 +-
 .../clang-linker-wrapper/LinkerWrapperOpts.td |  2 +-
 .../ClangNVLinkWrapper.cpp|  2 +-
 .../tools/clang-nvlink-wrapper/NVLinkOpts.td  |  2 +-
 clang/tools/clang-scan-deps/Opts.td   |  2 +-
 clang/tools/driver/cc1_main.cpp   |  2 +-
 clang/tools/driver/cc1as_main.cpp |  2 +-
 clang/tools/driver/driver.cpp |  2 +-
 flang/lib/Frontend/CompilerInvocation.cpp |  2 +-
 .../ExecuteCompilerInvocation.cpp |  2 +-
 flang/tools/flang-driver/fc1_main.cpp |  2 +-
 lld/COFF/Options.td   |  2 +-
 lld/ELF/Options.td|  2 +-
 lld/MachO/Driver.h|  2 +-
 lld/MachO/Options.td  |  2 +-
 lld/MinGW/Options.td  |  2 +-
 lld/wasm/Options.td   |  2 +-
 lldb/tools/driver/Options.td  |  2 +-
 lldb/tools/lldb-dap/Options.td|  2 +-
 lldb/tools/lldb-dap/lldb-dap.cpp  |  2 +-
 lldb/tools/lldb-server/LLGSOptions.td |  2 +-
 lldb/tools/lldb-server/lldb-gdbserver.cpp |  2 +-
 llvm/include/llvm/Option/ArgList.h|  4 +-
 llvm/include/llvm/Option/Option.h |  4 +-
 .../Option/{OptParser.td => OptionParser.td}  |  8 +--
 .../{OptSpecifier.h => OptionSpecifier.h} |  8 +--
 .../llvm/Option/{OptTable.h => OptionTable.h} | 17 +++--
 .../ExecutionEngine/JITLink/COFFOptions.td|  2 +-
 llvm/lib/Option/ArgList.cpp   |  6 +-
 llvm/lib/Option/CMakeLists.txt|  2 +-
 llvm/lib/Option/Option.cpp|  2 +-
 .../Option/{OptTable.cpp => OptionTable.cpp}  | 65 ++-
 .../llvm-dlltool/DlltoolDriver.cpp|  2 +-
 llvm/lib/ToolDrivers/llvm-dlltool/Options.td  |  2 +-
 llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp   |  2 +-
 llvm/lib/ToolDrivers/llvm-lib/Options.td  |  2 +-
 llvm/tools/dsymutil/Options.td|  2 +-
 llvm/tools/llvm-cgdata/Opts.td|  2 +-
 llvm/tools/llvm-cvtres/Opts.td|  2 +-
 llvm/tools/llvm-cxxfilt/Opts.td   |  2 +-
 llvm/tools/llvm-debuginfod-find/Opts.td   |  2 +-
 llvm/tools/llvm-debuginfod/Opts.td|  2 +-
 llvm/tools/llvm-dwarfutil/Options.td  |  2 +-
 llvm/tools/llvm-dwp/Opts.td   |  2 +-
 llvm/tools/llvm-gsymutil/Opts.td  |  2 +-
 llvm/tools/llvm-ifs/Opts.td   |  2 +-
 llvm/tools/llvm-libtool-darwin/Opts.td|  2 +-
 llvm/tools/llvm-lipo/LipoOpts.td  |  2 +-
 llvm/tools/llvm-ml/Opts.td|  2 +-
 llvm/tools/llvm-mt/Opts.td|  2 +-
 llvm/tools/llvm-nm/Opts.td|  2 +-
 llvm/tools/llvm-objcopy/BitcodeStripOpts.td   |  2 +-
 llvm/tools/llvm-objcopy/CommonOpts.td |  2 +-
 .../tools/llvm-objcopy/InstallNameToolOpts.td |  2 +-
 llvm/tools/llvm-objdump/ObjdumpOptID.h|  2 +-
 llvm/tools/llvm-objdump/ObjdumpOpts.td|  2 +-
 llvm/tools/llvm-objdump/OtoolOpts.td  |  2 +-
 llvm/tools/llvm-rc/Opts.td|  2 +-
 llvm/tools/llvm-rc/WindresOpts.td |  2 +-
 llvm/tools/llvm-rc/llvm-rc.cpp|  2 +-
 llvm/tools/llvm-readobj/Opts.td   |  2 +-
 llvm/tools/llvm-readtapi/TapiOpts.td  |  2 +-

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[Rahul Joshi via lldb-commits]

https://github.com/jurahul ready_for_review 
https://github.com/llvm/llvm-project/pull/110692
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Even if a variable is defined in the main file, it could conflict with
another one in a library being linked.

In any case, my understanding is that this is a common C++ practice,
thought I do not see it spelled out explicitly in the coding standards doc:
file scoped global variables that are not intended to be referenced outside
be marked static (or be in an anonymous namespace, but LLVM’s coding
standards disallow that). It not solving any problem, its just code cleanup.

On Sat, Feb 8, 2025 at 4:10 AM Mehdi Amini ***@***.***> wrote:

> The linked bug seems to explain it, I think?
>
> I don't actually see a description of what is the problem to solve really.
>
> It seems to be the usual "if something isn't/doesn't need to be declared
> in a header, then it should be file-local static, so as to not conflict
> with other local names in other files"?
>
> All of the files are main() file, not library files. Not clear to me what
> they would conflict with?
>
> Right, though I think @joker-eph  comment
> is that for lot of the MLIR changes, these option variables are function
> local and they need not be changed to static. This issue is only applicable
> to file scoped variables.
>
> That too :)
>
> —
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>


https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

@@ -20,7 +20,7 @@ using namespace mlir::lsp;
 
 LogicalResult mlir::MlirLspServerMain(int argc, char **argv,
   DialectRegistry ®istry) {
-  llvm::cl::opt inputStyle{
+  static llvm::cl::opt inputStyle{

jurahul wrote:

These are function local variables and need not be changed to static.

https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

@@ -119,24 +119,24 @@ int main(int argc, char **argv) {
   // options as static variables.. some of which overlap with our options.
   llvm::cl::ResetCommandLineParser();
 
-  llvm::cl::opt inputFilename(
+  static llvm::cl::opt inputFilename(

jurahul wrote:

These are function local variables and need not be changed to static.

https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

jurahul wrote:

> > I don't quite follow the motivation for this, can you expand on this in the 
> > description please? Right now this seems unnecessary for the changes I can 
> > see in MLIR for example.
> 
> The linked bug seems to explain it, I think? It seems to be the usual "if 
> something isn't/doesn't need to be declared in a header, then it should be 
> file-local static, so as to not conflict with other local names in other 
> files"?

Right, though I think @joker-eph comment is that for lot of the MLIR changes, 
these option variables are function local and they need not be changed to 
static.

https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Also, the commit description should be something like: "[NFC] Make file-local 
cl::opt global variables static" or something like that

https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

jurahul wrote:

I understand this is mostly mechanical changes, but wondering if review wise it 
will help if its split up into 4-5 PRs. For example, bolt, clang, flag, llvm, 
mlir etc.

https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

jurahul wrote:

Yeah, that’s why the description said most but not all. We should get link
failures if one of these is made static.

On Fri, Feb 7, 2025 at 7:17 AM Joseph Huber ***@***.***>
wrote:

> ***@***. commented on this pull request.
>
> This should definitely be split up. Also some options are referenced in
> multiple places, i.e.
>
> // foo.h
> extern cl::opt <...>
>
> // foo.cpp
> cl::opt <...>
> use opt
>
> //bar.cpp
> use opt.
>
> —
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>


https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

jurahul wrote:

@arsenm did you intent to approve it without splitting? Is this trivial enough 
to not need splitting as long as CI checks pass?

https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [clang-tools-extra] [flang] [lldb] [llvm] [mlir] [polly] Add static to command line option (cl::opt) (PR #126243)

[Rahul Joshi via lldb-commits]

jurahul wrote:

You can check the CI logs for the exact command line it uses and replicate
it locally.

On Fri, Feb 7, 2025 at 7:46 AM chrisPyr ***@***.***> wrote:

> OK, I'll do it.
> Except for checking it on CI, is there any other method I can test
> locally? E.g. what options should I add
> I've tried compile with all project enabled, I thought by that way all
> changes were already being tested.
> However, the CI failed, so it seems what I thought was wrong.
>
> —
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>


https://github.com/llvm/llvm-project/pull/126243
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits