-
Notifications
You must be signed in to change notification settings - Fork 44
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
Invalidate memory properly with laxLoadsAndStores #928
Conversation
I've marked this as a draft for now since I'd like to test this more in SAW (the intended use case) before I land it. Feedback is very much welcome in the meantime. |
9854366
to
d7e54ab
Compare
@@ -645,7 +647,12 @@ readMemInvalidate :: | |||
SymBV sym w {- ^ The length of the set region -} -> | |||
(StorageType -> LLVMPtr sym w -> ReadMem sym (PartLLVMVal sym)) -> | |||
ReadMem sym (PartLLVMVal sym) | |||
readMemInvalidate sym w end mop (LLVMPointer blk off) tp d msg sz readPrev = | |||
readMemInvalidate sym w end mop (LLVMPointer blk off) tp d msg sz readPrev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, this is too easy to be correct, I think. I think this has to be pushed down into the InRange
cases of the subFn
functions below, which implement the case that the requested read actually overlaps with the invalidation in question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Darn. I'm a bit alarmed that this causes the test case that I wrote for UnstableSymbolic
to pass, and moreover, it seems to do what I'd expect on the SAW side. Perhaps I'm not tickling the right code paths. In any case, I'll look into pushing this logic down lower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic you have here will work correctly iff you invalidate all the memory in an allocation. Otherwise, you actually have to check to what degree the requested read overlaps with the invalidation write, which is what the more complicated logic in the body does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Let me know if you'd prefer the test case to distinguish between these two cases of readMemInvalidate
, but I'm pretty confident that the changes I've just pushed should do the right thing now.
d7e54ab
to
664753f
Compare
This bumps the `crucible` submodule to include GaloisInc/crucible#928. The `crucible` changes require adding some additional `?memOpts :: MemOptions` constraints to various functions.
I've had the chance to test this on SAW, and everything works pretty well in the
I need to do some further digging to figure out if this is the fault of Crucible or SAW. |
This bumps the `crucible` submodule to include GaloisInc/crucible#928. The `crucible` changes require adding some additional `?memOpts :: MemOptions` constraints to various functions.
The In any case, this gives me confidence that everything is working as expected (SBV limitations notwithstanding). I'll mark this as ready for review. |
Prior to this change, invalidated memory would always result in an error when read from. This isn't quite the semantics that we would want when `laxLoadsAndStores` is enabled, however, since reading should always result in a symbolic value of some sort. This patch changes the mechanics of `doInvalidate` and `readMemInvalidate` such that when `laxLoadsAndStores` is enabled, the memory is given the appropriate semantics depending on whether `indeterminateLoadBehavior` is `StableSymbolic` or `UnstableSymbolic`. This adds a `?memOpts :: MemOptions` constraint to `doInvalidate`, which is a breaking API change. That being said, the only call site that I am aware of for the `doInvalidate` function is SAW, so the breakage should be relatively minor. Furthermore, there were no test cases for `doInvalidate` before this, so this patch adds some.
664753f
to
f241c80
Compare
This bumps the `crucible` submodule to include GaloisInc/crucible#928. The `crucible` changes require adding some additional `?memOpts :: MemOptions` constraints to various functions.
Submodules: Adapt to GaloisInc/crucible#914, GaloisInc/crucible#906, and GaloisInc/crucible#928
Prior to this change, invalidated memory would always result in an error when read from. This isn't quite the semantics that we would want when
laxLoadsAndStores
is enabled, however, since reading should always result in a symbolic value of some sort. This patch changes the mechanics ofdoInvalidate
andreadMemInvalidate
such that whenlaxLoadsAndStores
is enabled, the memory is given the appropriate semantics depending on whetherindeterminateLoadBehavior
isStableSymbolic
orUnstableSymbolic
.This adds a
?memOpts :: MemOptions
constraint todoInvalidate
, which is a breaking API change. That being said, the only call site that I am aware of for thedoInvalidate
function is SAW, so the breakage should be relatively minor. Furthermore, there were no test cases fordoInvalidate
before this, so this patch adds some.