Fix tests for iverilog v11 (SFT deprecated warning)

[Vekhir]

In IVerilog v11+, the use of sft files is deprecated and creates a warning. This warning leads to failing test cases, though behaviour is unchanged.

This PR is based on https://steveicarus.github.io/iverilog/usage/vpi.html and replaces the sft file creation with the -L option to pass the vpi files. The design works for IVerilog v11 and v12.
In addition, a version check is introduced that applies the old behaviour if the version is 10.x. I wasn't able to test with IVerilog v10.x as my distro doesn't provide this version for some time. So that's left for the CI to check.

Fixes #344

[quark17]

On lines 158 and 170, there's a non-breaking space (NBSP) character (ASCII 0xA0) instead of an ordinary space (0x20) between the [ and " in the start of the if-statement:

if [ "$CREATESFT" = "yes" ]; then

In my emacs, it shows up as a red underscore. Displaying it with more shows it as a space. I can delete it in emacs (and replace it with a space), and that fixes the problem.

When the character is there, it causes the following shell errors:

+/path/bsc/inst/lib/exec/bsc_build_vsim_iverilog: 158: /path/bsc/inst/lib/exec/bsc_build_vsim_iverilog: [ yes: not found
+/path/bsc/inst/lib/exec/bsc_build_vsim_iverilog: 170: /path/bsc/inst/lib/exec/bsc_build_vsim_iverilog: [ yes: not found

[Vekhir]

Interesting. Thanks for the comment, that would have been hard to find. Especially since my shell seems unaffected by it.

The latest commit should fix that. After that I'll reintroduce the version check and add the -L option for newer IVerilog. Hopefully without rogue whitespaces 😄

[Vekhir]

This commit series should now fix the issue and give IVerilog the VPI files instead of SFT while preserving support for 10.x

@quark17 Since the tests are passing on main, are there test changes/additions that should be made in order to show that the issue is solved? I'm thinking of suppressed warnings that can be re-enabled or something.

[Vekhir]

Ok, it seems that the version check doesn't work, i.e. the returned version string on ubuntu 20.04 looks entirely different from the current one.
For reference, my version string looks like this: Icarus Verilog version 12.0 (stable) (v12_0-dirty)

At this point, I don't have access to an IVerilog 10.3 package, so I cannot determine the necessary matching pattern to detect version 10.x. I'll try setting up a VM

[Vekhir]

Getting an Ubuntu 20.04 VM running was easier than I thought.

Apparently, the Ubuntu release does not contain the v10_x tag, so we can't rely on it. Replaced it with the Version 10.x
Reference for Ubuntu 20.04: Icarus Verilog version 10.3 (stable) ()

[quark17]

In the testsuite (in get_verilog_compiler_version in testsuite/config/verilog.tcl), we get the iverilog version by getting the first line of output:

iverilog -V 2>/dev/null | head -1

and then extracting the first group of non-space characters following Icarus Verilog version:

regexp -- {^Icarus Verilog version ([^ ]+).*$} $line1 tmp verilog_compiler_version

This could be of form x.y.z and might even have some tag information at the end, I think (as alphanumeric and underscores)? The testsuite is just interested in the major and minor version, when concerned about version, so in those places we get the "maj.min" string with tcl regexp, which we can then compare numerically. For example in testsuite/bsc.verilog/tasks/tasks.exp:

if { $verilog_compiler == "iverilog" &&
     [regexp {^\d+\.\d+} $verilog_compiler_version majmin] &&
     $majmin < 11
 } {

Although I do see one place in tasks.exp, where we are matching the whole version string, which is probably bad (and should be changed to using the extracted majmin and numerical comparison operators):

if { $verilog_compiler == "iverilog" &&
     ($verilog_compiler_version == "10.1" || $verilog_compiler_version == "10.2") } {

(Also, I'm a bit concerned that the Tcl is using -1 instead of -n 1, but it seems to work? And I don't know if your piping of stderr to stdout is the best way to handle errors?)

I wonder if your grep should be checking for the whole pattern, Icarus Verilog version 10.?

Since the tests are passing on main, are there test changes/additions that should be made in order to show that the issue is solved? I'm thinking of suppressed warnings that can be re-enabled or something.

If we want to add a test, it could probably be done in testsuite/bsc.codegen/foreign/. In foreign.exp, there are examples containing foreign functions that are being compiled and run. After one of those, you could add a check that SFT (or sft) doesn't exist in the output of the BSC linking stage. You'd want to guard it, so that the check only occurs for iverilog and only for versions 11+. So maybe something like this:

if { $verilog_compiler == "iverilog" &&
     [regexp {^\d+\.\d+} $verilog_compiler_version majmin] &&
     $majmin >= 11
 } {

There are test procedures like find_n_strings and find_regexp etc defined in testsuite/config/unix.exp that can be used. Maybe find_regexp_fail, which expects to not find the pattern. However, if iverilog's error message changes and doesn't contain SFT, then this test would continue to pass, even if there was a problem. So a stronger possibility would be to compare the entire output against an expected file (with compare_file), but iverilog also outputs a warning about VPI support being incomplete, so you'd need to include that for now. Just checking for SFT is probably fine.

[Vekhir]

The great restriction for version checking is that bsc_build_vsim_iverilog is run using the POSIX shell sh, so e.g. regexp is not available. We only need to check for existence anyway, so grep should be fine.
I have nonetheless changed the error pipe to /dev/null, and the entire prefix up to "10." is checked as it is unlikely to change for this version.

As for tests, I found that the warning is suppressed in bsc.options/options.exp for IVerilog v11, which explains why I get the error, but the CI runs fine (I have v12). I think that removing that exemption is good enough for a test.

While at it, I also changed the 10.1 check to your suggested syntax using numerical comparison instead of string comparison. There are also workarounds for VPI issues in bsc.codegen/foreign/foreign.exp and bsc.codegen/foreign/battery/battery.exp which I can confirm are still needed for v12.

[Vekhir]

@quark17 I'm happy with these commits. Is there anything you'd like to see before merging?

[Vekhir]

@quark17 I've implemented your suggestion.

I can only check that nothing else breaks, as can the CI since Ubuntu 18.04 was dropped.

[quark17]

Thank you for your patience! Looks good, I'll merge. Also, sorry that I forgot about your other issue asking about v12 failures and the testsuite. I'll look at that and respond now. Thank you again!

[Vekhir]

Your guidance throughout this PR is also much appreciated. I'm glad I could be helpful!