https://github.com/steakhal edited
https://github.com/llvm/llvm-project/pull/123003
___
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.
LGTM, thanks for the improvements :)
In the follow-up commits let's pay attention to the `ArrayBound` lower bound
check where the code says that _"A symbolic region in unknown space represents
an unknown pointer that may point into the m
https://github.com/Xazax-hun approved this pull request.
I am fine going ahead with this.
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
steakhal wrote:
Ping. I wonder if there is anything missing for this to be merged.
@NagyDonat
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
steakhal wrote:
I've pushed a couple of commits to sync this PR with what I had in mind.
I hope you don't mind @Flandini.
Please anyone do a thorough review.
---
I've considered uplifiting a couple of the `VarRegion::getStackFrame()`, and
similar `getStackFrame()` functions, because internall
https://github.com/steakhal edited
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -948,8 +948,8 @@ SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state,
const MemRegion *LeftBase = LeftMR->getBaseRegion();
const MemRegion *RightBase = RightMR->getBaseRegion();
-const MemSpaceRegion *LeftMS = LeftBase->getMemorySpace();
-const MemSpac
@@ -119,7 +120,26 @@ class MemRegion : public llvm::FoldingSetNode {
virtual MemRegionManager &getMemRegionManager() const = 0;
- LLVM_ATTRIBUTE_RETURNS_NONNULL const MemSpaceRegion *getMemorySpace() const;
+ /// Deprecated. Gets the 'raw' memory space of a memory region'
https://github.com/NagyDonat commented:
Thanks for the updates!
I added some inline remarks, but overall I'm satisfied with the state of this
PR and I hope that it can be merged soon.
However, I became a bit uncertain about the plans for follow-up changes,
because it seems that there are seve
https://github.com/NagyDonat edited
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -1544,7 +1544,7 @@ SVal
RegionStoreManager::getBinding(RegionBindingsConstRef B, Loc L, QualType T)
// The location does not have a bound value. This means that it has
// the value it had upon its creation and/or entry to the analyzed
// function/method. These are e
@@ -353,7 +352,7 @@ static std::string getRegionName(const SubRegion *Region) {
return "the memory returned by 'alloca'";
if (isa(Region) &&
- isa(Region->getMemorySpace()))
+ isa(Region->getRawMemorySpace()))
NagyDonat wrote:
I think this sho
https://github.com/steakhal edited
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -119,7 +120,26 @@ class MemRegion : public llvm::FoldingSetNode {
virtual MemRegionManager &getMemRegionManager() const = 0;
- LLVM_ATTRIBUTE_RETURNS_NONNULL const MemSpaceRegion *getMemorySpace() const;
+ /// Deprecated. Gets the 'raw' memory space of a memory region'
Flandini wrote:
Made the above changes. I moved it all into `MemRegion` and changed the names
of and added deprecation comments to the old `MemRegion` instance methods that
queried mem space info.
https://github.com/llvm/llvm-project/pull/123003
__
@@ -119,7 +120,26 @@ class MemRegion : public llvm::FoldingSetNode {
virtual MemRegionManager &getMemRegionManager() const = 0;
- LLVM_ATTRIBUTE_RETURNS_NONNULL const MemSpaceRegion *getMemorySpace() const;
+ /// Deprecated. Gets the 'raw' memory space of a memory region'
NagyDonat wrote:
> > > Have you considered changing `MemRegion::getMemorySpace()` into
> > > `MemRegion::getMemorySpace(ProgramStateRef)`?
> >
> >
> > I thought about this, but I decided against it since I am thinking that
> > MemSpaces will eventually be their own separate thing, not part of
steakhal wrote:
> Benchmarking is taking a while for me, and I can only really run benchmarks
> with low run-time variance overnight. I will post more over time as I get
> more benchmark projects added.
>
> Here is the result from benchmarking one C project, libsodium, so far. These
> results
Flandini wrote:
Benchmarking is taking a while for me, and I can only really run benchmarks
with low run-time variance overnight. I will post more over time as I get more
benchmark projects added.
Here is the result from benchmarking one C project, libsodium, so far. These
results are for 13
@@ -437,7 +437,7 @@ bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
// tell if anything still refers to this region. Unlike SymbolicRegions,
// AllocaRegions don't have associated symbols, though, so we don't actually
// have a way to track their liveness.
- retur
steakhal wrote:
> Working on some benchmarking. Posted some problems in the discord channel,
> but working through it now, and benchmarking with the CodeChecker tool using
> only clangsa. Is there an already established set of benchmarks y'all use
> somewhere? IIRC, there was some github repo
steakhal wrote:
> > Have you considered changing `MemRegion::getMemorySpace()` into
> > `MemRegion::getMemorySpace(ProgramStateRef)`?
>
> I thought about this, but I decided against it since I am thinking that
> MemSpaces will eventually be their own separate thing, not part of the
> MemRegio
@@ -437,7 +437,7 @@ bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
// tell if anything still refers to this region. Unlike SymbolicRegions,
// AllocaRegions don't have associated symbols, though, so we don't actually
// have a way to track their liveness.
- retur
https://github.com/Flandini edited
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -437,7 +437,7 @@ bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
// tell if anything still refers to this region. Unlike SymbolicRegions,
// AllocaRegions don't have associated symbols, though, so we don't actually
// have a way to track their liveness.
- retur
Flandini wrote:
Working on some benchmarking. Posted some problems in the discord channel, but
working through it now, and benchmarking with the CodeChecker tool using only
clangsa. Is there an already established set of benchmarks y'all use somewhere?
IIRC, there was some github repo with som
Flandini wrote:
> Have you considered changing `MemRegion::getMemorySpace()` into
> `MemRegion::getMemorySpace(ProgramStateRef)`?
I thought about this, but I decided against it since I am thinking that
MemSpaces will eventually be their own separate thing, not part of the
MemRegion hierarchy,
@@ -0,0 +1,72 @@
+//===-- MemSpaces.cpp -*- C++
-*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
https://github.com/steakhal edited
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/steakhal commented:
I had a quick look at the PR, and over all it looks as expected. Good job!
Have you considered changing `MemRegion::getMemorySpace()` into
`MemRegion::getMemorySpace(ProgramStateRef)`?
This should lessen the confusion of which APIs should a dev use to get
https://github.com/Flandini updated
https://github.com/llvm/llvm-project/pull/123003
>From 7e0758d2ead53bd4288989b8b2eda218cd6dc34a Mon Sep 17 00:00:00 2001
From: Michael Flanders
Date: Mon, 13 Jan 2025 12:34:50 -0600
Subject: [PATCH 01/15] [analyzer] Add MemSpace trait to program state
This t
NagyDonat wrote:
> The only spot I thought was questionable to change is this lower bound check
> in `ArrayBoundCheckerV2`; I think it is fine to leave unchanged for now,
> thoughts?
>
> https://github.com/llvm/llvm-project/blob/7e0758d2ead53bd4288989b8b2eda218cd6dc34a/clang/lib/StaticAnalyzer
Xazax-hun wrote:
> This expression is indeed error-prone, but the problem is not the use of isa
> but the use of the method getMemorySpace()
My bad! You are right, it is not the `isa`. :)
> this error-prone situation will be resolved soon in a follow-up commit
I think it would be nice if w
NagyDonat wrote:
@Xazax-hun
When you write that this commit "makes memspaces a bit more error prone to use.
Do you think we could find a way to prevent using isa on memspaces?" do I
understand it correctly you mean that it's error-prone to use
`isa<...>(MR->getMemorySpace())` directly? This e
@@ -0,0 +1,72 @@
+//===-- MemSpaces.cpp -*- C++
-*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
Xazax-hun wrote:
> Do you mean moving away from a class hierarchy definition for memspaces and
> towards something else, maybe a enum Kind definition, or maybe just try to
> hide all the casting behind a different API?
I think it would be nice to do the latter in this patch so using `isa` dire
@@ -0,0 +1,72 @@
+//===-- MemSpaces.cpp -*- C++
-*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
@@ -0,0 +1,72 @@
+//===-- MemSpaces.cpp -*- C++
-*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
Flandini wrote:
> I like the direction of this PR but at the same time it makes memspaces a bit
> more error prone to use. Do you think we could find a way to prevent using
> `isa` on memspaces?
Do you mean moving away from a class hierarchy definition for memspaces and
towards something else
@@ -0,0 +1,72 @@
+//===-- MemSpaces.cpp -*- C++
-*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
https://github.com/Xazax-hun commented:
I like the direction of this PR but at the same time it makes memspaces a bit
more error prone to use. Do you think we could find a way to prevent using
`isa` on memspaces?
https://github.com/llvm/llvm-project/pull/123003
https://github.com/Xazax-hun edited
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Flandini wrote:
@NagyDonat, I made the requested changes, lmk if this looks better.
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/Flandini updated
https://github.com/llvm/llvm-project/pull/123003
>From 7e0758d2ead53bd4288989b8b2eda218cd6dc34a Mon Sep 17 00:00:00 2001
From: Michael Flanders
Date: Mon, 13 Jan 2025 12:34:50 -0600
Subject: [PATCH 01/14] [analyzer] Add MemSpace trait to program state
This t
@@ -0,0 +1,62 @@
+//===-- MemSpaces.cpp -*- C++
-*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
@@ -2404,8 +2414,10 @@ bool MallocChecker::SummarizeRegion(raw_ostream &os,
return true;
default: {
const MemSpaceRegion *MS = MR->getMemorySpace();
+const MemSpaceRegion *MSTrait = memspace::getMemSpaceTrait(State, MR);
Flandini wrote:
Changed
@@ -0,0 +1,62 @@
+//===-- MemSpaces.cpp -*- C++
-*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apa
https://github.com/Flandini updated
https://github.com/llvm/llvm-project/pull/123003
>From 7e0758d2ead53bd4288989b8b2eda218cd6dc34a Mon Sep 17 00:00:00 2001
From: Michael Flanders
Date: Mon, 13 Jan 2025 12:34:50 -0600
Subject: [PATCH 01/13] [analyzer] Add MemSpace trait to program state
This t
https://github.com/Flandini updated
https://github.com/llvm/llvm-project/pull/123003
>From 7e0758d2ead53bd4288989b8b2eda218cd6dc34a Mon Sep 17 00:00:00 2001
From: Michael Flanders
Date: Mon, 13 Jan 2025 12:34:50 -0600
Subject: [PATCH 1/6] [analyzer] Add MemSpace trait to program state
This tra
https://github.com/Flandini edited
https://github.com/llvm/llvm-project/pull/123003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
50 matches
Mail list logo
