Skip to content
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

[erlc] miscompilation with optimizations turned off #7248

Closed
RobinMorisset opened this issue May 16, 2023 · 4 comments · Fixed by #7281
Closed

[erlc] miscompilation with optimizations turned off #7248

RobinMorisset opened this issue May 16, 2023 · 4 comments · Fixed by #7281
Assignees
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM

Comments

@RobinMorisset
Copy link
Contributor

Describe the bug
On the following code:

f() ->
    try _V6 = (_V5 = bit_size(erlang:iolist_to_binary("a"))) rem 1 of
        _ ->
            _V5;
        _ ->
            _V6
    catch
        _ ->
            ok
    end.

wrapper0() ->
    io:write(f()).

Running it with the following commands:

erlc -W0 ~/minimized/test62768.erl
erl -pa . -noshell -s test62768 wrapper0 -s init stop

results in 8 being printed to the shell. But adding +no_bool_opt +no_ssa_opt to erlc makes it output 0 instead.

Expected behavior
I believe that 8 is the valid result:

  • first _V5 is set to 8
  • then _V6 is set to 8 rem 1 which is 0
  • then _V6 is matched against the first pattern, which is _ so it is a match
  • and so _V5 is returned.

Affected versions

Additional context
The following were tried and make the bug disappear:

  • replacing the try/of/catch/end by case/of/end
  • replacing the whole computation of _V5 by _V5 = 8
  • only passing one of the two flags to erlc
@RobinMorisset RobinMorisset added the bug Issue is reported as a bug label May 16, 2023
@bjorng bjorng self-assigned this May 17, 2023
@bjorng bjorng added the team:VM Assigned to OTP team VM label May 17, 2023
bjorng added a commit to bjorng/otp that referenced this issue May 17, 2023
This fixes erlang#7248, but it might not be the best or most elegant
fix.
@bjorng
Copy link
Contributor

bjorng commented May 17, 2023

Yes, 8 is the correct result.

There is a bug in the register allocator in beam_ssa_pre_codegen. The bug is in the handling of unused phi nodes. Here is part of the output when the dprecg option is used:

3:
  %% _4: 24..24
  x0/_4 = phi { x1/_V6, ^10 }

  %% _V5: 24..27
  x0/_V5 = phi { x0/_2, ^10 }

  %% _17: 24..24
  x0/_17 = phi { x1/_V6, ^10 }

  %% @ssa_ignored: 25..25
  [25] z0/@ssa_ignored = kill_try_tag y0/@ssa_catch_tag
  %% Anno: #{deallocate => 1}
  [27] ret x0/_V5

Here the values of all three phi nodes end up in the same BEAM register (x0). The correct one (the middle one) is overwritten by the third phi node.

This bug it not normally exposed, because there are optimizations that remove assignments to variables that are never used, and there is another optimization that eliminates singled-valued phi nodes. The bug disappears when case is used because a case does not introduce the single-valued phi nodes in the first place.

There are several possible ways to fix this bug. I will do some more thinking before I create a pull request.

@RobinMorisset
Copy link
Contributor Author

Here is another miscompilation that seems fairly similar:

f(_V0) ->
    try not (_V3 = (ok >= _V0)) of
        _V3 -> iolist_size(maybe 
            [] ?= _V3,
            <<>> ?= list_to_binary(ok)
        end);
        _ -> ok
    after
        ok
    end.

wrapper0() -> 
	io:write(catch f(ok)).

It prints ok by default (which I believe to be correct), but with +no_bool_opt +no_ssa_opt it prints {'EXIT',{badarg,[{erlang,iolist_size,[true], ....

master...bjorng:otp:bjorn/compiler/beam_ssa_pre_codegen/GH-7248 does not fix it, although it fixes the original testcase.

@RobinMorisset
Copy link
Contributor Author

And another one:

f(_V1) ->
    try erlang:bump_reductions(_V1) of
        _V3 ->
            try not (_V4 = (_V3 andalso is_number(ok))) of
                _V4 ->
                    (_V5 = ok) andalso ok;
                _ ->
                    _V4
            catch
                _ ->
                    ok
            end
    after
        ok
    end.

wrapper0() ->
    io:write(catch f(2147483647)).

It prints false by default, but with +no_bool_opt +no_ssa_opt, it prints {'EXIT',{{badarg,ok}, ...

bjorng added a commit to bjorng/otp that referenced this issue May 24, 2023
With optimizations disabled, the register allocator in
`beam_ssa_pre_codegen` could allocate different variables to the
same register if single-valued phi nodes in a try/catch construct
were present.

Closes erlang#7248
@bjorng
Copy link
Contributor

bjorng commented May 24, 2023

Turned out that all single-valued phi nodes are problematic. The linked pull request has a more general solution (which is also simpler) than my previous attempt.

bjorng added a commit to bjorng/otp that referenced this issue May 24, 2023
With optimizations disabled, the register allocator in
`beam_ssa_pre_codegen` could allocate different variables to the
same register if single-valued phi nodes in a try/catch construct
were present.

Closes erlang#7248
bjorng added a commit to bjorng/otp that referenced this issue May 26, 2023
With optimizations disabled, the register allocator in
`beam_ssa_pre_codegen` could allocate different variables to the
same register if single-valued phi nodes in a try/catch construct
were present.

Closes erlang#7248
bjorng added a commit to bjorng/otp that referenced this issue May 28, 2023
…8600' into bjorn/maint/primary_preloaded

* bjorn/compiler/beam_ssa_pre_codegen/erlangGH-7248/OTP-18600:
  Fix register allocation in the presence of singled-valued phi nodes
bjorng added a commit that referenced this issue May 30, 2023
…gen/GH-7248/OTP-18600

Fix register allocation in the presence of singled-valued phi nodes
@bjorng bjorng closed this as completed in 3cb4455 May 30, 2023
rickard-green pushed a commit that referenced this issue Jun 8, 2023
…into maint-26

* bjorn/compiler/beam_ssa_pre_codegen/GH-7248/OTP-18600:
  Fix register allocation in the presence of singled-valued phi nodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants