-
Notifications
You must be signed in to change notification settings - Fork 830
Fuzzer mutation: Use GLB to find the type to replace with #8100
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
tlively
left a comment
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 wonder if it would be less new code to just use an element of the ValType lattice and use meet to update it: https://github.com/WebAssembly/binaryen/blob/main/src/analysis/lattices/valtype.h
| private: | ||
| // The greatest lower bound. As we go this always contains the latest value | ||
| // based on everything we've seen so far. | ||
| Type glb = Type::unreachable; |
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 should use Type::none as the initial value, since it behaves very similarly to a top type whereas Type::unreachable is the bottom type. (This still requires some special casing, since Type::getGLB does not treat Type::none as top; we should consider fixing that to make Type::getLUB and Type::getGLB more consistent with each other.)
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.
Unreachable happens to be the bottom type in the lattice, but in this API, unreachable is the "uninitialized / we have seen no reachable data yet" marker. We ignore inputs of unreachable, and we start in that state.
That is, we want to ignore unreachable data - it does not constrain us - so GLBFinder.note(unreachable) should do nothing, not turn us into the bottom.
Avoiding that is consistent with LUBFinder and seems like the most intuitive API here to me.
(For similar reasons, using ValType's meet would be more work here, I found as I tried that now.)
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.
But the GLB/meet of any type t with bottom (i.e. unreachable) is t, so you should get the "ignore unreachable" behavior for free, right? All you have to do is use none as the "uninitialized / we have seen no reachable data yet" marker.
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.
Wait, isn't it the opposite? Meet refines, towards the bottom, using the GLB. So the GLB/meet of any type t with bottom (i.e unreachable) is bottom (unreachable).
Here is that in the code:
binaryen/src/wasm/wasm-type.cpp
Lines 808 to 810 in 8e5f757
| if (!a.isRef() || !b.isRef()) { | |
| return Type::unreachable; | |
| } |
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.
Oh right, I was confused.
|
|
||
| using namespace wasm; | ||
|
|
||
| TEST(GLBsTest, Basics) { |
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 would be good to test the edge cases around Type::none and Type::unreachable as well.
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 added testing for one corner case, and assertions for others that it seems better to just disallow as invalid.
Mutation will find the necessary types of children, which can be more
general than the current child, and replace them. It did not handle
children with multiple constraints, however, like this:
The value here must be a subtype of the thing the
br_ifflows into,and also of the block it targets - the value is sent twice, effectively,
so it has two subtyping constraints.
Add a GLBFinder utility for this, parallel to LUBFinder.