We want to escape all symbols that are stored into escaped regions. The problem is, we did not know which local regions were escaped. Until now.
Details
Diff Detail
Event Timeline
So some of the questions that came to my mind:
- Should this be exposed to the checkers?
- Where should we actually have this trait registered? ProgramState? ExprEngine? Both of them need access, and due to layering I feel like it cannot be in ExprEngine but I might be wrong.
I've been hiding these internal traits in ExprEngine lately but i don't have a strong preference. I don't see any immediate need to expose this info to the checkers, nor do i see a need to actively hide it.
And, yeah, needs dead region cleanup :)
clang/lib/StaticAnalyzer/Core/ProgramState.cpp | ||
---|---|---|
229–233 ↗ | (On Diff #232663) | Ok, so how do we want to deal with non-base regions? I think for the purposes of escaping we try to stick to per-base-region granularity (cf. the C-style linked list example). This is also going to be nice because the amount of regions we'll have to track is going to be more limited and there's going to be no problem of representing the same region in multiple different ways. |
Hiding the trait in ExprEngine has the additional benefit of disallowing *direct* access to the trait - it can only be accessed through getters. It also doesn't prevent us from exposing the trait to checkers as a method on ProgramState in the future, because ProgramStateManager has access to SubEngine.
clang/lib/StaticAnalyzer/Core/ProgramState.cpp | ||
---|---|---|
48–61 ↗ | (On Diff #232663) | Wait, you are preventing direct access anyway by putting this stuff into the .cpp file. In this case i think you can safely use the REGISTER_... macros. |
clang/lib/StaticAnalyzer/Core/ProgramState.cpp | ||
---|---|---|
48–61 ↗ | (On Diff #232663) | Yeah, initially I wanted to hide this trait but at this point I have no idea how to do that. If I get the layering right, ExprEngine is using ProgramState, and ProgramState should not reference ExprEngine. So if I want to use a trait in both it should be in ProgramState. The reason why I also need to touch ProgramState, because the list of invalidated regions are readily available there. Probably it is possible to somehow get that list in ExprEngine, but I think that would be way more complicated than the current solution. |
clang/lib/StaticAnalyzer/Core/ProgramState.cpp | ||
---|---|---|
48–61 ↗ | (On Diff #232663) | Well, I realized that ProgramState can access SubEngine, so as long as we extend the abstract interface there we could move the trait to ExprEngine. What do you prefer? |
Looks great, thanks! I'd love to see what effect it immediately causes and whether there will be any true or false negatives because of this change.
Also i-i-it's not that i insist or something, but somebody will have to add it to the exploded graph dumps :)
clang/lib/StaticAnalyzer/Core/ProgramState.cpp | ||
---|---|---|
48–61 ↗ | (On Diff #232663) | I very slightly prefer to have it in ExprEngine because it's consistent with other existing traits: defined in the place in which they're maintained, don't show up in the overall definition of the ProgramState. |