-
Notifications
You must be signed in to change notification settings - Fork 88
Recognize pointers wrapped in TNamed in memOutOfBounds and fix bit-to-byte comparison bug
#1676
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
Conversation
Co-authored-by: Simmo Saan <[email protected]>
| let one = intdom_of_int 1 in | ||
| let casted_es = ID.sub casted_es one in | ||
| let casted_offs = ID.cast_to (Cilfacade.ptrdiff_ikind ()) offs_intdom in | ||
| let casted_offs = ID.div (ID.cast_to (Cilfacade.ptrdiff_ikind ()) offs_intdom) (ID.of_int (Cilfacade.ptrdiff_ikind ()) (Z.of_int 8)) in |
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.
Nice catch, I guess this tended to kill us almost all of the time.
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.
There's also a documentation problem (by me) that might've contributed to this:
analyzer/src/cdomain/value/cdomains/offset_intf.ml
Lines 92 to 96 in 153ce28
| val to_index: ?typ:GoblintCil.typ -> t -> idx | |
| (** Physical memory offset in bytes of the entire offset. | |
| Used for {!semantic_equal}. | |
| @param typ base type. *) |
Contrary to the description, it returns the offset in bits (matching what CIL has). It was fine for
semantic_equal because both would be in bits, but not for other uses (this is the only other one I think).
I'll have to open a follow-up PR to fix that. Changing the documentation would be easy, but it's not entirely the right thing: ptrdiff_t is large enough to fit offsets in bytes, but not necessarily in bits (in practice the difference might be irrelevant unless a program actually allocates more than 8th of the address space). Having to_index return bytes would be consistent with ptrdiff_ikind.
Looking through Goblint logs for the SV-COMP MemSafety tasks revealed several issues that are fixed in this PR:
TNamedwere not recognized and considered as pointersTNamedwas not workingAdditional fixes:
ID.lt casted_es casted_offsrequires subtracting 1 fromcasted_es, but the corresponding log message about the size ofcasted_eswas misleading because of the subtraction. The log message now displays the actual size before the subtraction to prevent confusion.