Fix `_yosys_version()` #364

hofstee · 2020-04-22T11:23:27Z

re.match only matches at the beginning of the string, not each line. This change changes it to do a re.search with the multiline flag. I think this is primarily of use when the user has compiled yosys with verific extensions.

Example of yosys -V output that failed with the prior code:

~/s/nmigen · yosys -V
-- (c) Copyright 1999 - 2020 Verific Design Automation Inc. All rights reserved
-- Compilation time was Fri Feb 28 11:19:43 2020

-- This program will expire on Sun Jun  7 12:19:43 2020

Yosys 0.9+2406 (git sha1 cf716e1f, gcc 8.3.0-6 -Os)

Verilog generation is still borked. yosys -q - doesn't seem to stop the Verific license from being printed, so it gets captured in the subprocess stdout when nmigen grabs the output from yosys. It might be a bit more foolproof to modify that to write the data to a tempfile instead and pass the file between nmigen/yosys.

-- (c) Copyright 1999 - 2020 Verific Design Automation Inc. All rights reserved
-- Compilation time was Fri Feb 28 11:19:43 2020

-- This program will expire on Sun Jun  7 12:19:43 2020

/* Generated by Yosys 0.9+2406 (git sha1 cf716e1f, gcc 8.3.0-6 -Os) */

(* generator = "nMigen" *)
(* top =  1  *)
(* \nmigen.hierarchy  = "top" *)
module top(clk, rst, o);

codecov · 2020-04-22T11:39:29Z

Codecov Report

Merging #364 into master will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #364      +/-   ##
==========================================
+ Coverage   82.27%   82.55%   +0.28%     
==========================================
  Files          35       35              
  Lines        5849     5950     +101     
  Branches     1207     1210       +3     
==========================================
+ Hits         4812     4912     +100     
+ Misses        872      871       -1     
- Partials      165      167       +2

