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

Towards fixing GobView search: Avoid truncation of very large integers #1092

Merged
merged 9 commits into from
Feb 7, 2024

Conversation

stilscher
Copy link
Member

@stilscher stilscher commented Jun 20, 2023

When working on goblint/gobview#7 (comment) I tried the following semantic query on the regression test case tests/regression/00-sanity/01-assert.c:
{"kind":["var"],"target":["name","fail"],"find":["uses"],"expression":"fail == 0","mode":["Must"]}

It results in a fixpoint not found. The reason for this is that integers larger than 232, namely the start id for varinfos created by Goblint and the ids of pseudo return nodes are truncated (OCaml int is converted to a 32bit int in JavaScript with js_of_ocaml) and therefore differ from what was previously stored in the solver data. My proposed easy fix (at least for not too large projects) would be to decrease the start id by a factor of 10.

Todo

@stilscher stilscher self-assigned this Jun 20, 2023
@sim642 sim642 self-requested a review June 20, 2023 15:56
@sim642 sim642 added the bug label Jun 20, 2023
@sim642
Copy link
Member

sim642 commented Jun 20, 2023

Maybe we could just use negative IDs for what we generate in Goblint? Start counting down from -1. That way they don't conflict with anything CIL generates, regardless of imaginable program size and are also small.

Or is there some reason the IDs shouldn't be negative? Maybe @michael-schwarz knows?

@stilscher stilscher changed the title Avoid truncation of very large integers Fix GobView search: Avoid truncation of very large integers Jun 21, 2023
@michael-schwarz
Copy link
Member

Or is there some reason the IDs shouldn't be negative? Maybe @michael-schwarz knows?

Off the top of my head, I cannot think of any reason against it.

Tbh, I am bit surprised we end up with 32-bit integers on the javascript side, normally this should be 54 bit integers...

@stilscher
Copy link
Member Author

Here it states that OCaml ints are converted to JavaScript 32-bit integers. Also, the truncation occurs for integers >= 2147483648 (2^31). This is observable because an error is thrown when transpiling:
Warning: integer overflow: integer 0x80000000 (2147483648) truncated to 0x80000000 (-2147483648); the generated code might be incorrect.
Where did you find the information that it should be 54 bit integers?

@michael-schwarz
Copy link
Member

Very strange. Might have to make some experiments after the holidays...

I still like simmo's idea to go for negative numbers in these cases.

The thing is that JavaScript doesn't really have integers: on that vpage it says they're mapped to number (as is float), which is 64bit IEEE 754 floating points, which would give you 54 bit integers. But maybe they do some bitwise crap with every instruction to make sure it is actually 32 bit operations?.

Comment on lines +63 to +67
|> List.filter_map (function
| Cil.GVar (v, _, _) -> Some (v.vname, Cil.Fv v)
| Cil.GFun (f, l) -> Some (f.svar.vname, Cil.Fv f.svar)
| Cil.GVarDecl (v, l) -> Some (v.vname, Cil.Fv v)
| _ -> None)
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought for future: at some point this old expression evaluation should be switched from the odd Format-CIL parser (see goblint-cil issues) to the normal expression parser that's used for witness invariants. That avoids fixing the same issues twice.

At that point one might reconsider this being a transformation altogether, because it isn't. Simply using EvalInt or EvalValue queries directly from GobView should be possible.

@michael-schwarz
Copy link
Member

What is the status here?

src/util/cilfacade.ml Outdated Show resolved Hide resolved
src/framework/cfgTools.ml Outdated Show resolved Hide resolved
@stilscher stilscher changed the title Fix GobView search: Avoid truncation of very large integers Towards fixing GobView search: Avoid truncation of very large integers Feb 1, 2024
@stilscher stilscher marked this pull request as ready for review February 1, 2024 18:16
Copy link
Member

@sim642 sim642 left a comment

Choose a reason for hiding this comment

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

Looks like this should be good to go now.

@stilscher stilscher merged commit 5e3ed6c into master Feb 7, 2024
17 checks passed
@stilscher stilscher deleted the fix-gobview-search branch February 7, 2024 11:43
@sim642 sim642 added this to the v2.4.0 milestone Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants