emsec / hal

HAL – The Hardware Analyzer
MIT License
619 stars 74 forks source link

Verilator simulation won't run in project directories containing spaces #496

Closed RenWal closed 1 year ago

RenWal commented 1 year ago

Describe the bug When creating a project directory containing spaces, such as /home/user/hal_projects/foo bar/, executing a simulation using netlist_simulator_controller with the verilator simulation engine will fail.

To Reproduce Steps to reproduce the behavior:

  1. Import the toy cipher netlist into a project directory containing at least one space
  2. Load the example script for the new simulator and execute it
  3. Observe that [info] Simulation engine process error appears in the log
  4. Repeat the sequence in a project directory that does not contain a space and observe that the simulation succeeds

This is caused by the verilator plugin attempting to run make in the project directory, which fails. https://github.com/emsec/hal/blob/46ea8210dca83db3c82c8573ba19ed8972dcbfb5/plugins/simulator/verilator/src/verilator.cpp#L234

There happens to be a long-standing bug (20th anniversary was not long ago …) in GNU Make that causes this, resulting in the appropriate error message in the engine_log.html:

/usr/share/verilator/include/verilated.mk:43: *** Unsupported: GNU Make cannot build in directories containing spaces, build elsewhere: '/home/user/hal_projects/foo bar/hal_simulation_sim_controller1_YYUrsX/obj_dir'. Schluss.

Expected behavior The simulation should not fail. If it must fail, there should be some indication in the UI as to why. Currently one has to look into the engine_log.html in the simulator working directory, the UI log only shows the generic engine process error one-liner (as a least-effort solution, I guess something like "engine process error, look into $workingDir/engine_log.html for details" would be nice?).

Desktop (please complete the following information):

Additional context There exists a cascade of secondary bugs that is triggered by the simulation engine failing in this way. While not critical, this led to some head-scratching during debugging because of weird behavior; so maybe when we fix the above, we can fix those as well:

When verilator does not run to completion, finalize() is never called. This results in mResultFilename never being set, which in turn leads to a read from uninitialized memory in the simulator controller: https://github.com/emsec/hal/blob/46ea8210dca83db3c82c8573ba19ed8972dcbfb5/plugins/simulator/netlist_simulator_controller/src/netlist_simulator_controller.cpp#L725

There is a check in the simulator controller that is supposed to verify that the verilator output file actually exists. However, in the case where we get lucky and don't crash here due to memory access violation, resultFile is now most likely the empty path. This causes the next few lines of code to prepend the working directory to get the absolute path. Hence, the following exists() check passes, but the code inspected the working directory itself, not the actual result file. In the end, we don't bail out but pass this file name to the VCD parser, which then attempts to open a directory as a file and fails. Cannot open VCD input file then appears in the log.

joern274 commented 1 year ago

Users should be aware that directory names containing spaces will cause a lot of trouble, especially when working with applications that are provided for more than one platform.

In this particular case it is gmake refusing to compile the Verilator if a space in directory name was detected. The error will be shown when checking the output of Verilator.

To increase the awareness of the problem users will receive a log message when attempting to run a simulation in a directory containing a space and the simulator controller will not be instantiated.