Impacted Files	Coverage Δ
nmigen/back/verilog.py	`67.56% <100.00%> (ø)`
nmigen/hdl/cd.py	`100.00% <0.00%> (ø)`
nmigen/hdl/dsl.py	`99.73% <0.00%> (+<0.01%)`	⬆️
nmigen/build/dsl.py	`96.27% <0.00%> (+0.02%)`	⬆️
nmigen/hdl/ir.py	`95.41% <0.00%> (+0.02%)`	⬆️
nmigen/back/rtlil.py	`81.57% <0.00%> (+0.02%)`	⬆️
nmigen/hdl/mem.py	`98.16% <0.00%> (+0.03%)`	⬆️
nmigen/back/pysim.py	`91.04% <0.00%> (+0.05%)`	⬆️
nmigen/hdl/ast.py	`88.77% <0.00%> (+0.10%)`	⬆️
nmigen/hdl/xfrm.py	`96.24% <0.00%> (+0.14%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e18429...963ac24. Read the comment docs.

whitequark · 2020-04-22T11:50:18Z

yosys -q - doesn't seem to stop the Verific license from being printed, so it gets captured in the subprocess stdout when nmigen grabs the output from yosys.

I wonder if we can fix that upstream, or if that's something Verific forces on us.

It might be a bit more foolproof to modify that to write the data to a tempfile instead and pass the file between nmigen/yosys.

The current behavior is very much intentional; eventually I would like to ship Yosys (in the default configuration at least) as a library (likely compiled as WASM, but other options would work too), and I find it rather distasteful when libraries communicate with their host application through files. You are right that it would be a bit more foolproof though.

whitequark · 2020-04-22T11:51:28Z

nmigen/back/verilog.py

@@ -16,7 +16,7 @@ class YosysError(Exception):
 def _yosys_version():
    yosys_path = require_tool("yosys")
    version = subprocess.check_output([yosys_path, "-V"], encoding="utf-8")
-    m = re.match(r"^Yosys ([\d.]+)(?:\+(\d+))?", version)
+    m = re.search(r"^Yosys ([\d.]+)(?:\+(\d+))?", version, flags=re.M)


A comment here would be handy:

Suggested change

m = re.search(r"^Yosys ([\d.]+)(?:\+(\d+))?", version, flags=re.M)

# If Yosys is built with Verific, then Verific license information is printed first.

m = re.search(r"^Yosys ([\d.]+)(?:\+(\d+))?", version, flags=re.M)

Co-Authored-By: whitequark <whitequark@whitequark.org>

whitequark · 2020-04-22T12:23:20Z

Thanks!

hofstee · 2020-04-22T12:28:38Z

I wonder if we can fix that upstream, or if that's something Verific forces on us.

I think it's Verific. Here's a relevant gdb backtrace:

(gdb) bt
#0  __GI___libc_write (fd=1, buf=0x17ea360, nbytes=80) at ../sysdeps/unix/sysv/linux/write.c:26
#1  0x00007ffff750a3bd in _IO_new_file_write (f=0x7ffff764b760 <_IO_2_1_stdout_>, data=0x17ea360, n=80) at fileops.c:1183
#2  0x00007ffff750975f in new_do_write (fp=0x7ffff764b760 <_IO_2_1_stdout_>, fp@entry=0x50, 
    data=0x17ea360 "-- (c) Copyright 1999 - 2020 Verific Design Automation Inc. All rights reserved\n", 
    to_do=to_do@entry=80) at libioP.h:839
#3  0x00007ffff750b509 in _IO_new_do_write (to_do=80, data=<optimized out>, fp=0x50) at fileops.c:430
#4  _IO_new_do_write (fp=fp@entry=0x7ffff764b760 <_IO_2_1_stdout_>, data=<optimized out>, to_do=80) at fileops.c:430
#5  0x00007ffff750b8f3 in _IO_new_file_overflow (f=0x7ffff764b760 <_IO_2_1_stdout_>, ch=10) at fileops.c:791
#6  0x00000000011c4ce5 in Verific::Message::MsgVaList(Verific::msg_type_t, char const*, unsigned long, char const*, __va_list_tag*) ()
#7  0x00000000011c51d8 in Verific::Message::Msg(Verific::msg_type_t, char const*, unsigned long, char const*, ...) ()
#8  0x00000000011c524c in Verific::Message::PrintLine(char const*, char const*, char const*, char const*, char const*) ()
#9  0x00000000011d4aba in Verific::TimeBomb::TimeBomb() ()
#10 0x0000000000cbebd9 in _GLOBAL__sub_I__ZN7Verific9veri_file14_all_librariesE ()
#11 0x00000000012604c5 in __libc_csu_init ()
#12 0x00007ffff74b302a in __libc_start_main (main=0x78f736 <main>, argc=3, argv=0x7fffffffe518, 
    init=0x1260480 <__libc_csu_init>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe508)
    at ../csu/libc-start.c:264
#13 0x00000000007a1e7a in _start ()

I tried looking through the yosys code earlier and couldn't see anything that seemed like it would be the cause of it. It seems like yosys doesn't print anything before the header, which gets disabled if you pass -q.

whitequark · 2020-04-22T12:29:17Z

Verific::TimeBomb::TimeBomb()

That's, uh, one way to name your license check function.

whitequark · 2020-04-22T12:30:43Z

See YosysHQ/yosys#1980.

whitequark · 2020-04-22T12:31:37Z

As a ~~temporary~~permanent workaround we could strip the initial lines that are empty or start with --, since such lines are not valid in RTLIL anyway.

whitequark · 2020-04-22T15:16:36Z

@hofstee Looks like it's going to be a permanent workaround. Feel free to submit a PR that adds such a hack, it's pretty bad but using temporary files in a situation like this is IMO also quite bad so I prefer the bad option that won't have any impact on people not using Verific.

Fix _yosys_version()

5587c97

whitequark reviewed Apr 22, 2020

View reviewed changes

Add comment about verific

963ac24

Co-Authored-By: whitequark <whitequark@whitequark.org>

whitequark merged commit 875579e into amaranth-lang:master Apr 22, 2020

hofstee deleted the patch-1 branch April 22, 2020 12:26

whitequark mentioned this pull request Apr 22, 2020

Verific unconditionally prints license information to stdout YosysHQ/yosys#1980

Closed

hofstee mentioned this pull request Apr 23, 2020

remove non-verilog code at start of output #365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `_yosys_version()` #364

Fix `_yosys_version()` #364

hofstee commented Apr 22, 2020

codecov bot commented Apr 22, 2020 •

edited

Loading

whitequark commented Apr 22, 2020

whitequark Apr 22, 2020

whitequark commented Apr 22, 2020

hofstee commented Apr 22, 2020

whitequark commented Apr 22, 2020

whitequark commented Apr 22, 2020

whitequark commented Apr 22, 2020 •

edited

Loading

whitequark commented Apr 22, 2020

	m = re.search(r"^Yosys ([\d.]+)(?:\+(\d+))?", version, flags=re.M)
	# If Yosys is built with Verific, then Verific license information is printed first.
	m = re.search(r"^Yosys ([\d.]+)(?:\+(\d+))?", version, flags=re.M)

Fix _yosys_version() #364

Fix _yosys_version() #364

Conversation

hofstee commented Apr 22, 2020

codecov bot commented Apr 22, 2020 • edited Loading

Codecov Report

whitequark commented Apr 22, 2020

whitequark Apr 22, 2020

Choose a reason for hiding this comment

whitequark commented Apr 22, 2020

hofstee commented Apr 22, 2020

whitequark commented Apr 22, 2020

whitequark commented Apr 22, 2020

whitequark commented Apr 22, 2020 • edited Loading

whitequark commented Apr 22, 2020

Fix `_yosys_version()` #364

Fix `_yosys_version()` #364

codecov bot commented Apr 22, 2020 •

edited

Loading

whitequark commented Apr 22, 2020 •

edited

Loading