- Notifications
You must be signed in to change notification settings - Fork 817
MemoryPacking: Handle non-empty trapping segments #6261
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
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.
Would it be worth continuing to split these possibly-trapping active segments, but ensure that their last chunks are not elided? Then the initialization of the last chunk (or a previous chunk) would trap iff the original segment would have trapped. Or would it be a problem that this would cause partial initialization to become visible before the trap? I always forget whether we care about side effects before a trap.
Interesting idea! I hadn't thought of that. I think this would be technically incorrect because of partial initialization. At least that was the case in the bulk memory proposal ("bounds checks are done before any writes") - I'm not sure offhand where to read the spec for active segments, but surely we didn't introduce a difference there..? However it might be an incorrectness we can live with - we already live with reordering traps, for example (which have different stack traces, which is observable). But I think it would be more complicated to do this idea - we'd need to track split-off parts of the original segment etc., and I think for basically no benefit - no real-world code has trapping segments (I hope). |
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.
LGTM. We definitely don't need to care about modules that definitely trap, but it's more realistic that there might be modules that use a global as the offset, in which case it would be unfortunate if we did not optimize. (Do shared libraries already do this?)
(memory 1 2 shared) | ||
(data (i32.const -1) "\00") |
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.
You can give these names to make the output more readable.
Dynamic linking uses imported offsets, correct. Maybe it's better to try to avoid any regression there, good point. We already have a flag for "zero memory initialized". If that also let us assume the segments did not trap, that would be enough here, but it is perhaps slightly awkward. (Traps never happen is the more directly relevant flag, but in general C++ can't use it.) |
I think this should actually be a fairly localized change to |
Hmm, I don't see how to do that only in binaryen/src/passes/MemoryPacking.cpp Lines 564 to 566 in b593849
That is where we skip zero ranges, but in your suggestion IIUC we'd keep the last zero in the range (to ensure we still trap). So I checked meanwhile and this doesn't regress any use case I can find:
|
It should be sufficient to have |
|
Basically |
Ok, done, see last commits. This is a little more complicated than I like given the amount of benefit, but maybe it's ok, and it is definitely future-proof in that it will help us if we split more segment types later. Note that I had to also add code in another place to handle overflows for this to work. |
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.
LGTM!
Followup to WebAssembly#6243 which handled empty ones.
Followup to #6243 which handled empty ones.