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

Add fuzzer to bounds_of_expr_in_scope + fix discovered overflow bugs #5895

Merged
merged 12 commits into from
Apr 12, 2021

Conversation

rootjalex
Copy link
Member

This PR adds fuzz testing (based off of fuzz_simplify.cpp), and fixes a few of the overflow bugs that it discovers. Addresses #4724. The two examples below are results from the fuzzer:

Expr test = (x2(uint32(1607181356)) + max(x2(uint32(uint8(e))), x2(UIntImm::make(UInt(32), 3844224344))*ramp(uint32(uint8(d)), uint32(uint8(c)), 2)));
Scope<Interval> scope;
scope.push("c", Interval(uint8(89), uint8(99)));
scope.push("d", Interval(uint8(44), uint8(64)));
scope.push("e", Interval(uint8(90), uint8(111)));
Interval interval = bounds_of_expr_in_scope(test, scope);
std::cerr << "[" << interval.min << "\t, " << interval.max << "]\n";
map<string, Expr> vars;
vars["c"] = uint8(89);
vars["d"] = uint8(51);
vars["e"] = uint8(93);
int lane = 0;
Expr test_expr = extract_lane(test, lane);
Expr test_sol = simplify(substitute(vars, test_expr));
std::cerr << test_expr << " to: " << test_sol << "\n";

->

[(uint32)1607181446 , (void *)pos_inf]
(max(uint32((uint8)d)*(uint32)3844224344, uint32((uint8)e)) + (uint32)1607181356) to: (uint32)94127284

This bug is the result of improper overflow handling for UInt(32), as these values can be labelled as infinite by bounds inference, but have defined overflow semantics.

Expr test = int32x2(ramp(uint32(b), UIntImm::make(UInt(32), 4294967261), 2));
Scope<Interval> scope;
scope.push("b", Interval(uint1(0), uint1(0)));
Interval interval = bounds_of_expr_in_scope(test, scope);
std::cerr << "un-simplified: [" << interval.min << "\t, " << interval.max << "]\n";
interval.min = simplify(interval.min);
interval.max = simplify(interval.max);
std::cerr << "simplified: [" << interval.min << "\t, " << interval.max << "]\n";
map<string, Expr> vars;
vars["b"] = uint1(0);
int lane = 0;
Expr test_expr = extract_lane(test, lane);
Expr test_sol = simplify(substitute(vars, test_expr));
std::cerr << "Simplified " << test_expr << " to: " << test_sol << "\n";

->

un-simplified: [int32((uint32(uint1(0)) + ((uint32)0*(uint32)4294967261)))      , int32((uint32(uint1(0)) + ((uint32)1*(uint32)4294967261)))]
simplified: [0  , -35]
Simplified int32((uint1)b) to: 0

This bug is a result of inconsistent handling of UInt(32) -> Int(32) casts, which has defined behavior in the simplifier but was not properly handled in bounds inference.

Both bugs were fixed, and repeated running of the fuzzer has not revealed more inconsistencies yet.

While writing this fuzzer, I noticed that a lot of the fuzzers share code, and I think it would be very useful to have a shared set of fuzzing code, so I opened #5894 to make this a TODO.

@rootjalex rootjalex requested a review from abadams April 10, 2021 18:06
src/Bounds.cpp Outdated
@@ -257,7 +257,7 @@ class Bounds : public IRVisitor {
bool could_overflow = true;
if (to.can_represent(from) || to.is_float()) {
could_overflow = false;
} else if (to.is_int() && to.bits() >= 32) {
} else if (to.is_int() && to.bits() > 16 && !(from.is_uint() && from.bits() > 16)) {
// If we cast to an int32 or greater, assume that it won't
Copy link
Member

Choose a reason for hiding this comment

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

This comment now only applies to floats

@rootjalex
Copy link
Member Author

Failures seem to be due to the uint32 -> int32 "fix", investigating now

@rootjalex
Copy link
Member Author

Update: the second fix broke mirror_interior. After discussion with @abadams , I changed the simplifier to catch overflow on uint->int casts as signed_integer_overflow, and reverted that fix in bounds inference.

@rootjalex
Copy link
Member Author

The failures are also timeouts on the correctness tests. The added test takes ~15-20s on my machine - perhaps I should drop the test iteration count to 100, from 1000? @abadams

@abadams
Copy link
Member

abadams commented Apr 12, 2021

100 is better, but the timeouts are actually unrelated (something's up with one of the cuda tests under llvm 13). Good to land.

@rootjalex rootjalex merged commit 9cc17b4 into master Apr 12, 2021
@rootjalex rootjalex deleted the rootjalex/add_bounds_fuzzing branch April 12, 2021 17:24
frengels pushed a commit to frengels/Halide that referenced this pull request Apr 30, 2021
…alide#5895)

* add interval bounds fuzzer

* correct overflow checks in bounds inference

* catch uint32->int32 overflow in simplifier and revert bounds change
@alexreinking alexreinking added this to the v12.0.0 milestone May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We should fuzz test interval arithmetic in the same way as we do the simplifier
3 participants