Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider pointees separately for refinement #1659

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

michael-schwarz
Copy link
Member

Closes #1658

@@ -100,6 +100,25 @@ struct
if M.tracing then M.tracel "inv" "st from %a to %a" D.pretty st D.pretty r;
r
)
| Mem (Lval lv), NoOffset ->
let lvals = eval_lv ~man st x in
let res = AD.fold (fun a acc ->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does something special need to be done for the unknown pointer?

@@ -100,6 +100,25 @@ struct
if M.tracing then M.tracel "inv" "st from %a to %a" D.pretty st D.pretty r;
r
)
| Mem (Lval lv), NoOffset ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to limit this to NoOffset? Seems like it shouldn't really matter whether the pointer in in a struct field or something.

let old_val = map_oldval old_val (Cilfacade.typeOfLval x) in
let v = apply_invariant ~old_val ~new_val:c' in
if is_some_bot v then
D.join acc (try contra st with Analyses.Deadcode -> D.bot ())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now swallows all deadcode, but this might not be entirely right. In particular, if the invariant doesn't hold for any pointees, this will join together a bunch of bots, but not raise Deadcode, which I think it would've done before.
We might have to have some extra logic here that if all cases were dead, then Deadcode is also raised after the fold. I think MCP does something similar (but other way around, for any instead of all).

@sim642
Copy link
Member

sim642 commented Jan 24, 2025

I started thinking about this more in relation to double dereferences, etc and realized this should be some kind of recursive process because pointers can be refined at every level.
For example

p → {&q, &r}
q → {&x, &y}
r → {&z, &w}
x → {1}
y → {2}
z → {1}
w → {3}

Assuming **p == 2 should refine p → {&q} and q → {&y}.
Seems like Mem should just recurse (while refining the pointer) and Var would be the base case.

@sim642
Copy link
Member

sim642 commented Jan 24, 2025

Or perhaps not be so ambitious and handle nested dereferences... It's surprisingly tricky to get right even in very simple theoretical setting (the unassume paper appendix on pointers). We spent over an hour with Vesal trying to get it right in a semi-general case (where lvalues are sequences of derefs ending with a variable, so no arbitrary expressions in dereference, nor offsets). The formal definition is also very inefficient in subtle ways because the AST of such lvalues is in the wrong direction compared to the order of dereferencing pointers (which happens from inside out).
It's worth revisiting separately to not block this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine pointer sets upon assertion about pointee
2 participants