![[logo]](/img/veripool_small.png)
Issue #370
vpi_register_cb problem
| Status: | Closed | Start date: | 07/20/2011 | |
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assignee: | Wilson Snyder | % Done: | 0% |
|
| Category: | Unsupported | |||
| Target version: | - |
Description
I'm using the VPI with Verilator v3.812 on 64-bit Linux. I register a function with vpi_register_cb, passing in a s_cb_data. When the function is called it is passed the s_cb_data structure that was passed in at registration time, rather than a new one containing e.g. the value of the signal that has changed.
A quick edit to verilated_vpi.h 'fixes' the problem.
329d328 < vop->cb_datap()->value->value.integer = *((PLI_INT32*)newDatap);
If you agree this is a bug and with some help I should be able to work this up into a proper patch, and/or provide testcases.
History
Updated by Wilson Snyder 10 months ago
- Category set to Unsupported
- Status changed from New to Assigned
- Assignee set to Wilson Snyder
I think you're close, thanks for tracking this down. Your change assumed the value is an integer; I think the right thing to do is to call vpi_get_value(object, vop->cb_datap()->value) so that arbitrary types will work out right and so if value is NULL it won't crash.
Would you be willing to update test_regress/t/t_vpi_var.cpp to test this (run test_regress/t/t_vpi_var.pl to run it), see it fail, then try the vpi_get_value?
Thanks
Updated by Thomas Watts 10 months ago
That works, but I have a question.
We are calling vpi_get_value using a pointer which points to the value structure from the initial call to vpi_register_cb. Is this the intention or should the vpi allocate its own structure in vpi_register_cb and free it in vpi_remove_cb?
Perhaps adding a function to VerilatedVpioCb to allocate a new structure to its m_cbData? Called from VerilatedVpi::cbReasonAdd and then freed in ~VerilatedVpioCb.
Updated by Wilson Snyder 10 months ago
You're right again. The spec says s_cb_data and other structures, s_vpi_value and s_vpi_time, are simulator owned. I'd suggest that VerilatedVpioCb have member objects of s_vpi_value just as it has cb_data now, so it will be created and destroyed as you suggest. (No need to separately new() unless I'm missing something.)
Updated by Wilson Snyder 10 months ago
Any luck with a patch? I'm hoping to release in a few days and would be good to have this in, thanks.
Updated by Thomas Watts 10 months ago
- File verilated_vpi.h.patch added
- File t_vpi_var.cpp.patch added
- File t_vpi_var.v.patch added
Patch attached.
I haven't done anything regarding adding an s_vpi_time object yet.
Updated by Wilson Snyder 10 months ago
- Status changed from Assigned to Resolved
Great! We'll leave time for later, as Verilator times aren't yet standard.
Pushed to git towards 3.814.
Also available in: Atom