khepri_fun: propagate argument annotations to callee in bs_no_start_match2 validation failures #63
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 419
💛 - Coveralls |
dumbbell
requested changes
Mar 18, 2022
the-mikedavis
force-pushed
the
md-propagate-comments-forward
branch
from
772c633 to
ec87b2d
Compare
the-mikedavis
commented
Mar 18, 2022
These test case functions are almost the same as the original
parse_float and parse_digits functions, except that they introduce
extra intermediary calls of parse_digits2 with additional bindings
prepended to the parameter list. These new bindings throw off the
arity of the bitstring match parameter `Bin` for which the original
validation fix tested by `allowed_bs_match_digit_parser_test` was
hard-coded.
These test cases also reveal some interesting behavior when we play
with the current code for fixing this validation error. This is the
original code which hard-codes a fix for a 1-arity function missing
the annotation on register {x,0} (the only argument):
handle_validation_error(
Asm,
{_FailingFun,
{{Call, 1 = _Arity, {f, EntryLabel}},
_,
no_bs_start_match2}},
Error) when Call =:= call orelse Call =:= call_only ->
Register = {x, 0},
Comment = {'%', {var_info, Register, [accepts_match_context]}},
Location = {'after', {label, EntryLabel}},
add_comment_and_retry(Asm, Error, EntryLabel, Location, Comment);
Let's add a clause for a 2-arity function and hard-code the register
to {x,1}:
handle_validation_error(
Asm,
{_FailingFun,
{{Call, 2 = _Arity, {f, EntryLabel}},
_,
no_bs_start_match2}},
Error) when Call =:= call orelse Call =:= call_only ->
Register = {x, 1},
Comment = {'%', {var_info, Register, [accepts_match_context]}},
Location = {'after', {label, EntryLabel}},
add_comment_and_retry(Asm, Error, EntryLabel, Location, Comment);
Initially, I expected this case to cover the new `parse_digits2/2` but
not `parse_digits2/3`. But In fact, `parse_digits2/3` needs no
annotations. This means that in order to fix this case, we don't need
to reach into the call-graph of `parse_digits2` and determine the
typings of the functions it calls. Instead we need to propagate the
`accepts_match_context` typing from `parse_float2/1` (`parse_digit2`'s
only caller).
This is good news because the validation error data does not include
any information about the eventual call to `parse_digits2/4`. We would
need to reach down the entire call-graph from `parse_float2/1` to
determine the types. Instead, the validation error contains enough
context to add the necessary annotations without ambiguity (see the
next commit).
(See the parent commit for some a primer on the problem and test
cases.)
This commit is refactors a fix for a `beam_validator` failure we
are currently solving by hard-coding a register. In the test case:
parse_float2(<<".", Rest/binary>>) ->
parse_digits2([], Rest);
parse_float2(Bin) -> {[], Bin}.
parse_digits2(Foo, Bin) ->
parse_digits2(Foo, [], Bin).
parse_digits2(Foo, Bar, Bin) ->
parse_digits2(Foo, Bar, Bin, []).
parse_digits2(
Foo, Bar, <<Digit/integer, Rest/binary>>, Acc)
when is_integer(Digit) andalso Digit >= 48 andalso Digit =< 57 ->
parse_digits2(Foo, Bar, Rest, [Digit | Acc]);
parse_digits2(_Foo, _Bar, Rest, Acc) ->
{lists:reverse(Acc), Rest}.
Prior to this commit, `beam_validator` gave the following validation
error:
{{'kfun__tx_funs__-allowed_bs_match_digit_parser2_test/0-fun-0-__33998213',
tx_funs__parse_float2,1},
{{call_only,2,{f,7}},10,no_bs_start_match2}}}
Which happens because `parse_float2/1` calls `parse_digits2/2`
with an argument which is annotated as accepting a match context,
but the parameter in `parse_digits2/2` is not yet annotated as
accepting a match context.
In order to fix this scenario, we need to determine which parameter
of `parse_digits2/2` needs to accept a match context, since that
information is not available in the `beam_validator` failure message.
Lets look at the disasm for that function:
{function,tx_funs__parse_float2,1,4,
[{label,3},
{line,
[{location,
"/home/michael/dev-tools/code/khepri/test/tx_funs.erl",
279}]},
{func_info,
{atom,
'kfun__tx_funs__-allowed_bs_match_digit_parser2_test/0-fun-0-__33998213'},
{atom,tx_funs__parse_float2},
1},
{label,4},
{'%',{var_info,{x,0},[accepts_match_context]}},
{bs_start_match4,{atom,no_fail},1,{x,0},{x,0}},
{test,bs_match_string,
{f,5},
[{x,0},8,{string,<<".">>}]},
{move,{x,0},{x,1}},
{move,nil,{x,0}},
{call_only,2,{f,7}},
{label,5},
{bs_get_tail,{x,0},{x,0},1},
{test_heap,3,1},
{put_tuple2,{x,0},{list,[nil,{x,0}]}},
return]}
We want to pull forward the annotation (5th instruction, 1-indexed)
for `{x,0}` to the definition of the function being called ({f,7}).
First we lookup the function code for the failing function by Name
and Arity (`find_comments_for_call/4`), then determine the typing
of the branch in which the call occurs: in this case by scanning the
instructions between {label,4} (the start of the branch) and
instruction 10 (the call_only; we get this index from the
`beam_validator` failure message), gathering annotations and
following {move,Src,Dst} instructions.
Once we have the annotations leading up to the call, we can add
them to right after the callee's entrypoint. The `merge_comments/2`
and `add_comments_to_code/4` functions have been refactored to
allow merging in multiple comments at once, which may be necessary
if we're propagating the typing for multiple parameters.
This function is useful for finding a function by name and arity (if defined) in a list of functions. We get a list of functions either from the asm of the module being generated or from the disasm from an existing module being disassembled, so this function can be used by both `handle_validation_error/3' and `lookup_function/4`.
the-mikedavis
force-pushed
the
md-propagate-comments-forward
branch
from
1c28f1b to
0821c6d
Compare
dumbbell
approved these changes
Mar 21, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow up for removing the hard-coded register
FIXME(#49 (comment)). Sorry the title is so long and jargon-y 😅A simplification of the problem is that function
Acalls functionBcalls functionC.Apasses an argument toBthataccepts_match_context, butBdoesn't have any annotations for its parameters, so thebeam_validatoremits an error on the call instruction saying:no_bs_start_match2.I got a bit derailed looking into this by function
C...In the test case fixtures,
Chas a parameter thataccepts_match_context, so I thought that in order to fix this, we would need to determine the types of each function in the call graph (which naively seems easy based onbeam_validator:build_function_table/2). It turns out though that this is not the case (see the commit message for edf1e98 for some more details on this).In order to handle this particular
beam_validatorfailure, we detect the annotations within the branch of the failed call instruction and add those annotions to the callee right after its entrypoint.