-
Notifications
You must be signed in to change notification settings - Fork 77
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
Bitfield Domain #1623
base: master
Are you sure you want to change the base?
Bitfield Domain #1623
Conversation
…d some revision. Possible side-effects and runtime in O(n^2) while O(n) should be possible.
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.
Semgrep OSS found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…act bitfield by eliminating constants that would result in a shift to zero beforehand
* implemented modulo * current changes * implementation of add, sub, mul based on paper * implemented neg * bug fixes for arith operators --------- Co-authored-by: Adrian Krauß <[email protected]>
Overflow and wrapping
let o_mask = ref o in | ||
let z_mask = ref z 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.
Why ref
?
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.
It was not necessary to use references in this case, therefore the code was changed and the ref removed.
let range ik bf = (BArith.min ik bf, BArith.max ik bf) | ||
|
||
let maximal (z,o) = | ||
if (z < Ints_t.zero) <> (o < Ints_t.zero) then Some o |
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.
Can you explain this?
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.
We basically check whether the sign bit is clear, i.e., whether we only represent positive or negative numbers. Under such a condition, the maximal represented number is just the number with the maximal number of ones.
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.
Hmm, that works if the ikind
and the type of Ints_t
coincide. But what if you have, e.g., the ikind
of signed char
? There you would need to check the 7th bit right?
I wasn't there for the first couple of meetings, so maybe you'll need to enlighten us tomorrow how you deal with different ikind
s in your domain in general 😃
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.
Thanks for the PR! I left some first comments in some places, but have not worked through the entire thing.
bug fix in to_bitfield()
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.
Thank you for the PR! I left some comments below.
let to_bitfield ik z = | ||
match z with None -> (Ints_t.lognot Ints_t.zero, Ints_t.lognot Ints_t.zero) | Some (x,y) -> | ||
let (min_ik, max_ik) = Size.range ik in | ||
let startv = Ints_t.max x (Ints_t.of_bigint min_ik) 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.
Can you make this function easier to read?
A comment about what is done here overall may help. Possibly one may also find a more telling name for the helper function analyze_bits
?
let isNegative = Ints_t.logand signBit o <> Ints_t.zero in | ||
if isSigned ik && isNegative then Ints_t.logor signMask (Ints_t.lognot z) | ||
else Ints_t.lognot z | ||
in let max ik (z,o) = |
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.
Indentation.
let signBit = Ints_t.shift_left Ints_t.one ((Size.bit ik) - 1) in | ||
let signMask = Ints_t.lognot (Ints_t.of_bigint (snd (Size.range ik))) 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.
signBit
and signMask
only seem to depend on ik
. Can one extract them from min
and max
, so they don't have to be defined twice?
tests/unit/cdomains/intDomainTest.ml
Outdated
assert_shift_right ~rev_cond:true ik (`I [max_of ik]) top top; (* the sign bit shouldn't be set with right shifts if its unset *) | ||
|
||
assert_shift_right ik (`I [2]) (`I [-1]) top; (* Negative shifts are undefined behavior *) | ||
(*assert_shift_right ik (`I [min_of ik]) top top;*) (*TODO implementation-defined sign-bit handling *) |
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.
What is this todo about?
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.
This test case could be removed if you agree with our design choice to handle right shifts as arithmetic ones.
Given the C standard's definition of right shifts on signed integers, it might be better to leave the upper bits undetermined. In that case this test case should pass.
| true, true -> `Undetermined | ||
| false, false -> `Invalid | ||
| true, false -> `Zero | ||
| false, true -> `One |
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.
Is there a specific reason you are using polymorphic variants? If not, it would be preferable to define a variant type. See also here.
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.
No, they were naively chosen to be used for readability purposes here but not necessary in any way. This part will be removed.
else None | ||
|
||
let minimal (z,o) = | ||
if (z < Ints_t.zero) <> (o < Ints_t.zero) then Some (!:z) |
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.
Using the polymorphic compare <
here probably works, but the compare on Ints_t
is probably still the better choice?
(* (M.warn ~category:M.Category.Integer.overflow "Bitfield: Value was outside of range, indicating overflow, but 'sem.int.signed_overflow' is 'assume_none' -> Returned Top"; *) | ||
(* (bot (), overflow_info)) *) |
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.
Commented out code should be removed in the final version.
|
||
(* Conversions *) | ||
|
||
let of_interval ?(suppress_ovwarn=false) ik (x,y) = |
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.
Does this function share code with the to_bitfield
function in the interval domain? If so, could this be deduplicated?
if Ints_t.compare bigger_number endv <= 0 then | ||
`top | ||
else | ||
if Ints_t.equal (Ints_t.logand (Ints_t.shift_right startv pos) Ints_t.one) Ints_t.one then | ||
`one | ||
else | ||
`zero |
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.
Here, one could also define a variant type, instead of using a polymorphic variant type.
(* Unit Tests *) | ||
|
||
let arbitrary ik = |
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.
arbitrary
is used for unit tests, indeed; still the comment may be a bit misleading, since there are no unit tests following the comment.
…ck' into pull-request-feedback
fixed non-compilation
let defined_shifts = cap_bitshifts_to_precision ik b in (* O(2^(log n)) *) | ||
let max_shift = if Z.fits_int (BArith.max ik b) then Z.to_int (BArith.max ik b) else Int.max_int in | ||
let (min_ik, max_ik) = Size.range ik in | ||
let min_res = if max_shift < 0 then Z.sub min_ik Z.one else Z.shift_left (BArith.min ik a) max_shift in |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: semgrep.z-sub-one Warning
let max_shift = if Z.fits_int (BArith.max ik b) then Z.to_int (BArith.max ik b) else Int.max_int in | ||
let (min_ik, max_ik) = Size.range ik in | ||
let min_res = if max_shift < 0 then Z.sub min_ik Z.one else Z.shift_left (BArith.min ik a) max_shift in | ||
let max_res = if max_shift < 0 then Z.add max_ik Z.one else Z.shift_left (BArith.max ik a) max_shift in |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: semgrep.z-add-one Warning
This pull request enhances the IntDomain by introducing a new BitfieldDomain capable of tracking bitwise operations.
Additionally, it adds all newly possible refinement directions to improve precision in the IntDomain.
This implementation is part of the
Automated Bug Hunting and Beyond
practical course at TUM.