-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 crash in beam_ssa_private_append #8633
Conversation
With the right input, the beam_ssa_private_append pass could crash when it, during the initial value tracking phase, selected literals which were not compatible with the tracked element structure for later patching. This patch ensures that only compatible literals are scheduled for later patching. This crash is only present in OTP versions >= 26 && <= 27 and this fix should not be merged to master as dce585f ("compiler: In-place update of tuples/records"), merged for OTP 27, includes essentially the same functionality. Closes erlang#8630
CT Test Results 2 files 296 suites 10m 49s ⏱️ Results for commit 6b00105. ♻️ 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 |
@sverker, I don't have permissions to set labels, but shouldn't this get the "testing" label so it runs in nightly tests, even though its for maint-26? |
maint-26 is currently locked as someone is making a 26 patch release. So, I changed to PR to be on maint and set the testing label. |
I can add it manually to the maint-26 testing as well, after the current release work is done, if you don't have access to our lab machines. |
Ok, that did not work. It conflicted with 1f33624 which was merged into OTP-27.0 and is part of both maint and master. I can put it into maint-26 testing when it opens up. |
@IngelaAndin is planning a 26 patch tomorrow. So, if you (@frej) determine the risk/urgency ratio is low enough I could add it to maint-26 testing for tonight and to be released tomorrow. |
I think that easiest way to handle patches is to, if possible base them on a green released version, and then most of the time the same branch can be run in both maint-XX track and maint (opu) and master builds to maximize test coverage. Sometimes it is of course not possible. |
Thanks for the PR! I'll add it to testing once the patch is released, and merge to |
@sverker: I don't know how the labels work, and I don't have access to any internal systems. @sverker, @jhogberg and @IngelaAndin: as it says in the commit message, the patch can only go to the 26 branch, and should not be propagated to maint as it cannot be merged into master as, in master (and for that matter 27), the functionality this PR corrects has been superseded by a new better (and correct) implementation. I asked @bjorng how I should create the PR and he told me to base it on OTP-26.2 and target maint-26. The bug was this PR corrects was discovered over a year after 26 was released and it can be avoided by just disabling the offending pass. The fix matches, in functionality if not in implementation, what's in 27, so I'd say its both low risk and low urgency. |
Yes, you've done everything correctly. I'll take it from here :) |
With the right input, the beam_ssa_private_append pass could crash when it, during the initial value tracking phase, selected literals which were not compatible with the tracked element structure for later patching. This patch ensures that only compatible literals are scheduled for later patching.
This crash is only present in OTP versions >= 26 && < 27 and this fix should not be merged to maint or master as dce585f ("compiler: In-place update of tuples/records"), merged for OTP 27, includes essentially the same functionality.
Closes #8630