wasmparser: Remove offset param in VisitOperator #804
Conversation
| self.validator.with_resources(&self.resources) | ||
| .$visit(offset $($(,$arg)*)?) | ||
| fn $visit(&mut self $($(,$arg: $argty)*)?) -> Result<()> { | ||
| // TODO: whatever happens if the caller does not set `op_offset` ahead of time? |
There was a problem hiding this comment.
The public VisitOperator implementation on this type is problematic. The validation today depends on offsets always getting set-up correctly for more than just the error messages. In particular it sets self.end_which_emptied_control to the offset of the very last end for the function, which is then checked in the finish method.
On the other hand, it is possible for the users to call reader.visit_operator(func_validator) while forgetting to set-up the op_offset. This is distinct from FuncValidator::op which does the right thing.
I would say resolving this will require to remove or move this implementation to some other type which forces callers to set up the offset one way or another, much like how with_resources below does.
Maybe something like
fn visit_operator(&self, reader: BinaryReader) -> ... { }
There was a problem hiding this comment.
Hm yeah that is indeed unfortunate. It's fine to implement the end_which_emptied_control bit entirely differently, that was just the first thing I reached for.
One possibility, though, could perhaps be a default-does-nothing trait method which is along the lines of "I'm about to start visiting this offset", which could be called at the start of visit_operator and ignored by almost everything except the validator?
There was a problem hiding this comment.
Yeah, adding a visit_offset or such to the VisitOperator is an option. It is an obvious way forward, but it isn’t clear to me if it is the best way forward quite yet.
I want to give it some thought and maybe experiment with them to see how things work out if done differently.
There was a problem hiding this comment.
I ended up with what I think is a simpler approach for now of adding a visitor method to the FuncValidator which returns an opaque impl VisitOperator. This is basically what all of the methods were delegating to anyway.
nagisa
force-pushed
the
remove-offsets
branch
from
36f4db8 to
5fa2c9f
Compare
|
The main motivation behind the |
Implementing `VisitOperator` is complicated by the additional passing-through of the `offset` everywhere. A much simpler alternative code wise is to stash the `offset` away within the implementation of `VisitOperator` before invoking `SomeReader::visit_operator`. This raises some complications with visitors that depend on the offsets and rely on them for functional correctness, though. This is the case for the `FuncValidator` which uses offsets to verify that the very last `end` operator seen is at the same offset as the reader when the validator is finalized.
This type depends on `offset`s being passed in correctly, that’s how it validates whether the last `End` operator is actually the end of the function. Prior to removal of the `offset` parameters, they’d be correct by the definition of `reader` passing the offset in. However, with move of this responsibility to the user code, it now became straightforward to invoke the visitor methods in a way that violates the assumptions in the implementation of the `FuncValidator`. Instead, in order to obtain a visitor, users are now required to call a method which also sets up the offsets accordingly.
nagisa
force-pushed
the
remove-offsets
branch
from
5fa2c9f to
510ab04
Compare
I definitely am aware of that. I wouldn’t have proposed this change if my intuition didn’t suggest the different ways of passing the offset around didn’t meaningfully change the perf characteristics of the code. My dev machine isn’t really set-up for quality benchmarking, with a ton of stuff running in there, but here are the results: the benchmark runner is indeed reporting some regressions over the baseline, however where there are regressions, they are definitely still within run-to-run variance on my machine, as running the same benchmark will readily report an improvement of about as many percent. |
|
This seems reasonable to me and I like the way it's integrated with For benchmarking using |
|
I think this is ready to merge at this point. I wasn’t thinking of making any significant additional changes in this PR, except for addressing any feedback. |
Implementing
VisitOperatoris complicated by the additional passing-through of theoffseteverywhere. A much simpler alternative code wise is to stash theoffsetaway within the implementation ofVisitOperatorbefore invokingSomeReader::visit_operator.This raises some complications with visitors that depend on the offsets and rely on them for functional correctness, though. This is the case for the
FuncValidatorwhich uses offsets to verify that the very lastendoperator seen is at the same offset as the reader when the validator is finalized.