Verilator's vpiStringVal implementation masks byte values of 32 and 0

[mjeje]

Verilator's vpiStringVal implementation masks byte values of 32 and 0.

verilator version: Verilator 5.022 2024-02-24 rev UNKNOWN.REV
OS: macOS Sonoma

Hello. I am writing handler functions to write and read signals from a Verilog Top module. My application uses vpiStringVal format. I noticed in the implementation for vl_get_value(...) that byte values equal to 0 are returned as spaces (8'b32) to the caller.

//verilated_vpi.cpp:vl_get_value:2349
...
// other simulators replace [leading?] zero chars with spaces, replicate here.
t_outStr[i] = val ? val : ' ';

To test an edge case where this mechanic might fail, I wrote a simple Verilog Top module that assigns the values (32 << 8) and 32 to two constant outputs. 32 is represented as the ASCII space character.

//Top.v
module Top(output [15:0] const1, output [15:0] const2);
   assign const1 = 16'h2000;
   assign const2 = 16'h0020;
endmodule;

And a test bench that calls the vl_get_value wrapper function vpi_get_value() for each output.
The result is two identical char arrays. This behavior is problematic because it is not possible to decode the true value of this output when using vpiStringVal format.

//TestBench.cpp
...
t_vpi_value const1;
const1.format = vpiStringVal;
vpiHandle vh1 = vpi_handle_by_name("const1", NULL);
vpi_get_value(vh1, &const1);
printCharArray(const1.value.str); //{32, 32, 0}
...
t_vpi_value const2;
const2.format = vpiStringVal;
vpiHandle vh1 = vpi_handle_by_name("const2", NULL);
vpi_get_value(vh1, &const2);
printCharArray(const2.value.str); //{32, 32, 0}

The expected result of printing const1 is {32, 0, 0}, and const2 is {0, 32, 0}.

[wsnyder]

Can you please modify one of the Verilator test cases to show this, and also make sure it passes on 2 other simulators and make a pull request or patch with that? A fix would also be appreciated - should be straight forward once have the tests (to include/verilated_vpi.cpp)!

[mjeje]

I created tests for these edge cases in test_regress/t/t_vpi_var.cpp.
https://github.com/mjeje/verilator/tree/vpi-string-val-fix-%235036

These tests fail because vpiStringVal was intended to be used to store ASCII characters for human-readable strings. This explains why leading zeros are replaced by the space character. And why the test macro TEST_CHECK_CSTR incorrectly compares a vpi value beginning with '\0', as this marks the end of the string.

While the above functionality is ok for ASCII strings, it limits the ability to use vpiStringVal as a byte array data structure. This format is the most memory space efficient of all the options VPI provides: vpiBinStrVal stores 1 bit per byte, vpiHexStrVal stores 4 bits per byte, and vpiStringVal stores 8 bits per byte. For that reason, this use case should be supported.

Adding this change would mean that any code that reads a vpiStringVal could not reliably use the null terminator character to determine the end of the string. From a quick keyword search of the codebase, it looks like this only happens in the tests directory. I can create a PR to add those changes in.

[wsnyder]

Which other simulators dld you test? We want it passing on at least 2 others, as this change may break code assuming current behavior, so we need it to be right. Thanks.

[mjeje]

Would Vivado or Icarus suffice?

[wsnyder]

Yes, but big-3 preferred.

[mjeje]

I created a repo to demonstrate this issue in Modelsim, Icarus, and Verilator. All three produced unique outputs when I tested the edge cases above. This may point to ambiguity in the IEEE standard, which allows for an implementation change that resolves these issues.

https://github.com/mjeje/vpiStringVal-bug-demo/tree/main

[wsnyder]

Thanks for experimenting. Given that, is this then not a bug?

[mjeje]

That's correct. This is technically not a bug in that it observes IEEE 1800. However, there is evidently some wiggle room in how vpiStringVal can be implemented and the current implementation restricts its use to ASCII strings. I am proposing an edit to expand the versatility of this format so that it can be used to store N-width integers. This would allow for memory optimization and more predictable behavior when accessing large hardware logic values.

[wsnyder]

I am proposing an edit to expand the versatility of this format so that it can be used to store N-width integers. This would allow for memory optimization and more predictable behavior when accessing large hardware logic values.

This matches at least one of the different other simulators? If so I'll take the pull request.

[mjeje]

I do not have access to the other big three simulators, so I can't confirm how VCS and Cadence behave. As for the simulators I tested in the experiment, neither ModelSim nor Icarus allow this usability. However, Verilator's output within the experiment would remain equally distinct (wrt to other simulators) with or without the change.

[wsnyder]

Ok, please submit the pull to fix this.

[wsnyder]

Looking at the change proposed, the putVal is now ignoring null characters. What is passed in is a C string, which must by rules of C be null terminated. So I don't think we can do this. Also it looks like most of the simulators that aren't completely broken agree with Verilator.

Questa:

vpi:     test1=ab20
vpi:     test2=20ab
vpi:     test3=20ab
vpi:     test4=2020

Xcelium:

vpi:     test1=0000
vpi:     test2=ab00
vpi:     test3=ab00
vpi:     test4=0000

Modelsim:

vpi:     test0=2020
vpi:     test1=ab20
vpi:     test2=20ab
vpi:     test3=ffab
vpi:     test4=2020

[wsnyder]

I think you'll need to rely on vpiRawTwoStateVal. That should also be portable. Does that work?

[mjeje]

vpiRawTwoStateVal may be a better alternative if it can also be used to read scalar logic values. But the get/put functions for array values are unimplemented in Verilator. Are there plans to add these in the future?

[mjeje]

Looking at the change proposed, the putVal is now ignoring null characters. What is passed in is a C string, which must by rules of C be null terminated. So I don't think we can do this. Also it looks like most of the simulators that aren't completely broken agree with Verilator.

Questa:

vpi:     test1=ab20
vpi:     test2=20ab
vpi:     test3=20ab
vpi:     test4=2020

Xcelium:

vpi:     test1=0000
vpi:     test2=ab00
vpi:     test3=ab00
vpi:     test4=0000

Modelsim:

vpi:     test0=2020
vpi:     test1=ab20
vpi:     test2=20ab
vpi:     test3=ffab
vpi:     test4=2020

I agree. Without providing the length along with the string it's impossible to know which null char indicates the termination of the string. vpi_put_value()'s handling for vpiVectorVal is nearly identical to how vpiStringVal is handled in this change, both are vulnerable to overrun.

Unfortunately, the standard has no room to add a length argument or struct member.

[wsnyder]

But the get/put functions for array values are unimplemented in Verilator. Are there plans to add these in the future?

Not currently, VPI is basically added as people need a feature, but should be straightforward to make a pull to support them - perhaps something you'd consider?

[mjeje]

Yes, I can add those functions.