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

Segmentation fault on OTP27 #9100

Closed
martosaur opened this issue Nov 21, 2024 · 4 comments · May be fixed by #9111
Closed

Segmentation fault on OTP27 #9100

martosaur opened this issue Nov 21, 2024 · 4 comments · May be fixed by #9111
Assignees
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM

Comments

@martosaur
Copy link

Describe the bug
Running this function results in a segmentation fault

defmodule Example do
  def foo() do
    %{
      prev: nil,
      next: [equal: "a", delete: "y", equal: "x"]
    }
    |> recursive({"", ""})
  end

  defp recursive(%{next: []}, _), do: IO.inspect(:ok)

  defp recursive(%{next: [{op, text} | next]} = diffs,
         {text_delete, text_insert}
       ) do
    acc =
      case op do
        :insert ->
          {text_delete, text_insert <> text}

        :delete ->
          {text_delete <> text, text_insert}

        :equal ->
          {"", ""}
      end

    %{
      prev: diffs.prev,
      next: next,
    }
    |> recursive(acc)
  end
end

Example.foo()

To Reproduce
Run the elixir script with OTP27

Expected behavior
Function should run normally

Affected versions
Affected:

  • 27.0
  • 27.1.2
  • ref:maint-27

Not affected:

  • 26.2.5.5

Additional context
Please excuse the absurd example, but this is what I could boil down the function to from the original version. Almost any part seems to be important: making recursive function public, matching on prev in function head, removing unused :insert -> clause - all fixes the issue.

@martosaur martosaur added the bug Issue is reported as a bug label Nov 21, 2024
@frej
Copy link
Contributor

frej commented Nov 22, 2024

I know almost nothing about Elixir (and don't have the toolchain installed), but as this looks like a recursive function updating an accumulator represented by a tuple, it could be a fault with the destructive update of tuples optimization (new in OTP-27).

If you were running plain old Erlang and compiling the module with the compiler option +no_ssa_opt_destructive_update made the segfault go away, it was probably triggered by the above mentioned optimization. According to Google, including @compile {:no_ssa_opt_destructive_update} in your module should pass the flag to the Erlang compiler. If that makes the segfault go away, I would be helped by a dump of your module in Erlang abstract format.

@bjorng bjorng self-assigned this Nov 22, 2024
@bjorng bjorng added the team:VM Assigned to OTP team VM label Nov 22, 2024
@bjorng
Copy link
Contributor

bjorng commented Nov 22, 2024

@frej,

Here is the code translated to Erlang and slightly simplified:

-module(crash).
-export([foo/0]).

foo() ->
    recursive(#{prev => nil,
                next =>
                    [{equal, <<"a">>},
                     {delete, <<"y">>}]},
              {<<>>, <<>>}).

recursive(#{next := [{Op, Text} | Next]} = Diffs,
          {TextDelete, TextInsert}) ->
    Acc =
        case Op of
            insert ->
                {TextDelete,
                 <<TextInsert/binary,Text/binary>>};
            delete ->
                {<<TextDelete/binary,Text/binary>>,
                 TextInsert};
            equal ->
                {<<>>, <<>>}   %% Bug is here.
        end,
    recursive(#{prev =>
                    case Diffs of
                        #{prev := Prev} ->
                            Prev;
                        Other ->
                            ?MODULE:no_parens_remote(Other)
                    end,
                next => Next},
              Acc).

The bug seems to be in the code for line 22. Only one of the empty binaries is created using bs_init_writable.

@frej
Copy link
Contributor

frej commented Nov 22, 2024

Thanks Björn, I'll take a look as soon as I finish up another thing.

@frej
Copy link
Contributor

frej commented Nov 22, 2024

@bjorng: It's an error in how a a phi-instruction with literals is patched in the destructive update pass. If more than one element of a literal tuple is patched to get its value from bs_init_writable, only one patch will be applied. So it's pretty much local to that case, i.e. it's not any major fault in the alias analysis. Expect a patch on Monday.

For now, @martosaur, compile your module with +no_ssa_opt_destructive_update to work around the bug (the performance should be as for OTP26 then).

frej added a commit to frej/otp that referenced this issue Nov 25, 2024
The code responsible for patching Phi-instructions could not handle
multiple patches to a single literal value, it silently discarded all
but one of the updates. This MR corrects this by merging patches,
using the same mechanism as used for patching function arguments.

Closes erlang#9100
frej added a commit to frej/otp that referenced this issue Nov 26, 2024
While refactoring the code responsible for merging patches to
different elements of a literal in function calls in order to reuse it
to solve issue erlang#9100, it was discovered that it only correctly handle
patches to top-level elements. If multiple elements of a tuple inside
another tuple was patched, it would crash.

This commit fixes the above mentioned bug and also refactors the code
to separate the parts that are specific to handle function arguments
from the parts that merges patches to literals.

The correctness of the refactoring is tested by already existing
tests. A test case verifying the fix will be added in the patch
addressing erlang#9100 as is it not worthwhile to spend the time to
construct a test-case testing each of the issues separately.
frej added a commit to frej/otp that referenced this issue Nov 26, 2024
While refactoring the code responsible for merging patches to
different elements of a literal in function calls in order to reuse it
to solve issue erlang#9100, it was discovered that it only correctly handle
patches to top-level elements. If multiple elements of a tuple inside
another tuple was patched, it would crash.

This commit fixes the above mentioned bug and also refactors the code
to separate the parts that are specific to handle function arguments
from the parts that merges patches to literals.

The correctness of the refactoring is tested by already existing
tests. A test case verifying the fix will be added in the patch
addressing erlang#9100 as is it not worthwhile to spend the time to
construct a test-case testing each of the issues separately.
frej added a commit to frej/otp that referenced this issue Nov 26, 2024
The code responsible for patching Phi-instructions could not handle
multiple patches to a single literal value, it silently discarded all
but one of the updates. This MR corrects this by merging patches,
using the same mechanism as used for patching function arguments.

Closes erlang#9100
@bjorng bjorng linked a pull request Nov 26, 2024 that will close this issue
@bjorng bjorng closed this as completed in bb17fb6 Nov 27, 2024
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.

3 participants