Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix _yosys_version() #364

Merged
merged 2 commits into from Apr 22, 2020
Merged

Fix _yosys_version() #364

merged 2 commits into from Apr 22, 2020

Conversation

hofstee
Copy link
Contributor

@hofstee hofstee commented Apr 22, 2020

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
Copy link

codecov bot commented Apr 22, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

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.

Co-Authored-By: whitequark <whitequark@whitequark.org>
@whitequark whitequark merged commit 875579e into amaranth-lang:master Apr 22, 2020
@whitequark
Copy link
Member

Thanks!

@hofstee hofstee deleted the patch-1 branch April 22, 2020 12:26
@hofstee
Copy link
Contributor Author

hofstee commented Apr 22, 2020

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
Copy link
Member

Verific::TimeBomb::TimeBomb()

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

@whitequark
Copy link
Member

See YosysHQ/yosys#1980.

@whitequark
Copy link
Member

whitequark commented Apr 22, 2020

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

@whitequark
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants