beam_ssa_private_append: Fix crash on oddly structured code #7143
Conversation
The private append pass didn't expect to append anything to literals other than `<<>>`. Rare interactions with the type or bool passes could sneak in things like atoms in unexpected places, crashing the pass.
CT Test Results 2 files 296 suites 11m 55s ⏱️ Results for commit d50da87. ♻️ 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 |
| @@ -565,6 +565,8 @@ patch_literal_term(<<>>, self, Cnt0) -> | |||
| {V,Cnt} = new_var(Cnt0), | |||
| I = #b_set{op=bs_init_writable,dst=V,args=[#b_literal{val=256}]}, | |||
| {V, [I], Cnt}; | |||
| patch_literal_term(Lit, self, Cnt) -> | |||
There was a problem hiding this comment.
This fixes the symptom, but it's not the right fix as we should already have discovered that this isn't possible, i.e. add_literal/3 should never get to add the "patchee" to the set of literals to patch.
For the work I'm currently doing I have refactored add_literal/3 to do a validity check, so we never add something which later on turns out to be impossible. I could try to backport that work, but as it's a larger patch, this is probably correct the conservative thing to do for R26.
There was a problem hiding this comment.
I'd rather keep things small this late in the release process, but then again it might end up biting us later. How large do you think the add_literal/3 change would be?
There was a problem hiding this comment.
It will require adding additional arguments to add_literal/3 and thus changing all call sites.
Considering that the only drawback of your fix is that we may keep too many literals around and spend some time trying to patch them before giving up, I'm perfectly happy with your patch.
The no-non-patchable-literals refinement can wait until R27 and the added clause will show up as not visited while running coverage, so there is little risk that it will remain when merging the refined stuff.
There was a problem hiding this comment.
Thanks, I'll go ahead with this then :)
jhogberg
added
the
testing
The private append pass didn't expect to append anything to literals other than
<<>>. Rare interactions with the type or bool passes could sneak in things like atoms in unexpected places, crashing the pass.@frej