suoto/hdl_checker

Add support for verilator

suoto opened this issue · 13 comments

suoto commented
Add support for verilator
rafaelnp commented

I suppose the base_builder.py and ghdl.py would be a good starting point, would they?

suoto commented

Yes, the tool independent logic should sit on base_builder.py, while <tool_name>.py should have only specifics. (At least that's the goal...)

I'll add verilator to the docker image so we can run all tests from there (generating the docker file needs some manual stuff still....)

rafaelnp commented

I add some initial code to detect verilator and its version. So far I was not successful, mainly due to my poor python skills. Do you have any suggestions for debugging the project?

suoto commented

If you're running via ./run_tests.sh or tox, logs emitted by the loggers will be inside .tox/<environment>/log/tests.log, where <environment> is one of py27-linux, py27-windows, py37-linux or py37-windows.

The main test for the tools is done inside hdl_checker/tests/test_builders.py.

The way the tests have been setup is to have each tool's binary executable inside a specific directory (in the Docker container that'd be /builders), so tests can insert and remove them from the execution path. To add verilator to the container, you could create an image based off of suoto/hdl_checker_tests:latest and just add that (it can be consolidated later) and update references to paths and available builders on tox.ini, hdl_checker/tests/__init__.py#L389 and hdl_checker/builder_utils.py#L203.

rafaelnp commented

Do you suggest/have, beside the corresponding projects documentation, any tutorial for docker and tox? I am not acquainted with both, specially the latter.

suoto commented

Not really, I usually look how other projects do things. The testing scripts should ideally be transparent, although there's no guide on that atm...

HDL Checker testing is admittedly not the most straightforward. In summary:

  1. run_tests.sh script wraps docker run with tox arguments and some info about the host environment to avoid permission issues, then calls the entry point script
  2. .ci/scripts/docker_entry_point.sh uses the args passed previously to create a user with the same name and user/group IDs, then calls tox
  3. tox config is given by tox.ini, which defines
    1. 4 environments: py27-linux, py27-windows, py37-linux and py37-windows
    2. Dependencies needed for testing only
    3. The actual test commands, the main one being .ci/scripts/run_tests.py
  4. .ci/scripts/run_tests.py sets up Python logging and code coverage and calls nose2, which will then call the tests defined inside hdl_checker/tests/

Arguments passed to run_tests.sh will be passed on to tox, which can pass arguments to .ci/scripts/run_tests.py and indirectly to nose2, so you could write something like

$ ./run_tests.sh -e py37-linux -- hdl_checker.tests.test_database --fail-fast -v
                 |-----------|  A |---------------------------------------------|
                   tox args     |    arguments passed to .ci/scripts/run_tests.py
                               arg
                            separator

