[PATCH] D70320: [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
> I plan some refactoring but this first patch is meant to be a single
> separation of code.
Sounds great!
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val)
{
if (auto Reg = Val.getAsRegion()) {
Reg = Reg->getMostDerivedObjectRegion();
-return State->get(Reg);
+return State->remove(Reg);
} else if (const auto Sym = Val.getAsSymbol()) {
-return State->get(Sym);
+return State->remove(Sym);
} else if (const auto LCVal = Val.getAs()) {
baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > Charusso wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > Maybe move this function to `Iterator.cpp` as well, and then move
> > > > > > the definitions for iterator maps from `Iterator.h` to
> > > > > > `Iterator.cpp`, which will allow you to use the usual
> > > > > > `REGISTER_MAP_WITH_PROGRAMSTATE` macros, and additionally guarantee
> > > > > > that all access to the maps goes through the accessor methods that
> > > > > > you provide?
> > > > > Hmm, I was trying hard to use these macros but failed so I reverted
> > > > > to the manual solution. I will retry now.
> > > > Here is a how-to: https://reviews.llvm.org/D69726
> > > >
> > > > You need to add the fully qualified names to the register macro because
> > > > of the global scope. I hope it helps.
> > > OK, I checked it now. If we want to put the maps into `Iterator.cpp` then
> > > we also have to move a couple of functions there which are only used by
> > > the modeling part: the internals of `checkDeadSymbols()` and
> > > `checkLiveSymbols()` must go there, although no other checker should use
> > > them. Also `processIteratorPositions()` iterates over these maps, thus it
> > > must also go there. Should I do it? Generally I like the current solution
> > > better, only functions used by multiple checker classes are in the
> > > library.
> > >
> > > On the other hand I do not see why I should move `assumeNoOverflow()` to
> > > `Iterator.cpp`? This function is only used by the modeling part, and does
> > > not refer to the maps. You mean that we should ensure this constraint
> > > whenever setting the position for any iterator? This would mean that we
> > > should decompose every symblic expression and (re)assume this range on
> > > the symbolic part. Or we should replace `setPosition()` by at least two
> > > different functions (e.g. `setAdvancedPosition()` and
> > > `setConjuredPosition()`) but this means a total reqriting of the modeling.
> > >
> > > I plan some refactoring but this first patch is meant to be a single
> > > separation of code.
> > @Charusso It does not help because the macros put the maps into a local
> > namespace. If it is in a header, it means separate maps for every checker
> > and the modeling which of course does not work.
> @NoQ What is the decision? Should I move everything to the lib such as the
> body of `checkDeadSymbols()` and `checkLiveSymbols()` which I am reluctant
> because they belong only to the modeling, or leave it as it is now?
I think all the functions for manipulating the state trait (including dead
symbol cleanup and escapes) should live in the modeling file. Like, it doesn't
mean that `checkDeadSymbols` itself should necessarily live there, but in any
case the file should provide a nice cleanup method that can be invoked from the
real `checkDeadSymbols`.
This way you can encapsulate all the implementation details in the cpp file and
only interact with it with fancy accessors.
I'm perfectly ok with delaying this work ^.^
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70320/new/
https://reviews.llvm.org/D70320
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70320: [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers
baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val)
{
if (auto Reg = Val.getAsRegion()) {
Reg = Reg->getMostDerivedObjectRegion();
-return State->get(Reg);
+return State->remove(Reg);
} else if (const auto Sym = Val.getAsSymbol()) {
-return State->get(Sym);
+return State->remove(Sym);
} else if (const auto LCVal = Val.getAs()) {
baloghadamsoftware wrote:
> Charusso wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > Maybe move this function to `Iterator.cpp` as well, and then move the
> > > > definitions for iterator maps from `Iterator.h` to `Iterator.cpp`,
> > > > which will allow you to use the usual `REGISTER_MAP_WITH_PROGRAMSTATE`
> > > > macros, and additionally guarantee that all access to the maps goes
> > > > through the accessor methods that you provide?
> > > Hmm, I was trying hard to use these macros but failed so I reverted to
> > > the manual solution. I will retry now.
> > Here is a how-to: https://reviews.llvm.org/D69726
> >
> > You need to add the fully qualified names to the register macro because of
> > the global scope. I hope it helps.
> OK, I checked it now. If we want to put the maps into `Iterator.cpp` then we
> also have to move a couple of functions there which are only used by the
> modeling part: the internals of `checkDeadSymbols()` and `checkLiveSymbols()`
> must go there, although no other checker should use them. Also
> `processIteratorPositions()` iterates over these maps, thus it must also go
> there. Should I do it? Generally I like the current solution better, only
> functions used by multiple checker classes are in the library.
>
> On the other hand I do not see why I should move `assumeNoOverflow()` to
> `Iterator.cpp`? This function is only used by the modeling part, and does not
> refer to the maps. You mean that we should ensure this constraint whenever
> setting the position for any iterator? This would mean that we should
> decompose every symblic expression and (re)assume this range on the symbolic
> part. Or we should replace `setPosition()` by at least two different
> functions (e.g. `setAdvancedPosition()` and `setConjuredPosition()`) but this
> means a total reqriting of the modeling.
>
> I plan some refactoring but this first patch is meant to be a single
> separation of code.
@Charusso It does not help because the macros put the maps into a local
namespace. If it is in a header, it means separate maps for every checker and
the modeling which of course does not work.
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val)
{
if (auto Reg = Val.getAsRegion()) {
Reg = Reg->getMostDerivedObjectRegion();
-return State->get(Reg);
+return State->remove(Reg);
} else if (const auto Sym = Val.getAsSymbol()) {
-return State->get(Sym);
+return State->remove(Sym);
} else if (const auto LCVal = Val.getAs()) {
baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > Charusso wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > Maybe move this function to `Iterator.cpp` as well, and then move the
> > > > > definitions for iterator maps from `Iterator.h` to `Iterator.cpp`,
> > > > > which will allow you to use the usual
> > > > > `REGISTER_MAP_WITH_PROGRAMSTATE` macros, and additionally guarantee
> > > > > that all access to the maps goes through the accessor methods that
> > > > > you provide?
> > > > Hmm, I was trying hard to use these macros but failed so I reverted to
> > > > the manual solution. I will retry now.
> > > Here is a how-to: https://reviews.llvm.org/D69726
> > >
> > > You need to add the fully qualified names to the register macro because
> > > of the global scope. I hope it helps.
> > OK, I checked it now. If we want to put the maps into `Iterator.cpp` then
> > we also have to move a couple of functions there which are only used by the
> > modeling part: the internals of `checkDeadSymbols()` and
> > `checkLiveSymbols()` must go there, although no other checker should use
> > them. Also `processIteratorPositions()` iterates over these maps, thus it
> > must also go there. Should I do it? Generally I like the current solution
> > better, only functions used by multiple checker classes are in the library.
> >
> > On the other hand I do not see why I should move `assumeNoOverflow()` to
> > `Iterator.cpp`? This function is only used by the modeling part, and does
> > not refer to the maps. You mean that we should ensure this constraint
> > whenever setting the position for any iterator? This would mean that we
> > should decompose every symblic expression and (re)ass
[PATCH] D70320: [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers
baloghadamsoftware marked an inline comment as done and an inline comment as
not done.
baloghadamsoftware added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val)
{
if (auto Reg = Val.getAsRegion()) {
Reg = Reg->getMostDerivedObjectRegion();
-return State->get(Reg);
+return State->remove(Reg);
} else if (const auto Sym = Val.getAsSymbol()) {
-return State->get(Sym);
+return State->remove(Sym);
} else if (const auto LCVal = Val.getAs()) {
Charusso wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > Maybe move this function to `Iterator.cpp` as well, and then move the
> > > definitions for iterator maps from `Iterator.h` to `Iterator.cpp`, which
> > > will allow you to use the usual `REGISTER_MAP_WITH_PROGRAMSTATE` macros,
> > > and additionally guarantee that all access to the maps goes through the
> > > accessor methods that you provide?
> > Hmm, I was trying hard to use these macros but failed so I reverted to the
> > manual solution. I will retry now.
> Here is a how-to: https://reviews.llvm.org/D69726
>
> You need to add the fully qualified names to the register macro because of
> the global scope. I hope it helps.
OK, I checked it now. If we want to put the maps into `Iterator.cpp` then we
also have to move a couple of functions there which are only used by the
modeling part: the internals of `checkDeadSymbols()` and `checkLiveSymbols()`
must go there, although no other checker should use them. Also
`processIteratorPositions()` iterates over these maps, thus it must also go
there. Should I do it? Generally I like the current solution better, only
functions used by multiple checker classes are in the library.
On the other hand I do not see why I should move `assumeNoOverflow()` to
`Iterator.cpp`? This function is only used by the modeling part, and does not
refer to the maps. You mean that we should ensure this constraint whenever
setting the position for any iterator? This would mean that we should decompose
every symblic expression and (re)assume this range on the symbolic part. Or we
should replace `setPosition()` by at least two different functions (e.g.
`setAdvancedPosition()` and `setConjuredPosition()`) but this means a total
reqriting of the modeling.
I plan some refactoring but this first patch is meant to be a single separation
of code.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70320/new/
https://reviews.llvm.org/D70320
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70320: [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers
Charusso added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val)
{
if (auto Reg = Val.getAsRegion()) {
Reg = Reg->getMostDerivedObjectRegion();
-return State->get(Reg);
+return State->remove(Reg);
} else if (const auto Sym = Val.getAsSymbol()) {
-return State->get(Sym);
+return State->remove(Sym);
} else if (const auto LCVal = Val.getAs()) {
baloghadamsoftware wrote:
> NoQ wrote:
> > Maybe move this function to `Iterator.cpp` as well, and then move the
> > definitions for iterator maps from `Iterator.h` to `Iterator.cpp`, which
> > will allow you to use the usual `REGISTER_MAP_WITH_PROGRAMSTATE` macros,
> > and additionally guarantee that all access to the maps goes through the
> > accessor methods that you provide?
> Hmm, I was trying hard to use these macros but failed so I reverted to the
> manual solution. I will retry now.
Here is a how-to: https://reviews.llvm.org/D69726
You need to add the fully qualified names to the register macro because of the
global scope. I hope it helps.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70320/new/
https://reviews.llvm.org/D70320
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70320: [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers
baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val)
{
if (auto Reg = Val.getAsRegion()) {
Reg = Reg->getMostDerivedObjectRegion();
-return State->get(Reg);
+return State->remove(Reg);
} else if (const auto Sym = Val.getAsSymbol()) {
-return State->get(Sym);
+return State->remove(Sym);
} else if (const auto LCVal = Val.getAs()) {
NoQ wrote:
> Maybe move this function to `Iterator.cpp` as well, and then move the
> definitions for iterator maps from `Iterator.h` to `Iterator.cpp`, which will
> allow you to use the usual `REGISTER_MAP_WITH_PROGRAMSTATE` macros, and
> additionally guarantee that all access to the maps goes through the accessor
> methods that you provide?
Hmm, I was trying hard to use these macros but failed so I reverted to the
manual solution. I will retry now.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70320/new/
https://reviews.llvm.org/D70320
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70320: [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers
NoQ added a comment.
I love how this checker is pioneering in so many ways.
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val)
{
if (auto Reg = Val.getAsRegion()) {
Reg = Reg->getMostDerivedObjectRegion();
-return State->get(Reg);
+return State->remove(Reg);
} else if (const auto Sym = Val.getAsSymbol()) {
-return State->get(Sym);
+return State->remove(Sym);
} else if (const auto LCVal = Val.getAs()) {
Maybe move this function to `Iterator.cpp` as well, and then move the
definitions for iterator maps from `Iterator.h` to `Iterator.cpp`, which will
allow you to use the usual `REGISTER_MAP_WITH_PROGRAMSTATE` macros, and
additionally guarantee that all access to the maps goes through the accessor
methods that you provide?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70320/new/
https://reviews.llvm.org/D70320
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
