[Vpi] Allow using packed struct as a signal on sim module

[AndrewNolte]

The purpose of this is to be able to set/get a struct's entire value, rather than just its individual signals, which in turn will allow for more shared user code between verilator and the other environments.

I default packed structs to the new behavior since this is more correct, and allow casting from to the old hierarchy object via asHierarchyObject().

Closes #3623.

[AndrewNolte]

I realized when running this that doing

make -C tests/test_cases/test_struct SIM=xcelium
make -C tests/test_cases/test_struct SIM=verilator

in sequence causes the verilator one to not run and exit early, any idea why this might be the case? I have to make clean before I can run verilator

[AndrewNolte]

Can I get a look at this @marlonjames @ktbarrett. I'm not sure why it's tripping up on vhdl runs

[ktbarrett]

I'll try to do this tonight. Otherwise it might be a while.

[AndrewNolte]

It looks like vhdl sims actually seg fault when trying to call get_signal_val_binstr, which is why they failed even though expect_fail was set to true. And the verilog ones rn are because of the logicarray change, I'll fix those rq

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 72.63%. Comparing base (10feab6) to head (0aa6fcd).

Files	Patch %	Lines
src/cocotb/share/lib/vpi/VpiImpl.cpp	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3608      +/-   ##
==========================================
+ Coverage   72.26%   72.63%   +0.36%     
==========================================
  Files          49       49              
  Lines        8038     8044       +6     
  Branches     2216     2210       -6     
==========================================
+ Hits         5809     5843      +34     
+ Misses       1724     1690      -34     
- Partials      505      511       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ktbarrett]

Just changing structs to arrays is not the right answer here. I'm actually really surprised that the regression passes with this change. Clearly not enough testing is being done. What's needed is a new type that supports both accessing fields and setting the whole value.

I don't know what happens when you attempt to set the full value on something that isn't a packed struct, or if most simulators even support differentiating packed vs unpacked structs. Without more testing or more work on supporting both (so we don't have a breaking change), I'm not comfortable with this change.

[AndrewNolte]

It's essentially just adding the signal methods to the gpi handle, whereas before it didn't have these. Changing handle.py to allow both hierarchy and signal will be the tricky part

[marlonjames]

sample_module.inout_if happens to be a packed struct here, but unpacked structures and unpacked unions cannot be set in their entirety with vpi_put_value().

This type of feature would at minimum require getting vpi_get(vpiPacked) to differentiate, and I agree with Kaleb on developing a new type for interfacing to it.

[ktbarrett]

Actually now that I've been thinking about this. I think this might be a good idea. Indexing into packed objects, whether that's packed arrays, or packed structs, etc. is not kosher, at least conceptually. I'm now more surprised that what we currently have works.

We do need a new type for packed objects that aren't single-dimension packed arrays.

Using vpi_get(vpiPacked, handle) would definitely help in that regard, but I don't know how to differentiate packed single-dimension arrays from other types. Maybe that's a pipe-dream. Ideally we could extract a whole packed object structure from the VPI to structure the logic array being output.

[AndrewNolte]

"What's needed is a new type that supports both accessing fields and setting the whole value." - so only VpiSignal provide special extra methods, VpiObjHdl does not. Everything for accessing fields is just operating on the raw vpi handle, so our check for that is simply 'does the vpi throw an error' and relying on handle.py to call it on valid objects.

So I can make a VpiStructObjHdl that just extends VpiSignalObjHdl, but it'll add nothing. And I'll add the vpiPacked check as well. It would be nice to also have it extend VpiObjHdl to get defname and deffile, but then we'd have multiple inheritance

[ktbarrett]

I'm still trying to figure out a holistic approach to this.

In my mind:

1. You can't index into packed objects.
2. Packed objects are discovered as single GPI objects.
3. This gets mapped to a LogicObject or similar, which cannot be indexed.
4. You should be able to query the structure of that object ([0:2][0:2]packed_struct_t[7:0], etc.).
5. The Python type we return from .value of such objects should use the structure definition to support indexing and updated the value, but getting and setting the value on the handle only happens whole packed object at a time.

I know this is going to require both C++ and Python changes, but I'm uncertain to what extent. Also I'm uncertain if point 4 is even possible.

What's needed is a new type that supports both accessing fields and setting the whole value.

Yeah... maybe we should forget about accessing fields on the object and only worry about accessing fields on the value coming out.

[AndrewNolte]

Do many simulators support the type spec? For xcelium (and seemingly other sims) it just treats them as unpacked. I think just having this for now supports the old way via an explicit type change. I was originally going to support both .value and child handle access, but with the latest rebase there were some method conflicts between LogicObject and HierarchyObject, so I thought it best to default to the proper way for Packed Structs. (len) especially would be confusing

And again, what cpp changes need to be made if we're not inspecting the type spec? The object hierarchy was implemented as methods on GpiObjHdl and VpiImpl, not VpiObjHdl, so it works just fine.

[ktbarrett]

So uhhh... yeah I definitely want to go back to the original implementation on this. Let's just remove support for now and try to realize #3857 sooner rather than later.

[ktbarrett]

Riviera is failing with test_struct.test_struct_format failed: passed but we expected a failure.

Questa is failing with

#      0.00ns INFO     cocotb.regression                  test_iteration.recursive_discovery failed
#                                                         Traceback (most recent call last):
#                                                           File "/opt/actions-runner/_work/cocotb/cocotb/tests/test_cases/test_iteration_mixedlang/test_iteration.py", line 70, in recursive_discovery
#                                                             assert pass_total == total, "Expected %d but found %d" % (pass_total, total)
#                                                         AssertionError: Expected 1332 but found 1276
#                                                         assert 1332 == 1276

[ktbarrett]

@cocotb/maintainers Considering I plan on reintroducing the indexing in a future commit, is this good?