And run tests defined in hdl_checker.tests.test_database on the Python 3.7 @ Linux env. Additionally, --fail-fast -v will stop the tests in the first error (nose2's default is to run all and then report errors if any).

Because run_tests.sh wraps tox, commands below are equivalent:

# Listing tox envs from within docker
$ ./run_tests.sh -l
py27-linux
py27-windows
py37-linux
py37-windows

# Equivalent to the above, but runs tox locally
$ tox -l
py27-linux
py27-windows
py37-linux
py37-windows

Hope that helps!

rafaelnp commented

Thanks for the tips, I am making some progress here. I forked the project and I am working on a branch called verilator.

rafaelnp commented

I was able to get a basic parsing for verilator to work, but the results are not shown on neovim.
The _mapLibrary and _creatyLibrary methods just have a pass, Verilog and SystemVerilog provide all the language features direclty built-in in, no additional library declarations/includes are needed. Maybe tis issue is related to it. It would be nice if you could take a look and give me your feedback. Let me know if you need more infos.

The commit you can find here.

Below the logging for the committed code:

INFO    | 11:15:30 | hdl_checker.database @ addSource():150 Thread-3 |	Adding /home/rnp/tmp/simple_mux/simple_mux.v, library=None, flags=(single=(), dependencies=())
DEBUG   | 11:15:30 | hdl_checker.database @ _parseSource():420 Thread-3 |	Parsing /home/rnp/tmp/simple_mux/simple_mux.v
DEBUG   | 11:15:30 | hdl_checker.lsp @ _handleUiInfo():122 Thread-3 |	UI info: Added 1 sources (workspace=<pyls.workspace.Workspace object at 0x7f681d2b0850>)
DEBUG   | 11:15:30 | hdl_checker.base_server @ _updateConfigIfNeeded():227 Thread-3 |	Updated config file to WatchedFile(path=Path('/tmp/hdl_checker_project_pid294155.json'), last_read=1582625729.6559722, origin=<ConfigFileOrigin.generated: 'generated'>)
INFO    | 11:15:30 | hdl_checker.database @ getLibrary():389 Thread-3 |	Library for '/home/rnp/tmp/simple_mux/simple_mux.v' not set, inferring it
DEBUG   | 11:15:30 | hdl_checker.database @ _inferLibraryForPath():485 Thread-3 |	Units defined here in /home/rnp/tmp/simple_mux/simple_mux.v: ["VerilogDesignUnit(name='simple_mux', type=entity, owner='/home/rnp/tmp/simple_mux/simple_mux.v')"]
DEBUG   | 11:15:30 | hdl_checker.database @ getLibrariesReferredByUnit():527 Thread-3 |	Searching for uses of VerilogIdentifier('simple_mux')
INFO    | 11:15:30 | hdl_checker.database @ _inferLibraryForPath():499 Thread-3 |	Couldn't work out a library for path /home/rnp/tmp/simple_mux/simple_mux.v
DEBUG   | 11:15:30 | hdl_checker.database @ getDependenciesUnits():680 Thread-3 |	Searching {Path('/home/rnp/tmp/simple_mux/simple_mux.v')} resulted in dependencies: set()
DEBUG   | 11:15:30 | hdl_checker.database @ getDependenciesUnits():704 Thread-3 |	Search paths: set()
INFO    | 11:15:30 | hdl_checker.database @ _getBuildSequence():818 Thread-3 |	Nothing more to do after 0 steps
DEBUG   | 11:15:30 | hdl_checker.base_server @ _getBuilderMessages():422 Thread-3 |	Built dependencies, now actually building '/home/rnp/tmp/simple_mux/simple_mux.v'
INFO    | 11:15:30 | hdl_checker.builders.verilator @ build():426 Thread-3 |	Forcing build of /home/rnp/tmp/simple_mux/simple_mux.v
INFO    | 11:15:30 | hdl_checker.builders.verilator @ _buildSource():137 Thread-3 |	detected valid file: '/home/rnp/tmp/simple_mux/simple_mux.v'
INFO    | 11:15:30 | hdl_checker.builders.verilator @ _buildVerilog():163 Thread-3 |	verilator command line: '['verilator_bin', '--lint-only', '-Wpedantic', '-Wall', '/home/rnp/tmp/simple_mux/simple_mux.v']'
DEBUG   | 11:15:30 | hdl_checker.utils @ runShellCommand():259 Thread-3 |	verilator_bin --lint-only -Wpedantic -Wall /home/rnp/tmp/simple_mux/simple_mux.v
DEBUG   | 11:15:30 | hdl_checker.utils @ runShellCommand():275 Thread-3 |	Command '['verilator_bin', '--lint-only', '-Wpedantic', '-Wall', '/home/rnp/tmp/simple_mux/simple_mux.v']' failed with error code 1.
Stdout:
%Error: /home/rnp/tmp/simple_mux/simple_mux.v:15: syntax error, unexpected reg, expecting IDENTIFIER
 reg bar;
 ^~~
%Error: /home/rnp/tmp/simple_mux/simple_mux.v:32: syntax error, unexpected ';'
  bar == 0;
          ^
%Error: Exiting due to 2 error(s)
DEBUG   | 11:15:30 | hdl_checker.base_server @ _buildAndHandleRebuilds():454 Thread-3 |	Had no rebuilds for /home/rnp/tmp/simple_mux/simple_mux.v
DEBUG   | 11:15:30 | hdl_checker.base_server @ _saveCache():286 Thread-3 |	Saving state to '/home/rnp/tmp/simple_mux/.hdl_checker/cache.json'

I am running verilator against the following file:

module simple_mux(
	input [1:0] x,
	input [4:0] y,
	input [11:0] in,
	input s,
	output reg [1:0] m
	);

	this_is_an_error

	reg bar;

	// the line bellow introduces a warning
	wire [7:0] foo = in[11:0] + 3'b1;

	always @(x or y or s)
	begin
		if (s == 0)
			m = y;
		else
			m = x;

		bar <= foo;
	end

	always @(s)
	begin
		bar == 0;
	end
endmodule: simple_mux

The project configuration file:

{
  "sources": [
    "/home/rnp/tmp/simple_mux/*.v",
  ],
}

My neovim + coc configuration:

    "hdlchecker": {
       "command": "hdl_checker",
       "args": ["--lsp", "--log-level", "DEBUG"],
       "filetypes": ["vhdl", "verilog", "systemverilog"]
    }

Version information:

  • neovim: v0.4.3
  • node: v10.19.0
  • coc.nvim: 0.0.74-6700e7468d
  • os: Linux 5.4.15-arch1-1
  • python: 3.8.1
suoto commented

First of all, sorry for the long time to get back on this.

I tried cloning your repo and could not get it do reproduce this, I'm guessing not everything has been committed.

By the log it seems that parsing the command's output is not matching anything. This line here says which command was run and its output (which looks alright), but after this there should be messages reporting the parsed reports.

DEBUG   | 11:15:30 | hdl_checker.utils @ runShellCommand():275 Thread-3 |	Command '['verilator_bin', '--lint-only', '-Wpedantic', '-Wall', '/home/rnp/tmp/simple_mux/simple_mux.v']' failed with error code 1.
Stdout:
%Error: /home/rnp/tmp/simple_mux/simple_mux.v:15: syntax error, unexpected reg, expecting IDENTIFIER
 reg bar;
 ^~~
%Error: /home/rnp/tmp/simple_mux/simple_mux.v:32: syntax error, unexpected ';'
  bar == 0;
          ^
%Error: Exiting due to 2 error(s)

The most helpful tool I found to get this right is https://regex101.com/ (make sure to select Python flavour, the default is PCRE, and to put the right flags). You can run the regex on sample text and make sure the groups are correct.

After some fiddling, I got this regex that seems to work:

  _stdout_message_scanner = re.compile(
      r"""
      ^%(?P<severity>((\w+\-\w+)|(\w+)))\:\s*
      (?P<filename>[^:]+\.s?v):
      (?P<line_number>\d+)\:\s*
      (?P<error_message>.*)""",
      flags=re.VERBOSE,
  ).finditer

There's also other suggestions (as it seems your local copy is newer these might be out of date already)

  • Add Verilator to the AVAILABLE_BUILDERS list inside /hdl_checker/builder_utils.py
  • Inside /hdl_checker/builders/verilator.py
    • There's no need to run /bin/sh in runShellCommand(["/bin/sh", "verilator", "--version"]), just runShellCommand(["verilator", "--version"]) is enough
    • Prefer runShellCommand(["verilator", "--version"]) over runShellCommand("verilator --version", shell=True)
    • When extracting Verilator's version, you're returning a list, ideally that should return a string
  • I noticed some mypy warnings regarding to typing, just make sure you use it to check your Python code it does help quite a bit
  • Inside .ci/scripts/Dockerfile there's no need to run COPY verilator $BUILDERS/verilator because it is installed alongside the other packages. ModelSim is copied from the file system; it gets downloaded before building the Docker container to avoid having to re-download it all the time. Vivado is mostly the same, except I manually strip stuff out to reduce the size.

If you prefer asking stuff on Gitter.im, I think it's easier to help in smaller things really.

rafaelnp commented

In fact, one file was not committed, my bad. I committed it here, it sets verilator as first builder to be used, you should be able to reproduce it now.

The committed code is parsing and matching the warnings and error. I used the same web site (https://regex101.com/) to generate the regex, the only difference between what I committed is the last line, to match warnings as well.

    _stdout_message_scanner = re.compile(
        r"""^\%(?P<severity>((\w+\-\w+)|(\w+)))\:
        \s*(?P<filename>\w+\.(v|sv))\:
        (?P<line_number>\d+)\:\s*
        (?P<error_message>(.*\n.*\n.*)|(.*\n.*))""",
        flags=re.VERBOSE,
    ).finditer

What I could observe, is that verilator stops on the errors, once the are corrected it shows the warnings. The logging error messages are the matched contents from the regex. The problem is to pass this information (regex matching) correctly to neovim to show it to the user. So far I did not succeed, if I make any progress, I shall write it here..

Regarding your suggestions:

  1. I used runShellCommand("verilator --version", shell=True) because the verilator file is a perl script, not an ELF binary. Otherwise I get an Command '['verilator', '--version']' failed with [Errno 8] Exec format error: 'verilator'

  2. I did not know this mypy, I will take a look into it.

  3. I shall remove the COPY verilator $BUILDERS/verilator

  4. Thanks for the Gittter.im link, I alread signed in.

suoto commented

What I could observe, is that verilator stops on the errors, once the are corrected it shows the warnings. The logging error messages are the matched contents from the regex. The problem is to pass this information (regex matching) correctly to neovim to show it to the user. So far I did not succeed, if I make any progress, I shall write it here

That's were I think the unit tests really help, they usually catch the small details that might be breaking (esp chasing which stderr log file is the latest one).

I used runShellCommand("verilator --version", shell=True) because the verilator file is a perl script, not an ELF binary. Otherwise I get an Command '['verilator', '--version']' failed with [Errno 8] Exec format error: 'verilator'

I saw somewhere you were using verilator_bin, so I assumed they were the same.

  • 👍1
rafaelnp commented

The unit tests are a great idea, thanks for pointing it out. I am gonna dive into it.
Nevertheless, it would speed up this part of development process, to write a wiki page documenting the following points/steps:

  • add a new linter
  • to run them
  • run specific tests
  • add new tests
  • tweak the tox.ini file

I would gladly help in this effort. :)

Regarding the runShellCommand I am gonna change it for sure, but first I would like to get the verilator up and running.

rafaelnp commented

Following your suggestion, I started the playing with the unit tests, got 57% coverage according to Tox. I pushed these changes already. There is more to do for sure. 👍

However I noticed, the _makeRecords method is not called, only the _buildSource, in the verilator builder. I suppose I am missing some concept/connection in the builder hierarchy, so the former method is not called. I took a look at the MSim, Ghdland Xvhdl classes and could not figure it out myself so far. Maybe you could shed some light on it? :-)