-
Notifications
You must be signed in to change notification settings - Fork 3k
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
compiler: Fix skip clause for binary generators #8978
base: maint
Are you sure you want to change the base?
compiler: Fix skip clause for binary generators #8978
Conversation
When the pattern in a binary generator is of the form `X:Y/float`, the previous skip clause could never match. The skip pattern is changed to `_:Y/integer`, so that it can match as long as `Y` is valid. Before: 1> BadFloat = <<-1:64>>, [X || <<X:64/float>> <= <<BadFloat/binary, 1.0:64/float>>]. [] Now: 1> BadFloat = <<-1:64>>, [X || <<X:64/float>> <= <<BadFloat/binary, 1.0:64/float>>]. [1.0]
CT Test Results 4 files 435 suites 1h 14m 9s ⏱️ Results for commit 61bf342. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
@@ -1986,6 +1986,14 @@ append_tail_segment(Segs, St0) -> | |||
%% in the skip clause that will continue the iteration when | |||
%% the accumulator pattern didn't match. | |||
|
|||
skip_segments([#ibitstr{val=#c_var{},type=#c_literal{val=float}}=B|Rest], St, Acc) -> | |||
skip_segments(Rest, St, [B#ibitstr{type=#c_literal{val=integer}}|Acc]); |
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.
To make this consistent with how other skips are made, we ought to check that the size is 0, 16, 32, or 64 bits.
Unfortunately this will also kind of lock us into not supporting other float sizes, as a module is not supposed to behave differently when loaded in a newer compatible emulator (Edit: or at the very least do so consistently -- if we support 128-bit floats later, a variable-size match could behave differently on matches and skipping), but I think we can live with that. @bjorng?
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.
To make this consistent with how other skips are made, we ought to check that the size is 0, 16, 32, or 64 bits.
But what should we do if the size is not one of the supported sizes? Not skipping anything is not option, because that would cause an infinite loop. Crashing would work, but I don't think we should do that for the relaxed operator.
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.
If it's not one of the supported sizes, we'd bail out just like with all the other skips that go wrong "twice" (e.g. unicode) to keep things consistent.
I'm not super-sure this is a good idea in general however.
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 mean discarding the rest of the binary and returning whatever we have collected so far?
That would produce a nonsensical result. So would this PR.
When the size is nonsensical, do we really care about the exact nature of the nonsense we produce or how fast we can produce it?
The disadvantage of testing the size of the segment is that complicates the compiler (and evaluators), increases the size of the generated code, and slows down the success case. There is also the disadvantage you mentioned.
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 mean discarding the rest of the binary and returning whatever we have collected so far?
That would produce a nonsensical result. So would this PR.
Yes, however, it would be consistent with how the other patterns work, and the result would be less nonsensical in the sense that we'd produce a smaller amount of it. 😄
Let's skip the check. 🙂
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.
Looks good to me. I see no need to add any checks for legal segment sizes.
When the pattern in a binary generator is of the form
X:Y/float
, the previous skip clause could never match. The skip pattern is changed to_:Y/integer
, so that it can match as long asY
is valid.Before:
Now: