[1/3] Add external models to string_ptr; add byte_vector* as an alias

[umarcor]

Coming from #476. Related to #470.

In this PR, external modes are added to string_ptr.vhd through VHPIDIRECT (see https://ghdl.readthedocs.io/en/latest/using/Foreign.html). With 'external mode' we mean:

• VHDL can use get/set callback functions defined in C (or any C-alike compatible language) to interact with strings (byte vectors).
• An array of pointers to arrays of chars (char* []) can be shared between VHDL an any C-alike compatible language.

The usage is as follows:

• new_string_ptr(len, mode, eid, value) creates a new array of length len with mode mode and identifier id.
  • If mode = internal (default), the array is allocated in the testbench and it is initialized with value.
  • If mode = extfnc, the array is not allocated. When values are get/set, VHPIDIRECT functions/procedures read_char/write_char are called.
  • If mode = extacc, the array is not allocated. A pointer is created and the value is retrieved through VHPIDIRECT function get_string_ptr. The returned value must be a pointer to an array of type uint8_t (size of 1 byte). When data is get/set, content is directly modified in the shared memory.

Notes:

• When mode /= internal, value is not used and len is used to tell VHDL the maximum size of the external buffer. Currently, this is a fixed value, i.e., there is no 'synchronization' of this value.
• When mode /= internal, eid is used to decide which of the shared buffers to use.
• These features have been tested with C, Python and C++ (octave) tools.

---

In order to support using string_ptr with or without VHPIDIRECT, two different implementations of the external resources are provided:

• external_string_pkg-vhpi.vhd, declares the functions/procedures as external; therefore, C implementations must be provided.
• external_string_pkg-novhpi.vhd does not declare the functions/procedures as external; C implementations are not required, but it is not possible to create vectors with mode /= internal (an assertion of level error is raised).

The user needs to select whether to use external C objects. Furthermore, alternative implementions of *vhpi.pkg sources might be provided. This is not expected to be required for most use cases, but it is supported. This is achieved through a new external parameter to from_args and from_argv:

• None (defaults to False)
• {'string': False}
• {'string': True}
• {'string': ['path/to/custom/file']}

---

deallocate, reallocate and resize are not implemented for external modes (mode /= internal). For the case extacc, it would be easy to implement it. Since the 'connection' is a pointer, it is possible to use malloc or mmap in the C implementation. Coherently, it would be possible to update the length in the VHDL model according to the actual size in the C app.

However, when extfnc, it can be complex or not possible. Since read_char/write_char can be used to interact with external services/processes through pipes, sockets, packets, messages, etc., the implementation is likely to be very specific. Nonetheless, in this example the implementation can be the same for both modes.

---

byte_vector is added as an alias of string.

[umarcor]

@kraigher, I think that the most important part to understand this PR is https://github.com/dbhi/vunit/blob/ext-strings/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd#L56-L154. Precisely, https://github.com/dbhi/vunit/blob/ext-strings/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd#L65-L72:

type ptr_storage is record
  id    : integer;
  ptr   : integer;
  eptr  : integer;
  ids   : storage_vector_access_t;
  ptrs  : vava_t;
  eptrs : evava_t;
end record;

id, ptr and eptr are just pointers to the latest element in ids, ptrs and eptrs. Precisely, id is exactly the same as current_index in the master branch.

• ids contains an element/reference for each allocated string/vector, no matter if it is external or internal.
• ptrs contains an element/reference for each internally allocated string/vector, i.e. when the user uses new_string_ptr(id=>0).
• eptrs contains an element/reference for each externally allocated string/vector, i.e. when the user uses new_string_ptr(id=>n) where n>0.

When the user uses new_string_ptr(id=>n) where n<0, the reference is only located in ids, no additional metainfo is required.

As a result, each time get, set, length, etc. are called, first we retrieve the corresponding storage_t from ids, which will tell us what kind of vector we are dealing with. Then, we execute up to three different snippets. E.g.: https://github.com/dbhi/vunit/blob/ext-strings/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd#L213-L220

[kraigher]

The pull request does not contain any testing that the external mechanism works creating a technical debt.

[umarcor]

The pull request does not contain any testing that the external mechanism works creating a technical debt.

The fact that all the existing tests pass is a proof that id=0 works as it did before, so backwards compatibility is ensured. Unfortunately, testing that data can/is actually read/written for externally allocated buffers is a little more complex.

Nonetheless, the external_buffer example in #476 (which is just one commit ahead of this PR), is precisely meant to test this feature. Please, see: https://github.com/dbhi/vunit/blob/feat-external-arrays/examples/vhdl/external_buffer/run.py. I didn't add it here in order to keep this PR as simple as possible, but I can do it if you want.

I must ask for some trust here.

[kraigher]

If the example serves as the test of id != 0 I would like to include it in this PR.

[umarcor]

If the example serves as the test of id != 0 I would like to include it in this PR.

It is added now.

BTW, I expect py27-acceptance tests to pass when GHDL is 'fixed' or when #509 is merged.

[kraigher]

I have looked through this again and found some issues. I will look further after they are fixed. Sorry for this taking such a long time but you are trying to change a core data type in VUnit which requires a lot of care.

[umarcor]

@kraigher, I think I addressed all of the suggested changes. Let me know when you have another look at it.

[kraigher]

Ok I will look again. By the way I though the integer vector length -> len revert would become a separate PR. It would have been cleaner as that could be merged on its own.

[umarcor]

By the way I though the integer vector length -> len revert would become a separate PR. It would have been cleaner as that could be merged on its own.

I will make now did a separate PR from it: #515.

BTW, regarding this PR, do you think it is worth having separated vunit/vhdl/data_types/src/types/string_pkg.vhd and vunit/vhdl/data_types/src/types/byte_vector_pkg.vhd files? Can we merge them in a single file? Each of them is just 10 lines, so the file will not be longer than 40 lines (including the license) even after external integer_vector is added later.

[kraigher]

I assume id=0 means automatic id assignment. This is assumed with internal string. Maybe an check is needed that is it 0 when it cannot be a custom id.

[umarcor]

Not really. Note that this id is NOT the same id that was in the codebase until 6 months ago. That was renamed to ref: https://github.com/VUnit/vunit/blob/master/vunit/vhdl/data_types/src/string_ptr_pkg.vhd#L20 and https://github.com/VUnit/vunit/blob/master/vunit/vhdl/data_types/src/integer_vector_ptr_pkg.vhd#L21.

The mechanism to share buffers between C and VHDL is a char* [], i.e. an array of pointers to an arrays of chars. This id is used to decide which of those arrays of chars corresponds to a string in VHDL. See examples/vhdl/external_buffer/src/main.c:

• If mode is extacc, VHDL will execute get_string_ptr(id) to retrieve that pointer address.
• If mode is extfnc, VHDL will pass the id in each call to write_char or read_char.

In this example, using write_char (mode extfnc) is equivalent to VHDL writing directly with mode extacc. However, this is only because of the specific C implementation of write_char. In a practical example write_char is expected to do some more complex task, such as sending data through HTTP. That's where the id is handy.

When mode is internal, field id is not used at all.

Nonetheless, ref still exists. Each string, no matter the mode, has a ref; it is assigned automaticallly, as it has always been. See:

vunit/vunit/vhdl/data_types/src/string_ptr_pkg-body-200x.vhd

Lines 131 to 158 in ed75c87

	case mode is
	when internal =>
	st.ids(st.id) := (
	id => st.ptr,
	mode => internal,
	length => 0
	);
	reallocate_ptrs(st.ptrs, st.ptr);
	st.ptrs(st.ptr) := new vec_t'(1 to length => value);
	st.ptr := st.ptr + 1;
	when extacc =>
	st.ids(st.id) := (
	id => st.eptr,
	mode => extacc,
	length => length
	);
	reallocate_eptrs(st.eptrs, st.eptr);
	st.eptrs(st.eptr) := get_ptr(id);
	st.eptr := st.eptr + 1;
	when extfnc =>
	st.ids(st.id) := (
	id => id,
	mode => extfnc,
	length => length
	);
	end case;
	st.id := st.id + 1;
	return st.id-1;

Let's suppose we want to use four strings:

• internal
• extacc with id 0
• internal
• extfnc with id 1

        st.ids st.ptr st.eptr
string0    0        0
string1    1               0
string2    2        1
string3    3

So,

• st.ids is an array of ref that contains four entries, one for each string.
• st.ptr is the array of refs/pointers to internal strings (this is the element that has always existed, everything else is wrapped around it), and contains two entries.
• st.eptr is the array of refs/pointers to extacc strings (id is used to get_string_ptr once only).

As a result:

st.ids(0).id is 0 (index to read/write from/to st.ptr)
st.ids(1).id is 0 (index to read/write from/to st.eptr)
st.ids(2).id is 1 (index to read/write from/to st.ptr)
st.ids(3).id is 1 (id to read/write through read_char and write_char)

Maybe I am overusing the name id? Currently, it is:

• A parameter for extacc to get the pointer from C.
• A parameter for extfnc to pass to read_char/write_char.
• The name of the field st.ids, which contains a number (id) for each string.
• The name of field st.ids().id which contains a number representing either:
  • The corresponding index in st.ptr.
  • The corresponding index in st.eptr.
  • The id parameter used when creating an extfnc string.

EDIT

After reading this, I think that it might be easier to understand #507 (comment). I feel that it should be possible to merge st.ptr and st.eptr in a single array. But VHDL seems to fight against me doing it...

[kraigher]

Ok I understand a bit better, but it still means it is user error to call the function with internal and an explicit id either by name of position?

-- Case 1
new_string_ptr(length, mode => internal, id => 1) -- Not valid
-- Case 2
new_string_ptr(length, mode => internal, id => 0) -- Also not valid
-- Case 3
new_string_ptr(length, mode => internal) -- Valid

Maybe it is not so important to check "Case 2" but "Case 1" could easily give an error which might help debugging.

[umarcor]

Sure. I'd say that such a test existed: id /= 0 is not supported with mode internal. Maybe I unwantedly dropped it during some rebase/modification. Let me check.

BTW, I'm also understanding and having a look at the add_builtins comment above.

[umarcor]

I added an assertion for case 1. Note that case 3 defaults to case 2. Therefore, we have no mechanism to tell them apart. Should the default of id be some non-supported value, we could identify case 2.

[umarcor]

Now:

• I moved index_t to types_pkg.
• I renamed id fields to eid and idx, to make it easier to tell them apart.
• eid is now of type index_t and it defaults to -1.
• Assertions are added to detect cases 1 and 2, and also when extacc or extfnc are used without providing eid.

[umarcor]

@kraigher, shall I add tests for these assertions? If so, do they fit in vunit/vhdl/data_types/test/tb_byte_vector_ptr.vhd?

[LarsAsplund]

I'm ok with these changes now

[umarcor]

I'm ok with these changes now

🚀🚀🚀

However, let's not merge for now. After lunch, I will re-read all the comments in this issue again, to ensure that me/we are not missing anything. I will also add some further tests (e.g., to use different eid values, i.e., multiple vectors).

Thanks for your support @go2sh. Much appreciated.

[go2sh]

Awesome work in total (your PR series), this will rock. A did a quick test on my setup and seams to work fine, but I haven't checked the code in detail. Right now I use your code to get comfortable with VPI etc.

[umarcor]

I added four new files to the external_buffer example (cp.py, cp.c, tb_extcp_string.vhd and tb_extcp_byte_vector.vhd). Tests are similar to the other set of tbs, but data is copied from the array with eid=0 to the array with eid=1. There was no explicit test for this feature before, only eid=0 was tested.

@kraigher, if you are ok with the status of this PR, we can merge it and move forward.

[kraigher]

I am ok with this.

[bradleyharden]

Now that this is merged, will string_ptr_pkg be stable? I would like to update #522, but not if I will have to change it again soon.

In that PR, I modify queue_t to store string pointers, rather than characters, and I modify string_ptr_pkg to keep track of old, unused pointer indices in stack.

[umarcor]

The second part of this PR is still pending: #476. Here, string_ptr was modified. In #476, the same modifications are applied to integer_vector_ptr. Since #522 modifies both types, I'd suggest to either:

• split New queue implementation #522 in two PRs, to have the string_ptr part merged now;
• or, wait until [2/3] Add external modes to string_ptr and integer_vector_ptr (through VHPIDIRECT) #476 is merged and do everything at the same time.

Note that there are other PRs in the series (such as #470 and #568). However, any of those are to be evaluated after your contributions. This is because those higher level features might be adapted/improved based on #522.

[bradleyharden]

The modifications to integer_vector_ptr were really only to provide consistency between the types. It's not required. I'll probably get it up and running on top of this PR and then update integer_vector_ptr after #476