leonardt / fault

A Python package for testing hardware (part of the magma ecosystem)
BSD 3-Clause "New" or "Revised" License
41 stars 13 forks source link

Working with external verilog files #139

Closed sgherbst closed 5 years ago

sgherbst commented 5 years ago

These commits span updates to the ncsim, vcs, and iverilog simulators in SystemVerilogTarget to make it easier to work with external files:

  1. There is no file added to the source file list for the DUT itself when ext_model_file=True. This, combined with the existing ext_libs option, gives the user an option to point to external verilog files rather than copying them over to the build directory outside of fault. Perhaps in the future, when skip_compile=True, ext_model_file should default to True as well. But I didn't want to break existing programs that depend on the current behavior.
  2. The generation of commands for ncsim, vcs, and iverilog are moved into separate methods to make it easier to make simulator-specific updates in the future.
  3. The files specified by the ext_libs argument are included as libraries ("-v" or "-y", depending on the simulator) rather than directly as source files. This has the benefit of making their order unimportant. The behavior of the include_verilog_libraries argument is unmodified, although it's worth noting that it really is including files as verilog sources, not libraries. As an implementation note -- iverilog only supports libdirs, not lib files, so a set of unique directories is extracted from the ext_lib argument.
  4. The use of subprocess to call the three simulators is pulled into the method subprocess_run to avoid code duplication for shell escaping, subprocess run options, and logging of outputs. As a side note, it would be good to investigate why shell=True is needed -- test_power_domains fails without it on Travis but passes on my setup. For now I left shell=True option in.
  5. A new test, test_ext_vlog.py, exercises the above features.
sgherbst commented 5 years ago

Not sure why GarnetFlow is failing -- does anyone know what these errors mean? It doesn't look like the check is getting to the point that it can run fault tests.

error: patch failed: CMakeLists.txt:20
error: CMakeLists.txt: patch does not apply
error: patch failed: src/passes/analysis/verilog.cpp:2
error: src/passes/analysis/verilog.cpp: patch does not apply
./install.sh: line 13: cd: coreir/build: No such file or directory
python: can't open file 'python_repo.py': [Errno 2] No such file or directory
leonardt commented 5 years ago

CoreIR just merged a major update to the verilog backend/toolchain, it looks like this broke a downstream tool flow, we will investigate, but this is not your fault.

sgherbst commented 5 years ago

Just re-ran the tests after updates to GarnetFlow and it looks like everything is passing now.

leonardt commented 5 years ago

shell=True is used to make it easier to pass through shell commands. Typically, you need to pass the commands as a list of strings rather than a single command string. There are various reason fo this being the standard interface (programmatically building commands, preventing shell injection) but usually shell=True is simpler (re: quicker) to use. It's likely that we can't remove shell=True because there's some part of the command construction process that isn't properly splitting the command line arguments into separate strings.