balazs-benics-sonarsource wrote:
Confirmed crash, https://compiler-explorer.com/z/fzoqP36xq
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
balazs-benics-sonarsource wrote:
> Hello @balazs-benics-sonarsource
>
> The following starts crashing with this patch: `clang --analyze bbi-104578.c`
> It crashes with
Thank you for the heads up @mikaelholmen. I'll switch to it ASAP. I'd expect
the fix later today.
https://github.com/llvm/ll
mikaelholmen wrote:
Hello @balazs-benics-sonarsource
The following starts crashing with this patch:
```clang --analyze bbi-104578.c```
It crashes with
```
clang: ../../clang/lib/StaticAnalyzer/Core/RegionStore.cpp:375:
LimitedRegionBindingsRef LimitedRegionBindingsRef::addBinding(BindingKey, S
https://github.com/balazs-benics-sonarsource closed
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
github-actions[bot] wrote:
@balazs-benics-sonarsource Congratulations on having your first Pull Request
(PR) merged into the LLVM Project!
Your changes will be combined with recent changes from other authors, then
tested by our [build bots](https://lab.llvm.org/buildbot/). If there is a
pro
balazs-benics-sonarsource wrote:
> Anyway, let's just merge this as it is now.
>
> The code is basically OK, I still don't have the brainpower to hold all the
> details in my mind (kudos for the fact that _you_ managed to put this
> together) and if I'll catch some divine inspiration in the fu
https://github.com/NagyDonat edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/NagyDonat approved this pull request.
Anyway, let's just merge this as it is now.
The code is basically OK, I still don't have the brainpower to hold all the
details in my mind and if I'll catch some divine inspiration in the future, I
can still refactor this as a follow-up
NagyDonat wrote:
I tried to write down my rough ideas as a concrete commit, but I don't have
enough mental acuity for it today. I'll try to make another attempt on Monday
-- but if you wish to close this commit, then it's also OK if I do this
refactoring in a separate follow-up commit.
https:
@@ -359,7 +326,80 @@ class RegionBindingsRef : public
llvm::ImmutableMapRefhttps://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
@@ -359,7 +326,80 @@ class RegionBindingsRef : public
llvm::ImmutableMapRefhttps://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
balazs-benics-sonarsource wrote:
> As I thought a bit more about the reorganization that I suggested, I realized
> that it can be summarized as **we should synchronize adding the default
> `Unknown` binding and calling `escapeValue`** -- because they correspond to
> the two end-points of the s
https://github.com/NagyDonat edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/NagyDonat commented:
> I'm sorry if the poor code quality hindered the comprehension.
I wouldn't say "poor code quality" -- the code was difficult to understand, but
mostly due to the inherent complexity of the logic that's being implemented
here.
> In short, I decided to p
@@ -359,7 +326,80 @@ class RegionBindingsRef : public
llvm::ImmutableMapRef`
Perhaps use `Limited` instead of `Bounded` in the name of this class, because
the common linguistical origin of the words "Bounded" and "Binding" makes the
current name a bit hard to parse. (I almost r
https://github.com/NagyDonat edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -176,31 +177,59 @@ class RegionBindingsRef : public
llvm::ImmutableMapRefpush_back(V);
+return *this;
+ }
+ RegionBindingsRef escapeValues(nonloc::CompoundVal::iterator Begin,
+ nonloc::CompoundVal::iterator End) const {
+for (SVal V :
balazs-benics-sonarsource wrote:
Thanks for your long review. I'm sorry if the poor code quality hindered the
comprehension.
My goal was to minimize the diff for easier review, but I admit that I should
have attached some considerations as to why I implemented it this way, and also
how differe
https://github.com/balazs-benics-sonarsource edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/balazs-benics-sonarsource edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -483,6 +483,14 @@ ANALYZER_OPTION(
"behavior, set the option to 0.",
5)
+ANALYZER_OPTION(
+unsigned, RegionStoreMaxBindingFanOut, "region-store-max-binding-fanout",
+"This option limits how many sub-bindings a single binding operation can "
+"scatter int
@@ -2782,6 +2865,8 @@ RegionBindingsRef
RegionStoreManager::bindStruct(RegionBindingsConstRef B,
if (VI == VE)
break;
+ if (NewB.hasExhaustedBindingLimit())
+return NewB.escapeValues(VI, VE);
balazs-benics-sonarsource wrote:
I've r
@@ -176,31 +177,59 @@ class RegionBindingsRef : public
llvm::ImmutableMapRefpush_back(V);
+return *this;
+ }
+ RegionBindingsRef escapeValues(nonloc::CompoundVal::iterator Begin,
+ nonloc::CompoundVal::iterator End) const {
+for (SVal V :
https://github.com/balazs-benics-sonarsource updated
https://github.com/llvm/llvm-project/pull/127602
From f5cd6b22fb83c0bfb584717cde6899cd65fc1274 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Wed, 5 Feb 2025 17:13:34 +0100
Subject: [PATCH 1/7] [analyzer] Limit Store by region-store-bindi
NagyDonat wrote:
As I thought a bit more about the reorganization that I suggested, I realized
that it can be summarized as **we should synchronize adding the default
`Unknown` binding and calling `escapeValue`** -- because they correspond to the
two end-points of the same "_this_ value is sto
https://github.com/NagyDonat edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/NagyDonat edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/NagyDonat commented:
I'm really happy that you managed to track down and fix these slow corner
cases, and overall I like the code changes, but I there were a few places where
the code was difficult to understand for me (although this may be also related
to the fact that I di
@@ -176,31 +177,59 @@ class RegionBindingsRef : public
llvm::ImmutableMapRefpush_back(V);
+return *this;
+ }
+ RegionBindingsRef escapeValues(nonloc::CompoundVal::iterator Begin,
+ nonloc::CompoundVal::iterator End) const {
+for (SVal V :
@@ -2782,6 +2865,8 @@ RegionBindingsRef
RegionStoreManager::bindStruct(RegionBindingsConstRef B,
if (VI == VE)
break;
+ if (NewB.hasExhaustedBindingLimit())
+return NewB.escapeValues(VI, VE);
NagyDonat wrote:
This is confusing to r
@@ -483,6 +483,14 @@ ANALYZER_OPTION(
"behavior, set the option to 0.",
5)
+ANALYZER_OPTION(
+unsigned, RegionStoreMaxBindingFanOut, "region-store-max-binding-fanout",
+"This option limits how many sub-bindings a single binding operation can "
+"scatter int
https://github.com/NagyDonat edited
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
steakhal wrote:
> (I'm writing a review right now, please wait a bit -- at most a few hours --
> before merging.)
No worries. I wouldnt have merged tgis without your approval too.
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits maili
NagyDonat wrote:
(I'm writing a review right now, please wait a bit -- a few hours -- before
merging.)
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/list
https://github.com/Xazax-hun approved this pull request.
Makes sense giving up on large arrays.
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cf
balazs-benics-sonarsource wrote:
@Flandini @necto
https://github.com/llvm/llvm-project/pull/127602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
github-actions[bot] wrote:
Thank you for submitting a Pull Request (PR) to the LLVM Project!
This PR will be automatically labeled and the relevant teams will be notified.
If you wish to, you can add reviewers by using the "Reviewers" section on this
page.
If this is not working for you, it
llvmbot wrote:
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Balázs Benics (balazs-benics-sonarsource)
Changes
In our test pool, the max entry point RT was improved by this change: 1'181
seconds (~19.7 minutes) -> 94 seconds (1.6 minutes)
BTW, the 1.6 minutes is still really bad.
https://github.com/balazs-benics-sonarsource created
https://github.com/llvm/llvm-project/pull/127602
In our test pool, the max entry point RT was improved by this change: 1'181
seconds (~19.7 minutes) -> 94 seconds (1.6 minutes)
BTW, the 1.6 minutes is still really bad. But a few orders of ma
39 matches
Mail list logo
