YosysHQ / yosys

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

valgrind issue for `read_verilog -specify` on blackboxes #1736

Open eddiehung opened 4 years ago

eddiehung commented 4 years ago

Steps to reproduce the issue

read_verilog -specify <<EOT
(* blackbox *)
module top(input i, output o);
wire w;
specify
  (w => o) = 10;
endspecify
endmodule
EOT

Expected behavior

No valgrind errors.

Actual behavior

 Yosys 0.9+1706 (git sha1 7b543fdb, clang 9.0.0-2 -fPIC -Os)

-- Executing script file `issue.ys' --

1. Executing Verilog-2005 frontend: <<EOT
Parsing Verilog input from `<<EOT' to AST representation.
Generating RTLIL representation for module `\top'.
==8704== Invalid read of size 4
==8704==    at 0x694C7F: Yosys::AST::AstNode::genRTLIL(int, bool) (genrtlil.cc:1020)
==8704==    by 0x69432A: Yosys::AST::AstNode::genRTLIL(int, bool) (genrtlil.cc:1562)
==8704==    by 0x6716AD: Yosys::process_module(Yosys::AST::AstNode*, bool, Yosys::AST::AstNode*, bool) (ast.cc:1104)
==8704==    by 0x670567: Yosys::AST::process(Yosys::RTLIL::Design*, Yosys::AST::AstNode*, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool) (ast.cc:1219)
==8704==    by 0x66973A: Yosys::VerilogFrontend::execute(std::istream*&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cx
x11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, Yosys::RTLIL::Design*) (verilog_frontend.cc:468)
==8704==    by 0x56E0B3: Yosys::Frontend::execute(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, Yosys::RTLIL::Design*) (regi
ster.cc:450)
==8704==    by 0x56D3E5: Yosys::Pass::call(Yosys::RTLIL::Design*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >) (register.cc
:312)
==8704==    by 0x56D01B: Yosys::Pass::call(Yosys::RTLIL::Design*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (register.cc:289)
==8704==    by 0x5E5208: Yosys::run_frontend(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::al
locator<char> >*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, Yosys::RTLIL::Design*) (yosys.cc:969)
==8704==    by 0x55D917: main (driver.cc:517)
==8704==  Address 0x516fe64 is 4 bytes inside a block of size 264 free'd
==8704==    at 0x483BFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==8704==    by 0x6713FA: Yosys::process_module(Yosys::AST::AstNode*, bool, Yosys::AST::AstNode*, bool) (ast.cc:1078)
==8704==    by 0x670567: Yosys::AST::process(Yosys::RTLIL::Design*, Yosys::AST::AstNode*, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool) (ast.cc:1219)
==8704==    by 0x66973A: Yosys::VerilogFrontend::execute(std::istream*&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cx
x11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, Yosys::RTLIL::Design*) (verilog_frontend.cc:468)
==8704==    by 0x56E0B3: Yosys::Frontend::execute(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, Yosys::RTLIL::Design*) (regi
ster.cc:450)
==8704==    by 0x56D3E5: Yosys::Pass::call(Yosys::RTLIL::Design*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >) (register.cc
:312)
==8704==    by 0x56D01B: Yosys::Pass::call(Yosys::RTLIL::Design*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (register.cc:289)
==8704==    by 0x5E5208: Yosys::run_frontend(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::al
locator<char> >*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, Yosys::RTLIL::Design*) (yosys.cc:969)
==8704==    by 0x55D917: main (driver.cc:517)

It appears that during AST processing, blackbox modules will have everything inside it -- except for ports, parameters, and specify cells -- deleted: https://github.com/YosysHQ/yosys/blob/6eb528277e73a978ace7d6f693215d9ff92f898d/frontends/ast/ast.cc#L1055-L1088 Any AST_IDENTIFIER-s used inside specify cells that are not input/output ports will thus hold a dangling pointer in its id2ast member.

What is the best solution here? The LRM specifies that:

14.2.1 Module path restrictions Module paths have the following restrictions: The module path source shall be a net that is connected to a module input port or inout port. The module path destination shall be a net or variable that is connected to a module output port or inout port. The module path destination shall have only one driver inside the module.

-- concerningly, a net connected to a port is also allowed.

The LRM also says the following about state dependent paths (e.g. if (a) (i => o) = 10):

14.2.4.1 Conditional expression The operands in the conditional expression shall be constructed from the following: Scalar or vector module input ports or inout ports or their bit-selects or part-selects Locally defined variables or nets or their bit-selects or part-selects Compile time constants (constant numbers and specify parameters)

-- the second point of which really means that it could really be anything in the design, and throwing it all away may not be the best idea.

eddiehung commented 4 years ago

Found when debugging an inconsistent CI failure: https://travis-ci.org/YosysHQ/yosys/builds/658368799?utm_medium=notification&utm_source=github_status that was caused by the accidental use of an implicit signal that was fixed here: https://github.com/YosysHQ/yosys/commit/0930c00f038453685bf4d8f5366db7fe71f54cf8#diff-c8f274104b91615dd258e9b2c4e7c8c0L974-L1006

However, even the accidental use of a non-existent signal should not cause an invalid read.