Skip to content

Conversation

@sim642
Copy link
Member

@sim642 sim642 commented Mar 18, 2022

Turns out #634, which performs minimal restarting for write-only globals like accesses, is insufficient to even guarantee from-scratch consistency of write-only globals. Hence there's no from-scratch consistency of race warnings either!

Problems

malloc nodes

In tests/incremental/13-restart-write/04-malloc-node.c the incremental run gives two memory locations (old and new malloc) instead of just the expected one. This causes an inconsistency in the number of race warnings because the number of memory locations isn't even consistent (not to mention anything about their raciness).
This is because alloc variables are based on the node of malloc, which changes if the function is incrementally changed.

The possible solution for this is to not include nodes in alloc variables, just like the disabling of ana.thread.include-node doesn't include them in thread IDs. But then alloc variables would be just per-function, so all malloc data from a single function gets joined together. If two different types of data are stored, then we immediately get value domain supertop, which is far from ideal.

EDIT: I have now added ana.malloc.include-node, which does fix this specific problem.

Broader issue

We've seen the node-based things be a problem for incrementality in thread IDs and now alloc variables, but the issue is much more general. It really means that nothing should be based on nodes since incremental changes may change their IDs. This possibly also includes access collecting (which refer to nodes as their origin), but maybe more things which don't immediately pop into mind.

So really what we need is very fine-grained update, which keeps statement IDs for everything that didn't actually change and simply removes/adds IDs for actually removed/added statements.

Indirect access removal

In tests/incremental/13-restart-write/05-race-call-remove.c the incremental run has a race, whereas it should not. When the call to a function accessing the global in a racy way is removed, then minimal write-only restarting doesn't get rid of that access, because it doesn't originate from a changed function. The destabilization of the old copy doesn't destabilize anything in foo, because normal destabilization only follows infl, but not side_infl.

The possible solution for this is something like the transitive sides restarting with destabilize_with_side, but slightly weaker: it doesn't need to restart anything, but only destabilize via infl and side_infl.
This also fixes the above malloc node issue on the original knot_comb race benchmark, but doubles the basic incremental runtime from 15s to 30s (from-scratch is ~60s). Despite just being more extensive destabilization, it causes a lot more recomputation, which aborting doesn't even help with.

Broader issue

This again is not just specific to write-only restarting, but incremental postsolving in general. To avoid reevaluating everything we make the assumption that all superstable unknowns are reachable and don't prune them. But that's an overapproximation of the reachable unknowns, since in the call removal case it similarly would still think that the unknowns of foo are reachable since they never get destabilized. Destabilization along side_infl would also fix that, but at the same high cost.

@sim642 sim642 self-assigned this Mar 18, 2022
@sim642 sim642 mentioned this pull request Mar 18, 2022
5 tasks
@sim642
Copy link
Member Author

sim642 commented May 3, 2022

The second problem identified here was fixed differently by #713. The corresponding test was cherry-picked.

Since disabling ana.malloc.include-node introduces even more supertops and thus possible unsoundness, it's probably not needed now.

Therefore I'm just closing this PR for now.

@sim642 sim642 closed this May 3, 2022
@sim642 sim642 changed the title Minimal write-only restarting is insufficient Minimal write-only restarting is insufficient: add ana.malloc.include-node May 3, 2022
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.

1 participant