Nested struct list segfault #108

bartkrak · 2024-02-08T14:03:03Z

solves membraneframework/membrane_core#719

bartkrak · 2024-02-08T16:43:07Z

With CNode there are two cases that cause problems.
Case 1: A struct that holds a list of structs containing a list. Exact specification used for testing below.
spec file:

type nested_struct_inner :: %Nested.StructInner{
  name: string,
  data: [int],
  id: int
}
type nested_struct_list :: %Nested.StructList{
  inner_list: [nested_struct_inner],
  id: int
}
spec test_nested_struct_list(in_struct :: nested_struct_list) :: {:ok :: label, out_struct :: nested_struct_list}

C file:

UNIFEX_TERM test_nested_struct_list(UnifexEnv *env, nested_struct_list in_struct) {
  return test_nested_struct_list_result_ok(env, in_struct);
}

and test file:

test "nested struct list", context do
    cnode = context[:cnode]
    inner = %Nested.StructInner{id: 1, name: "Jan Kowlaski", data: [1,2,3]}
    nested_struct_list = %Nested.StructList{id: 1, inner_list: [inner]}
    assert {:ok, ^nested_struct_list} = Unifex.CNode.call(cnode, :test_nested_struct_list, [nested_struct_list])
end

Running above configuration results in the following error:

** (RuntimeError) Unifex CNode: cannot parse argument 'in_struct' of type ':nested_struct_list'
     code: assert {:ok, ^nested_struct_list} = Unifex.CNode.call(cnode, :test_nested_struct_list, [nested_struct_list])
     stacktrace:
       (unifex 1.1.1) lib/unifex/cnode.ex:122: Unifex.CNode.handle_response/1
       test/example_test.exs:163: (test)

Above error is raised when CNode tries to parse the most inner list (“data” field of nested_struct_inner)
It's related to parsing arguments in generate_caller_function in lib/unifex/code_generators/cnode.ex

Case 2: A struct that holds a list of structs. Exact specification used for testing below.
spec file:

type nested_struct_inner :: %Nested.StructInner{
  name: string,
  id: int
}
type nested_struct_list :: %Nested.StructList{
  inner_list: [nested_struct_inner],
  id: int
}
spec test_nested_struct_list(in_struct :: nested_struct_list) :: {:ok :: label, out_struct :: nested_struct_list}

C file:

UNIFEX_TERM test_nested_struct_list(UnifexEnv *env, nested_struct_list in_struct) {
  return test_nested_struct_list_result_ok(env, in_struct);
}

and test file:

test "nested struct list", context do
    cnode = context[:cnode]
    inner = %Nested.StructInner{id: 1, name: "Jan Kowlaski"}
    nested_struct_list = %Nested.StructList{id: 1, inner_list: [inner]}
    assert {:ok, ^nested_struct_list} = Unifex.CNode.call(cnode, :test_nested_struct_list, [nested_struct_list])
end

Running this configuration results in a timeout to CNode

** (RuntimeError) Timeout upon call to the CNode :"bundlex_cnode_0_8bf60324-c17c-4897-bd2c-73e1df7b68c5@Barteks-MacBook-Pro-2"
     code: assert {:ok, ^nested_struct_list} = Unifex.CNode.call(cnode, :test_nested_struct_list, [nested_struct_list])
     stacktrace:
       (bundlex 1.4.5) lib/bundlex/cnode.ex:139: Bundlex.CNode.call/3
       (unifex 1.1.1) lib/unifex/cnode.ex:96: Unifex.CNode.call/4
       test/example_test.exs:163: (test)

FelonEkonom · 2024-02-09T14:41:40Z

test/unifex/integration_test.exs

