@@ -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), Adapte
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
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
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
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
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
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/ll
@@ -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 Replac
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-
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
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
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/
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_l
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
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
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_
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
@@ -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:x
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/1
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
Fro
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/listin
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 severa
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
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
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
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
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
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
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
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
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-comm
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
___
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 7ee776230f113
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
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 variabl
@@ -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
@@ -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:
Thes
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
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
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 m
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 opt
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
h
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'
43 matches
Mail list logo
