preemptively infer var_info annotations#49
Conversation
Given some code like the following:
trim_leading_dash1(<<$-, Rest/binary>>) -> trim_leading_dash1(Rest);
trim_leading_dash1(Binary) -> Binary.
The compiler will add an annotation for the {x,0} register
{'%',{var_info,{x,0},[accepts_match_context]}}.
This annotation is not available in the disasm, so when we recompile
this function, the compiler gives an error.
This change detects which x-register should accept the match context
by peeking the first instruction after the entrypoint label. This
instruction can either be a {test,bs_start_match3,..} instruction or
bs_start_match4 (if the compiler can determine that the function will
_always_ be passed a binary), both of which contain the register
which will be matched.
closes rabbitmq#44
see also rabbitmq#36
This separates out the function clauses for handling failures from beam_validator, which allows us to write clauses for validator failures without duplication (because of the different shape of compiler errors between Erlang 23 and 24). This commit also fixes the prior commit's compatibility with Erlang 23.
I don't have a good test case for this but it should be possible to have this fail when the function does more than just call another function and then return.
In many cases we can infer the necessary typing of a variable based on
an instruction in a function body. For example, when we see
{get_tuple_element, Src, Element, _Dst}
We know that the register Src must contain a tuple with at least
`Element + 1' elements (if it weren't, the code could not have been
initially compiled), so we can add
{'%', {var_info, Src, [{type, {t_tuple, Element + 1, false, #{}}}]}}
To satisfy the beam_validator's type-checks. This commit removes the clause
of `handle_validation_error/3' which would correct these failures when
`compile:forms/2' would emit them: that clause no longer matches.
| {{Call, 1 = _Arity, {f, EntryLabel}}, | ||
| _Index, | ||
| no_bs_start_match2}}, | ||
| _Error) when Call =:= call orelse Call =:= call_only -> | ||
| %% TODO: hard-coded register | ||
| Comment = {'%', {var_info, {x, 0}, [accepts_match_context]}}, |
There was a problem hiding this comment.
This is currently hard-coded to work for a 1-arity function so that we know which register to annotate. I think to know which register to use we may need to track the typing of functions which might be difficult. I'll think about it some more 🤔
parse_float/1 starts with a bs_start_match4 instruction that needs to both accept match contexts and must be typed as a t_bitstring in order to satisfy the beam_validator. t_bitstring cannot be inferred in pass1_process_instructions/3 (see the parent commit), so it needs to be handled after it is noticed in the error of compile:forms/2. See the child commit for the necessary changes to compile parse_float/1 in a khepri_fun.
d8b06f1 to
86bf6fe
Compare
Pull Request Test Coverage Report for Build 355Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
It seems like a lot of work to try to reconstruct all of the annotations beam_validator needs and I think these annotations end up only being interpreted by beam_validator. Maybe we should propose adding an option to edit: the change would be very small ( |
Right. But even with this option, I think I prefer to get those issues and validation errors to make sure the code is correct. More importantly, if an instruction such as |
dumbbell
left a comment
dumbbell
left a comment
There was a problem hiding this comment.
Thank you for working on this! I submitted more inline comments.
|
(I'll squash the 'fix variable name...' commit before taking this out of draft status) edit: squashed! ✔️ |
The implementation of add_comment_to_code/5 prior to this commit
placed comments into function code immediately following the function's
entry-point label, which is correct in most scenarios. Some validation
failures may need comments at other points in the function code, though.
Luckily, beam_validator points to the location of the failing instruction
in the failure triple. So this commit allows one to pass the location where
the comment should go end up in the function code.
Included with this new structure are two validation-failure handling clauses:
one may add type information to a bs_start_match4 instruction when the
validator is not satisfied by the current lack of typing. (I don't believe
you can infer the typing of a bs_start_match4 easily, we'd have to look
into the validator to see how it's detecting the 'needed' type.) The other
is an interesting case where an intermediary function needs an
'accepts_match_context' annotation. In this case, function A calls function
B with some variable that accepts a match context. Function B then calls
function C, which accepts a variable that must be a match context. Functions
A and C may have their variables properly annotated, but function B does not,
because it does not contain any bs_start_match* instructions. It may look
something like:
{function, 'B', 1, 30,
[
{label, 29},
{func_info, {atom, my_module}, {atom, 'B'}, 1},
{label, 30},
{call_only, 1, {f, 32}} %% function 'C'
]}
So in this case we ignore the FailingFun and instead look at the entry-label
in the failing call/call_only label and use that to find the function. Then
we annotate the {x,0} variable in that function to accept a match context,
which satisfies the validator.
This is currently hard-coded for a 1-arity function. More testing will be
needed to find an example of this where we can't blindly assume {x,0}.
add_comment_to_code/4 now takes Location as a pair
{Placement :: before | 'after', Instruction :: tuple() | atom()}
And will use the instruction as the insertion point, placing the
annotation before or after it. This resolves some awkwardness from
using Location as an (1-indexed) index in the list of instructions of
a function. The instruction index works but is brittle when hard-coded
as it was for the `no_bs_start_match2' instruction. For example if
line instructions were not discarded, it could be impossible to tell
where in the instruction list the annotation should go.
Here we peek at the next instruction in either Result or Rest
(depending on Placement) to find an existing comment if there is
one and then we discard the existing comment after merging.
`beam_validator' currently accepts duplicate annotations for a
types: it discards all but the last annotation before a variable
is checked. For example if we were fixing the typing of a
`bs_start_match4`, we might end up with:
{'%', {var_info, {x, 0}, [accepts_match_context]}},
{'%', {var_info, {x, 0}, [{type, {t_bs_context,1,0,0}},
accepts_match_context]}},
{bs_start_match4,{atom,resume},2,{x,0},{x,0}}.
Currently `beam_validator` behaves as though it discards the first
annotation. This behavior is not documented, though, so we protect
against a future where `beam_validator' stops handling duplicates
like this by ourselves discarding the existing annotation.
Here we avoid an infinite loop of compilation that can happen when we attempt to fix a compilation error by adding an annotation and that annotation already exists where we expect. This catches the case where we expect an annotation to fix a compilation error, we add the annotation, and then the compiler fails with the same error.
b9f5b0b to
2403ae6
Compare
|
I think it makes sense to tackle the 'TODO' for the hard-coded register (this) in a separate PR: it will probably be a decently large change on its own. What do you think? |
Yes, I agree, let's see how it goes with this patch already. Again, thank you for this great improvement! |
3 participants
supersedes #45
closes #44
This PR:
pass1_process_instructions/1accepts_match_contextannotations forbs_start_match*instructionscompile:forms/2failureThis is still WIP while I figure out a good way to handle the hard-coded register 🤔