YosysHQ / yosys

Yosys Open SYnthesis Suite
https://yosyshq.net/yosys/
ISC License
3.36k stars 871 forks source link

Regression: synthesis fails when file names have spaces in path #1467

Open jpakkane opened 4 years ago

jpakkane commented 4 years ago

Not sure if this is a bug in yosys for generating broken files or in arachne-pnr for not parsing the input correctly. In either case if you first compile a .v file with in a path with a space in it with yosys and then try to create the asc file with arachne-pnr, it will error out with the following message:

fatal error: invalid format-actual

This is currently breaking Meson's unit tests and causing problems in Debian. To replicate the actual issue, check out Meson from git, cd to its root and do:

./meson.py test\ cases/fpga/1\ simple build
ninja -C build
daveshah1 commented 4 years ago

Running this random small test (I didn't bother trying to get the whole of Meson up and running):

% cat test\ path\ with\ spaces/top.v                                                   :(
module top(input [3:0] a, b, c, output [3:0] y);
    assign y = (a + b) ^ (a + c);
endmodule
% yosys -p "synth_ice40 -blif top.blif" test\ path\ with\ spaces/top.v

confirms that Yosys is incorrectly including unescaped paths in netnames on ports like

.gate SB_LUT4 I0=$abc$112$add$test path with spaces/top.v:2$1_Y[3] I1=c[3] I2=a[3] I3=$auto$alumacc.cc:485:replace_alu$15.C[3] O=y[3]

Are you aware when this regression started? Looking at the Yosys 0.8 build I have around, it looks like the frontend was already creating netnames with spaces; but in those days the abc pass threw away most net names so it wasn't causing a problem in the output (this was very much not a good feature). It seems like the "regression" might be abc now preserving these dodgy net names.

jpakkane commented 4 years ago

Are you aware when this regression started?

I noticed this when the Yosys package in Debian was updated some days ago. The full information is over here, but it seems that the version changed from 0.8+20190328git32bd0f2 to 0.9.

daveshah1 commented 4 years ago

Looking into a fix for this; I see two possible options:

(1) spaces inside identifiers are considered illegal in Yosys. An assertion should probably be added in a suitable location (either IdString constructor or check()?) to ensure this invariant is not violated. Frontends are modified as needed to prevent spaces from filenames entering cell/net names. This seems like the best way forward to me.

(2) spaces inside identifiers are considered acceptable; and backends are modified to escape them accordingly

any thoughts @cliffordwolf / @eddiehung ?

eddiehung commented 4 years ago

I am undecided right now. Verilog identifiers cannot contain spaces, but VHDL (extended) identifiers can... EDIT: I'd like to go with (1) but there's the issue above to deal with...

daveshah1 commented 4 years ago

As ilang uses space-terminated identifiers too, it also can't handle spaces in identifiers at present and would need changes to support identifiers with spaces as a first-class feature

whitequark commented 4 years ago

My view is that RTLIL should be changed to allow spaces in identifiers. @eddiehung mentions that VHDL allows them, and of course there is nothing special about spaces in nMigen as well. I can implement support for spaces in RTLIL if necessary.

daveshah1 commented 4 years ago

The main change in that case would be escaping them in the range of backends that don't allow spaces in identifiers. Such escaping would also have to be careful of conflicts between escaped names and existing unescaped names.