| Age | Commit message (Collapse) | Author |
|---|
|
Consider this code:
%% Start of block
get_tuple_element Tuple 0 Element
get_map_elements Fail Map [Key => Dest]
.
.
.
move Element UltimateDest
%% End of block
Fail:
%% Code that uses Element.
beam_block (more precisely, otp_tuple_element/1) would
incorrectly transform the code to this:
%% Start of block
get_map_elements Fail Map [Key => Dest]
.
.
.
get_tuple_element Tuple 0 UltimateDest
%% End of block
Fail:
%% Code that uses Element.
That is, the code at label Fail would use register Element,
which is either uninitalized or contains the wrong value.
We could fix this problem by always keeping label information
at hand when optimizing blocks so that we could check the code
at the failure label for get_map_elements. That would require
changes to beam_block and beam_utils. We might consider doing
that in the future if it turns out be worth it.
For now, I have decided that I want to keep the simplicity of blocks
(allowing them to be optimized without keeping label information).
That could be achieved by not including get_map_elements in
blocks. Another way, which I have chosen, is to only allow
get_map_elements as the first instruction in the block.
For background on the bug: c288ab8 introduced the beam_reorder pass
and 5f431276 introduced opt_tuple_element() in beam_block.
|
|
The expression in a bit string comprehension is limited to a
literal bit string expression. That is, the following code
is legal:
<< <<X>> || X <- List >>
but not this code:
<< foo(X) || X <- List >>
The limitation is annoying. For one thing, tools that transform
the abstract format must be careful not to produce code such as:
<< begin
%% Some instrumentation code.
<<X>>
end || X <- List >>
One reason for the limitation could be that we'll get
reduce/reduce conflicts if we try to allow an arbitrary
expression in a bit string comprehension:
binary_comprehension -> '<<' expr '||' lc_exprs '>>' :
{bc,?anno('$1'),'$2','$4'}.
Unfortunately, there does not seem to be an easy way to work
around that problem. The best we can do is to allow 'expr_max'
expressions (as in the binary syntax):
binary_comprehension -> '<<' expr_max '||' lc_exprs '>>' :
{bc,?anno('$1'),'$2','$4'}.
That will work, but functions calls must be enclosed in
parentheses:
<< (foo(X)) || X <- List >>
|
|
* bjorn/compiler/modernize-tests:
Remove ?line macros
Replace use of lists:keysearch/3 with lists:keyfind/3
Eliminate use of doc and suite clauses
Replace ?t with test_server
Replace use of test_server:format/2 with io:format/2
Eliminate use of test_server:fail/0,1
Eliminate use of ?config() macro
Modernize use of timetraps
Eliminate useless helper functions
|
|
|
|
|
|
Those clause are obsolete and never used by common_test.
|
|
The macro ?t is deprecated. Replace its use with 'test_server'.
|
|
There is no practial difference.
|
|
|
|
?config is ugly and not recommended. Use proplists:get_value/2
instead.
|
|
Either rely on the default 30 minutes timetrap, or set the timeout
using the supported methods in common_test.
|
|
Binary matching can be confusing. For example:
1> <<-1>> = <<-1>>.
** exception error: no match of right hand side value <<"ÿ">>
2>
When constructing binaries, the value will be masked to fit in
the binary segment. But no such masking happens when matching
binaries.
One solution that we considered was to do the same masking when
matching. We have rejected that solution for several reasons:
* Masking in construction is highly controversial and by some
people considered a bad design decision.
* While masking of unsigned numbers can be understood, masking of
signed numbers it not easy to understand.
* Then there is the question of backward compatibility. Adding
masking to matching would mean that clauses that did not match
earlier would start to match. That means that code that has
never been tested will be executed. Code that has not been
tested will usually not work.
Therefore, we have decided to warn for binary patterns that cannot
possibly match.
While we are it, we will also warn for the following example where
size for a binary segment is invalid:
bad_size(Bin) ->
BadSize = bad_size,
<<42:BadSize>> = Bin.
That example would crash the HiPE compiler because the BEAM compiler
would generate a bs_get_integer2 instruction with an invalid size
field. We can avoid that crash if sys_core_fold not only warns for bad
binary pattern, but also removes the clauses that will not match.
Reported-by: http://bugs.erlang.org/browse/ERL-44
Reported-by: Kostis Sagonas
|
|
andor_SUITE:check/2 function can be replaced with pattern matching.
|
|
Since no test suites includede test_server.hrl, there is no need
to have test_server in the include path or code path.
|
|
As a first step to removing the test_server application as
as its own separate application, change the inclusion of
test_server.hrl to an inclusion of ct.hrl and remove the
inclusion of test_server_line.hrl.
|
|
* bjorn/cuddle-with-tests:
erl_prim_loader_SUITE: Refactor helper functions
Move record compilation errors to erl_lint_SUITE
compile_SUITE: Use get_files/3 in more places
compile_SUITE: Replace confusing files/2 with get_files/3
|
|
* maint:
Eliminate crash because of unsafe delaying of sub-binary creation
|
|
The following code would fail to compile:
decode(<<Code/integer, Bin/binary>>) ->
<<C1/integer, B1/binary>> = Bin,
case C1 of
X when X =:= 1 orelse X =:= 2 ->
Bin2 = <<>>;
_ ->
Bin2 = B1
end,
case Code of
1 -> decode(Bin2);
_ -> Bin2
end.
The error message would be:
t: function decode/1+28:
Internal consistency check failed - please report this bug.
Instruction: return
Error: {match_context,{x,0}}:
The beam_bsm pass would delay the creation of a sub-binary when it was
unsafe to do so. The culprit was the btb_follow_branch/3 function that
for performance reasons cached labels that had already been checked.
The problem was the safety of a label also depends on the contents
of the registers. Therefore, the key for caching needs to be both
the label and the register contents.
Reported-by: José Valim
|
|
The two bad record usage test cases in compile_SUITE do not
belong there, as the errors are detected in erl_lint. Move the
test to the erl_lint_SUITE.
|
|
|
|
The files/2 function is confusing. The second argument names
the output directory, not the name of the source module. It is
common trap to attempt to point a different source file using
files/2.
Introduce the new get_files/3 which explicitly names the module
name.
|
|
* maint:
Fix crash when attempting to update a fun as if it were a map
|
|
The following example would cause an internal consistency
failure in the compiler:
f() -> ok.
update() -> (fun f/0)#{u => 42}.
The reason is that internally, v3_core will (incorrectly)
rewrite update/0 to code similar to this:
update() ->
if
is_map(fun f/0) ->
maps:update(u, 42, fun f/0)
end.
Since funs are not allowed to be created in guards, incorrect and
unsafe code would be generated.
It is easy to fix the bug. There already is a is_valid_map_src/1
function in v3_core that tests whether the argument for the map update
operation can possibly be a valid map. A fun is represented as a
variable with a special name in Core Erlang, so it would not be
recognized as unsafe. All we'll need to do to fix the bug is to look
closer at variables to ensure they don't represent funs. That will
ensure that the code is rewritten in the correct way:
update() ->
error({badmap,fun f/0})
end.
Reported-by: Thomas Arts
|
|
* maint:
Eliminate crash in v3_codegen
|
|
The following code would crash v3_codegen:
order(From) ->
catch
if
From#{[] => sufficient} ->
saint
end.
Before explaining the crash, first some background on the stack
frame and the Y registers.
Certain instructions, most notably the 'call' instructions, clobber
all X registers. Before any such instruction, all X registers that
have values that will be used after the call must be saved to Y
registers (i.e. to the stack frame). adjust_stack/4 will be called
when X registers must be saved.
There is also another situation when X registers must be saved, namely
within a 'catch' if we are about to execute any instruction that may
cause an exception. Examples of such instructions are some guard BIFs
(such as length/1) and construction of binaries or maps. Within a
'catch', X registers must be be saved because if an exception is
thrown and catched all X registers will be destroyed. The same
adjust_stack/4 function will be called for those instructions, but
only if they occur within a 'catch'.
There is actually one more complication. If there is code in
a guard within a catch, the X registers should not be saved, because
the code in a guard never clobbers any X registers that were alive
before the guard code was entered. v3_codegen is written with the
implicit assumption that code in guards never cause anything
to be saved to Y registers.
The code for building maps and binaries would incorrectly save X
registers within a guard inside a 'catch'.
For construction of binaries, that would mean that a useless but
harmelss 'move' instruction was generated.
But for construction of maps, the saving of the Y register would not
be harmless. There would be a crash when attempting to merge #sr{}
records. #sr{} records keeps track of the contents of X and Y
registers. When two separate code paths are joined (e.g. at the end of
'case' statement), the register descriptors must be reconciled.
Basically, the register descriptors for both paths must be identical.
The #sr{} record for one path must not claim that {y,0} contains
a certain value, while another path claims that {y,0} is dead.
Thus, the crash occurs in sr_merge/2 when failing to reconcile the
Y registers.
To fix this bug this bug we will introduce a new function called
maybe_adjust_stack/5. It will save X registers on the stack only
if the code is inside a catch but not inside a guard. We will
change all existing code to use this new function when appropriate.
Reported-by: Thomas Arts
|
|
* maint:
beam_bool: Fix unsafe optimization
|
|
beam_bool would make the following code unsafe (which would be
reported by beam_validator):
scotland(Echo) ->
found(case Echo of
Echo when true; Echo, Echo, Echo ->
Echo;
echo ->
[]
end,
Echo = placed).
found(_, _) -> million.
Basically, beam_bool would see that the 'case' would always return
the value of Echo. Thus:
scotland(Echo) ->
found(Echo, Echo = placed).
The only problem is that beam_bool would also remove a 'move'
instruction that would save Echo to the stack. Here is the
assembly code for part of the function:
{allocate_zero,1,1}.
{move,{x,0},{y,0}}. %% Save Echo on stack.
{bif,'=:=',{f,7},[{x,0},{atom,true}],{x,1}}.
{bif,'=:=',{f,7},[{x,0},{atom,true}],{x,2}}.
{bif,'=:=',{f,7},[{x,0},{atom,true}],{x,3}}.
{bif,'and',{f,7},[{x,2},{x,3}],{x,2}}.
{bif,'and',{f,7},[{x,1},{x,2}],{x,1}}.
{jump,{f,8}}.
{label,7}.
{move,{atom,false},{x,1}}.
{label,8}.
{bif,'or',{f,6},[{atom,true},{x,1}],{x,1}}.
{test,is_eq_exact,{f,6},[{x,1},{atom,true}]}. %% Jump never taken.
{jump,{f,5}}.
{label,6}.
{test,is_eq_exact,{f,9},[{x,0},{atom,echo}]}.
{move,nil,{x,0}}.
{jump,{f,5}}.
{label,9}.
{test_heap,3,0}.
{put_tuple,2,{x,0}}.
{put,{atom,case_clause}}.
{put,{y,0}}.
{line,[{location,"t.erl",5}]}.
{call_ext,1,{extfunc,erlang,error,1}}.
{jump,{f,5}}.
{label,5}.
{test,is_eq_exact,{f,12},[{atom,placed},{y,0}]}.
beam_bool would see that the is_eq_exact test at label 8 would
always succeed. It could therefore remove most of the code before
the jump to label 5. Unfortunately it also removed the essential
move of Echo to the stack:
{allocate_zero,1,1}.
%% Instruction incorrectly removed: {move,{x,0},{y,0}}.
{jump,{f,5}}.
{label,5}.
{test,is_eq_exact,{f,12},[{atom,placed},{y,0}]}.
The root cause of the problem is that the 'move' instruction is
included in the block of 'bif' instructions before label 8.
Normally the 'move' instruction would not have been discarded,
but because the left operand to the 'or' BIF is 'true', the
entire block with 'bif' instructions are dropped.
As far as I can see, there is no gain by including 'move'
instructions in the first place. There is no way that better
code will be produced. In fact, the entire optimization can
be given up if 'move' instructions are found in the block.
Thus we can fix this bug by never including any 'move' instructions
in the block of 'bif' instructions. We can also remove all the
code that deals with 'move' instructions within blocks.
Reported-by: Thomas Arts
|
|
|
|
* maint:
Fix missing filename and line number in warning
Conflicts:
lib/compiler/test/bs_match_SUITE.erl
|
|
When the 'bin_opt_info' is given, warnings without filenames
and line numbers could sometimes be produced:
no_file: Warning: INFO: matching non-variables after
a previous clause matching a variable will prevent delayed
sub binary optimization
The reason for the missing information is that #c_alias{} records lack
location information. There are several ways to fix the problem. The
easiest seems to be to get the location information from the
code).
Noticed-by: José Valim
|
|
|
|
|
|
|
|
c288ab87 added beam_reorder to move get_tuple_element instructions.
Compiling code such as the following would crash the compiler:
alloc(_U1, _U2, R) ->
V = R#alloc.version,
Res = id(V),
_ = id(0),
Res.
The crash would occur because the following two instructions:
{get_tuple_element,{x,2},1,{x,1}}.
{allocate_zero,1,2}.
were swapped and rewritten to:
{allocate_zero,1,1}.
{get_tuple_element,{x,2},1,{x,1}}.
That transformation is not safe because the allocate_zero instruction
would kill {x,2}, which is the register that is holding the reference
to the tuple. Only do the transformation when the tuple reference is
in an x register with a lower number than the destination register.
|
|
There is an optimization in beam_block to simplify a select_val
on a known boolean value. We can implement this optimization
in a cleaner way in beam_type and it will also be applicable
in more situations. (When I added the optimization to beam_type
without removing the optimization from beam_block, the optimization
was applied 66 times.)
|
|
The ASN.1 compiler often generates code similar to:
f(<<0:1,...>>) -> ...;
f(<<1:1,...>>) -> ....
Internally that will be rewritten to (conceptually):
f(<<B:1,Tail/binary>>) ->
case B of
0 ->
case Tail of ... end;
1 ->
case Tail of ... end;
_ ->
error(case_clause)
end.
Since B comes from a bit field of one bit, we know that the only
possible values are 0 and 1. Therefore the error clause can be
eliminated like this:
f(<<B:1,Tail/binary>>) ->
case B of
0 ->
case Tail of ... end;
_ ->
case Tail of ... end
end.
Similarly, we can also a deduce the range for an integer from
a 'band' operation with a literal integer.
While we are at it, also add a test case to improve the coverage.
|
|
In the future we might want to add more bit syntax optimizations,
but beam_block is already sufficiently complicated. Therefore, move
the bit syntax optimizations out of beam_block into a separate
compiler pass called beam_bs.
|
|
|
|
Make sure that all warnings produced when compiling the
test suite contains filenames and line numbers.
|
|
|
|
|
|
In 45f469ca0890, the BEAM loader started to use x(1023) as scratch
register for some instructions. Therefore we should not allow
x(1023) to be used in code emitted by the compiler.
|
|
When matching tuples, the pattern matching compiler would generate
code that would fetch all elements of the tuple that will ultimately
be used, *before* testing that (for example) the first element is the
correct record tag. For example:
is_tuple Fail {x,0}
test_arity Fail {x,0} 3
get_tuple_element {x,0} 0 {x,1}
get_tuple_element {x,0} 1 {x,2}
get_tuple_element {x,0} 2 {x,3}
is_eq_exact Fail {x,1} some_tag
If {x,2} and {x,3} are not used at label Fail, we can re-arrange the
code like this:
is_tuple Fail {x,0}
test_arity Fail {x,0} 3
get_tuple_element {x,0} 0 {x,1}
is_eq_exact Fail {x,1} some_tag
get_tuple_element {x,0} 1 {x,2}
get_tuple_element {x,0} 2 {x,3}
Doing that may be beneficial in two ways.
If the branch is taken, we have eliminated the execution of two
unnecessary instructions.
Even if the branch is never or rarely taken, there is the possibility
for more optimizations following the is_eq_exact instructions.
For example, imagine that the code looks like this:
get_tuple_element {x,0} 1 {x,2}
get_tuple_element {x,0} 2 {x,3}
move {x,2} {y,0}
move {x,3} {y,1}
Assuming that {x,2} and {x,3} have no further uses in the code
that follows, that can be rewritten to:
get_tuple_element {x,0} 1 {y,0}
get_tuple_element {x,0} 2 {y,1}
When should we perform this optimization?
At the very latest, it must be done before opt_blocks/1 in
beam_block which does the elimination of unnecessary moves.
Actually, we want do the optimization before the blocks have
been established, since moving instructions out of one block
into another is cumbersome.
Therefore, we will do the optimization in a new pass that is
run before beam_block. A new pass will make debugging easier,
and beam_block already has a fair number of sub passes.
|
|
|
|
|
|
regressions_SUITE will have code snippets which previously crashed the compiler.
This commits includes a test for Maps crash in beam_bool.
|
|
* bjorn/compiler/spurious-warning:
sys_core_fold: Eliminate warnings for unused terms in effect context
sys_core_fold: Eliminate warnings for unused terms
|
|
The optimization introduced in 0a0d39d351fc could cause spurious
warnings of the type: "a term is constructed, but never used".
That would happen for constructs in effect context.
To avoid those warnings, we will need to apply warning suppression
also in effect context.
|
|
|
|
The optimization introduced in 0a0d39d351fc would cause spurious
warnings of the type: "a term is constructed, but never used".
To avoid the warning, we must mark not only tuples and lists as
compiler_generated, but also each element. We must also propagate
compiler_generated annotations in lets. For example, if we have:
let <X -| ['compiler_generated']> = 42 in X + 1
we must propagate the compiler_generated annotation to the literal
when do constant propagation:
42 -| ['compiler_generated'] + 1
|
