This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Keep track of escaped locals.
ClosedPublic

Authored by xazax.hun on Dec 6 2019, 3:26 PM.

Details

Summary

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.

Diff Detail

Event Timeline

xazax.hun created this revision.Dec 6 2019, 3:26 PM

So some of the questions that came to my mind:

  1. Should this be exposed to the checkers?
  2. 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.
NoQ added a comment.Dec 6 2019, 3:50 PM

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.

NoQ added a comment.Dec 6 2019, 3:52 PM

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.

NoQ added inline comments.Dec 6 2019, 3:53 PM
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.

xazax.hun updated this revision to Diff 232675.Dec 6 2019, 4:25 PM
  • Add cleanup.
xazax.hun marked an inline comment as done.Dec 6 2019, 4:28 PM
xazax.hun added inline comments.
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.

xazax.hun edited the summary of this revision. (Show Details)Dec 6 2019, 4:55 PM
xazax.hun marked an inline comment as done.Dec 9 2019, 9:12 AM
xazax.hun added inline comments.
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?

NoQ accepted this revision.Dec 9 2019, 7:05 PM

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.

This revision is now accepted and ready to land.Dec 9 2019, 7:05 PM
This revision was automatically updated to reflect the committed changes.
xazax.hun updated this revision to Diff 233198.Dec 10 2019, 2:59 PM
  • A first take on trying to reuse processPointerEscapedOnBind.

Whoops, this one was updated accidentally. Wrong tab :(