Wasmtime GitHub notifications bot (Nov 02 2022 at 15:50): Wasmtime GitHub notifications bot (Nov 02 2022 at 15:50):afonso360 commented on issue #5167:
Added the above, turns out our testsuite is full of out of bounds loads and stores :upside_down:, even more than the zero sized accesses.
I've reverted the cases where we do a heap_addr of 0 bytes but then never use that pointer.
One thing that jumped at me was that some of our optimization test cases regressed when we used a non 0 byte heap_addr. This one in particular I think shouldn't have regressed. We may want to follow up on that.
Wasmtime GitHub notifications bot (Nov 02 2022 at 15:51):afonso360 edited a comment on issue #5167:
(I'm reusing this PR since it has a bit more context, instead of opening a new one. Hope that's okay!)
Added the above, turns out our testsuite is full of out of bounds loads and stores :upside_down:, even more than the zero sized accesses.
I've reverted the cases where we do a heap_addr of 0 bytes but then never use that pointer.
One thing that jumped at me was that some of our optimization test cases regressed when we used a non 0 byte heap_addr. This one in particular I think shouldn't have regressed. We may want to follow up on that.
Wasmtime GitHub notifications bot (Nov 02 2022 at 17:01):cfallin commented on issue #5167:
I actually have a slightly different opinion here: we should not make IR validation contingent on static analysis of offsets. This is a separation-of-concerns issue in my mind: the allowed IR should be defined separately from how powerful we can make our analyses (or in other words, IR should not be disallowed because "the compiler can't prove X"). That is a recipe for very confusing issues where validation state flips back and forth as optimizations change code.
For a very concrete example, removing redundant blockparams could allow an analysis to see further; it could then suddenly observe a statically-known-to-be-invalid combinatino of size and offset; and so we have an optimization pass that takes legal IR and produces illegal IR, according to our validator.
In general, the legality of loads/stores is a dynamic property, not a static (type-like) property, and for consistency we should keep it that way, I think; anything else causes confusing and unintuitive behavior.
Wasmtime GitHub notifications bot (Nov 02 2022 at 18:02):afonso360 commented on issue #5167:
For a very concrete example, removing redundant blockparams could allow an analysis to see further; it could then suddenly observe a statically-known-to-be-invalid combinatino of size and offset; and so we have an optimization pass that takes legal IR and produces illegal IR, according to our validator.
Explained that way, I totally agree!
It still feels like we should be able to provide these sorts of checks to IR producers. What would you think about adding this behind a separate flag that tries to do more advanced checks in general?
Wasmtime GitHub notifications bot (Nov 02 2022 at 18:02):afonso360 edited a comment on issue #5167:
For a very concrete example, removing redundant blockparams could allow an analysis to see further; it could then suddenly observe a statically-known-to-be-invalid combinatino of size and offset; and so we have an optimization pass that takes legal IR and produces illegal IR, according to our validator.
Explained that way, I totally agree!
It still feels like we should be able to provide these sorts of checks to IR producers. What would you think about instead adding this behind a separate flag that tries to do more advanced checks in general?
Wasmtime GitHub notifications bot (Nov 02 2022 at 18:09):cfallin commented on issue #5167:
It still feels like we should be able to provide these sorts of checks to IR producers. What would you think about instead adding this behind a separate flag that tries to do more advanced checks in general?
Possibly, yeah; a "lint" level of warning, operating sort of like classical static analysis tools, could be useful. As long as we state that the results are not hard errors (i.e., are not part of the definition of "correct IR"), that seems fine to me.
Wasmtime GitHub notifications bot (Nov 24 2022 at 15:47):afonso360 commented on issue #5167:
We discussed this PR yesterday in the Cranelift weekly meeting and reached the conclusion that we probably get the same coverage as this by adding heaps to fuzzgen and checking accesses in the interpreter (which we already do!).
Last updated: Oct 23 2024 at 20:03 UTC