@@ -75,6 +75,8 @@ defmodule Unifex.IntegrationTest do
    |> Enum.each(fn ref ->
      assert File.read!("test_projects/#{project}/c_src/example/_generated/#{interface}/#{ref}") ==
               File.read!("test/fixtures/#{project}_ref_generated/#{interface}/#{ref}")
+
+      true


This is unnecessary

FelonEkonom · 2024-02-29T16:40:21Z

lib/unifex/counter.ex

+  def start_link(initial_value) do
+    Agent.start_link(fn -> initial_value end, name: __MODULE__)
+  end


We can delete initial_value var since we always start counter from 0

FelonEkonom · 2024-02-29T16:40:38Z

lib/unifex/counter.ex

+    Agent.update(__MODULE__, &(&1 + 1))
+    Agent.get(__MODULE__, & &1)


use Agent.get_and_update instead

FelonEkonom · 2024-02-29T16:41:18Z

lib/tasks/compile_unifex.ex

  def run(_args) do
+    Counter.start_link(0)


Start Counter in the application module, in the supervisor tree

FelonEkonom · 2024-02-29T16:42:46Z

lib/unifex/counter.ex

+  end
+
+  @spec get_value() :: integer()
+  def get_value do


Suggested change

def get_value do

def get_and_increment() do

This describes better, what really happens

FelonEkonom · 2024-02-29T16:43:59Z

test_projects/cnode/c_src/example/_generated/example.h

+#pragma once
+
+#ifdef BUNDLEX_CNODE
+#include "cnode/example.h"
+#endif
+


Files from _generated shouldn't be in this PR

FelonEkonom · 2024-02-29T16:45:17Z

test_projects/cnode/lib/nested_struct_inner.ex

@@ -0,0 +1,3 @@
+defmodule Nested.StructInner do


Suggested change

defmodule Nested.StructInner do

defmodule Nested.InnerStruct do

this struct is unused, forgot to remove this

FelonEkonom · 2024-02-29T16:49:14Z

test_projects/cnode/c_src/example/example.spec.exs

@@ -52,6 +51,13 @@ type nested_struct :: %Nested.Struct{

 spec test_nested_struct(in_struct :: nested_struct) :: {:ok :: label, out_struct :: nested_struct}

-type my_enum :: :option_one | :option_two | :option_three | :option_four | :option_five
+type nested_struct_list :: %Nested.StructList{
+  inner_list: [my_struct],


Suggested change

inner_list: [my_struct],

list_of_structs: [my_struct],

Name inner_list suggests, that this list is wrapped in something, eg. another list

FelonEkonom · 2024-02-29T16:50:21Z

test_projects/cnode/c_src/example/example.spec.exs

+
+spec test_nested_struct_list(in_struct :: nested_struct_list) :: {:ok :: label, out_struct :: nested_struct_list}
+
+type(my_enum :: :option_one | :option_two | :option_three | :option_four | :option_five)


Remove the brackets

FelonEkonom · 2024-02-29T16:50:45Z

test_projects/cnode/test/example_test.exs

-    assert nil == Unifex.CNode.call(context[:cnode], :test_nil, [])
-  end
-
-  test "atom", context do
-    assert {:ok, :unifex} = Unifex.CNode.call(context[:cnode], :test_atom, [:unifex])
-  end
-
-  test "bool", context do
-    assert {:ok, true} = Unifex.CNode.call(context[:cnode], :test_bool, [true])
-    assert {:ok, false} = Unifex.CNode.call(context[:cnode], :test_bool, [false])
-    refute match?({:ok, false}, Unifex.CNode.call(context[:cnode], :test_bool, [true]))
-  end
-
-  test "float", context do
-    assert {:ok, 0.0} = Unifex.CNode.call(context[:cnode], :test_float, [0.0])
-    assert {:ok, 0.1} = Unifex.CNode.call(context[:cnode], :test_float, [0.1])
-    assert {:ok, -0.1} = Unifex.CNode.call(context[:cnode], :test_float, [-0.1])
-    refute match?({:ok, 1}, Unifex.CNode.call(context[:cnode], :test_float, [1.0]))
-  end
-
-  test "unsigned int", context do
-    cnode = context[:cnode]
-    assert {:ok, 0} = Unifex.CNode.call(cnode, :test_uint, [0])
-    assert {:ok, 5} = Unifex.CNode.call(cnode, :test_uint, [5])
-
-    assert_raise RuntimeError, ~r/argument.*in_uint.*unsigned/i, fn ->
-      Unifex.CNode.call(cnode, :test_uint, [-1])
-    end
-  end
-
-  test "string", context do
-    cnode = context[:cnode]
-
-    big_test_string =
-      "unifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexu
-      nifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexuni
-      fexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifexunifex"
-
-    assert {:ok, ""} = Unifex.CNode.call(cnode, :test_string, [""])
-    assert {:ok, "test_string"} = Unifex.CNode.call(cnode, :test_string, ["test_string"])
-    assert {:ok, "-12345"} = Unifex.CNode.call(cnode, :test_string, ["-12345"])
-    assert {:ok, "255"} = Unifex.CNode.call(cnode, :test_string, ["255"])
-    assert {:ok, ^big_test_string} = Unifex.CNode.call(cnode, :test_string, [big_test_string])
-  end
-
-  test "list as string", context do
-    0..253
-    |> Enum.each(fn x ->
-      list = [x, x + 1, x + 2]
-      assert {:ok, ^list} = Unifex.CNode.call(context[:cnode], :test_list, [list])
-    end)
-  end
-
-  test "list", context do
-    cnode = context[:cnode]
-    assert {:ok, []} = Unifex.CNode.call(cnode, :test_list, [[]])
-    assert {:ok, [-1, -1, -1]} = Unifex.CNode.call(cnode, :test_list, [[-1, -1, -1]])
-    assert {:ok, [-10, -17, -28]} = Unifex.CNode.call(cnode, :test_list, [[-10, -17, -28]])
-    assert {:ok, [355, 355, 355]} = Unifex.CNode.call(cnode, :test_list, [[355, 355, 355]])
-    assert {:ok, [1254, 1636, 3643]} = Unifex.CNode.call(cnode, :test_list, [[1254, 1636, 3643]])
-  end
-
-  test "list of strings", context do
-    cnode = context[:cnode]
-    assert {:ok, []} = Unifex.CNode.call(cnode, :test_list_of_strings, [[]])
-
-    assert {:ok, ["", "", ""]} = Unifex.CNode.call(cnode, :test_list_of_strings, [["", "", ""]])
-
-    assert {:ok, ["1", "2", "3"]} =
-             Unifex.CNode.call(cnode, :test_list_of_strings, [["1", "2", "3"]])
-
-    assert {:ok, ["abc", "def", "ghi"]} =
-             Unifex.CNode.call(cnode, :test_list_of_strings, [["abc", "def", "ghi"]])
-  end
-
-  test "list of unsigned ints", context do
-    cnode = context[:cnode]
-    assert {:ok, [0, 1, 2]} = Unifex.CNode.call(cnode, :test_list_of_uints, [[0, 1, 2]])
-  end
-
-  test "list with other arguments", context do
-    cnode = context[:cnode]
-
-    assert {:ok, [1, 2, 3], :other_arg} =
-             Unifex.CNode.call(cnode, :test_list_with_other_args, [[1, 2, 3], :other_arg])
-
-    assert {:ok, [300, 400, 500], :other_arg} =
-             Unifex.CNode.call(cnode, :test_list_with_other_args, [[300, 400, 500], :other_arg])
-  end
-
-  test "payload", context do
-    assert {:ok, <<2, 2, 3>>} = Unifex.CNode.call(context[:cnode], :test_payload, [<<1, 2, 3>>])
-  end
-
-  test "pid", context do
-    pid = self()
-    assert {:ok, ^pid} = Unifex.CNode.call(context[:cnode], :test_pid, [pid])
-  end
-
-  test "struct", context do


FelonEkonom · 2024-03-04T14:08:25Z

Bump version to 1.1.3 and merge and release this PR after #111

bartkrak added 5 commits February 7, 2024 14:16

segfault fixed, tests adjusted

e568b8a

added nif test for nested struct list

f4fd01d

cnode tests partially ready, working state

6c257eb

unifex raise numbered

fb074ff

nif ready, cnode wip

6e76026

bartkrak requested a review from FelonEkonom as a code owner February 8, 2024 14:03

bartkrak self-assigned this Feb 8, 2024

bartkrak added 8 commits February 12, 2024 18:46

wip, counter agent added to cnode list

c0f9e5b

wip

8c0c9ce

wip

990ec01

generated code with strange memory behaviour

e1f8baf

wip - generated code works

e414912

case 1 fixed

faf0a26

credo, tests

72fcd9a

vscode settings removed

5aafa61

FelonEkonom requested changes Feb 29, 2024

View reviewed changes

bartkrak added 6 commits February 29, 2024 18:18

refactor, _generated remove

ffd6eab

nif tests work

1183612

cnode change list name

d5c655e

counter agent moved to app, tests adjusted

4bd1544

ci fix

5734435

_generated unnecesary files removed

be043be

bartkrak requested a review from FelonEkonom March 1, 2024 15:26

FelonEkonom approved these changes Mar 4, 2024

View reviewed changes

version bump

27e1f63

bartkrak merged commit edc3ad1 into master Mar 4, 2024
3 checks passed

bartkrak deleted the nested-struct-segfault branch March 4, 2024 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested struct list segfault #108

Nested struct list segfault #108

bartkrak commented Feb 8, 2024

bartkrak commented Feb 8, 2024

FelonEkonom Feb 9, 2024

FelonEkonom Feb 29, 2024

FelonEkonom Feb 29, 2024

FelonEkonom Feb 29, 2024

FelonEkonom Feb 29, 2024

FelonEkonom Feb 29, 2024

FelonEkonom Feb 29, 2024

bartkrak Feb 29, 2024

FelonEkonom Feb 29, 2024 •

edited

Loading

FelonEkonom Feb 29, 2024

FelonEkonom Feb 29, 2024

FelonEkonom commented Mar 4, 2024

		Agent.update(__MODULE__, &(&1 + 1))
		Agent.get(__MODULE__, & &1)

	defmodule Nested.StructInner do
	defmodule Nested.InnerStruct do


		spec test_nested_struct_list(in_struct :: nested_struct_list) :: {:ok :: label, out_struct :: nested_struct_list}

		type(my_enum :: :option_one \| :option_two \| :option_three \| :option_four \| :option_five)

Nested struct list segfault #108

Nested struct list segfault #108

Conversation

bartkrak commented Feb 8, 2024

bartkrak commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelonEkonom Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelonEkonom commented Mar 4, 2024

FelonEkonom Feb 29, 2024 •

edited

Loading