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

CMake support #1363

Closed
veripoolbot opened this issue Oct 24, 2018 · 71 comments
Closed

CMake support #1363

veripoolbot opened this issue Oct 24, 2018 · 71 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Oct 24, 2018


Author Name: Patrick Stewart
Original Redmine Issue: 1363 from https://www.veripool.org

Original Assignee: Patrick Stewart


I've added support for building with CMake (the verilated output, not verilator itself), and for generating a python module as output using pybind11. The code is here: https://github.com/patstew/verilator/tree/python
I've added documentation and examples for each, hopefully they're clear enough.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 25, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-10-25T00:21:37Z


Nice patch set, you seem to have mastered the key stuff.

Though not explicitly stated I assume your goal is to eventually get this merged upstream so here's some comments.

First, please separate out the python and cmake patches (it seems python needs cmake though so would build on that patch), then when ready (below)
file a new bug for python.

As to the cmake patch my main concern is I haven't used CMake and when there are bugs a few years from now related to CMake are you willing to signup to support fixing them?

The same applies to the python code, but here I need to soul search a bit as to if I want to accept it. This feels more of something for your personal use case and unlikely to be widely useful. Perhaps use it for at least a few months and see how it settles out.

--- a/bin/verilator
+++ b/bin/verilator

  • --cmake Generate CMake file list instead of MakeFile

Maybe it should be --make cmake (versus the default "--make gmake" similar to the "--compiler gcc" flag), so when yet another build system we don't need to keep adding unique flags?

  • verilate(target TRACE SYSTEMC NAME name TOP top SOURCES ... ARGS ...
  •         INCLUDEDIRS ... SLOW_FLAGS ... FAST_FLAGS ...)
    

+Lowercase and ... should be replaced with arguments, the uppercase parts delimit
+the arguments and can be passed in any order, or left out entirely if optional.

I think we should make the flag names close or identical to existing makefile arguments out of clarity.

+=item NAME

PREFIX?

+=item TOP

TOP_MODULE?

+Optional. Sets the name of the top module. Defaults to the name of the first
+file in the SOURCES array.

+=item ARGS

VERILATOR_FLAGS?

+=item INCLUDEDIRS

INCLUDE_DIRS?

but is this critical? Expect -f lists are the commoner case.

+=item SLOW_FLAGS

OPT_SLOW

+=item FAST_FLAGS

OPT_FAST

+++ b/examples/cmake

Example names should perhaps be cmake_c and python_c, since they assume C code generation.

Please add comment headers similar to the existing example files.

Call the written vcd file logs/vlt_dump.vcd to match tracing_c example.

"make examples" on the top directory fails as no Makefile is found - think you need one just to run your steps documented.

Following your instructions I get this:

CMake Error at CMakeLists.txt:11 (cmake_policy):
Policy "CMP0074" is not known to this version of CMake.

+++ b/examples/cmake/.gitignore

Please also in gitignore include the files ignored in tracing_c/.gitignores.

Fix some missing trailing newline.

+++ b/examples/python/pyexample.cpp

  • //declare_globals declares the common parts of verilator (Verilated class and tracing classes)

Everywhere add one space after // or # comment chars.

+++ b/include/verilated_py.h
+#include "verilated.h"
+#include <pybind11/pybind11.h>
+#ifdef VM_TRACE
+#ifdef VM_TRACE_FST
+#include "verilated_fst_c.h"
+#else
+#include "verilated_vcd_c.h"
+#endif
+#endif

Indent #if blocks appropriately "# ifdef" etc.

+namespace vl_py {

VerilatedPy

+inline void declare_globals(pybind11::module& m) {

  • m.def("VL_THREAD_ID", &VL_THREAD_ID);

Verilator doesn't currently require C++11, and this seems to. Perhaps it's ok to require C++ for this mode, but the test needs to be skipped when not configured for C++11.

+++ b/src/V3EmitCBase.h

  • AstCFile* newCFile(const string& filename, bool slow, bool source) {
  • static AstCFile* newCFile(const string& filename, bool slow, bool source) {

Indeed. Pushed this.

+++ b/src/V3EmitCMake.cpp

+static void cmake_set(std::ofstream& of, const string& name, const string& value,

  •                    const string& cache_type = "", const string& docstring = "") {
    

Rather than pollute the global namespace, put these into your class.

+static string quote(const string& s) {

Quote is unused, remove.

+static string normalise(string s) {

Sorry, I'm American so "normalize". ;) Is non-windows happy with this?

+++ b/verilator-config.cmake
+#Check flag support. Skip on MSVC, these are all GCC flags.
+if (NOT (DEFINED VERILATOR_CFLAGS OR CMAKE_CXX_COMPILER_ID MATCHES MSVC))

  • include(CheckCXXCompilerFlag)
  • foreach (FLAG faligned-new fbracket-depth=4096 Qunused-arguments Wno-bool-operation Wno-parentheses-equality
  •              Wno-sign-compare Wno-uninitialized Wno-unused-but-set-variable Wno-unused-parameter
    
  •              Wno-unused-variable Wno-shadow)
    

I'm scared about having to maintain this code in two places (configure.ac and here). Not sure how to deal with that.

Put the -'s in front of these as much better when grepping and some compilers use +'s.

foreach (FLAG -faligned-new -fbracket-depth=4096 -Qunused-arguments ...

You don't seem to check for the appropriate multithreaded flags.

Need a test_regress/t/t_flag_cmake.pl test outside the examples. If your scenario line is right this will also test multithreaded.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 25, 2018


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2018-10-25T01:24:49Z


That was quick. I'm actually off on holiday imminently, so won't get round to an updated patchset for a few weeks.

The cmake and python bits are already in separate commits, though the python one depends on the cmake one, the cmake one works alone. I can focus on the cmake one in this issue if you like.

Primarily I'm just punting this code out in the spirit of sharing. I would like to see it upstream, because it would make updating a lot easier for me, though it's completely understandable if you don't want to merge stuff you'll never use. As far as supporting it goes, I intend to keep it working for myself for the foreseeable future, and I might check on here occasionally or when I'm alerted, but I'm unlikely to add improvements that I don't need or respond to issues as quickly/comprehensively as you seem to.

I might well be in a minority of one with the python stuff, though I think it's pretty good for directly comparing a reference implementation of some DSP algorithm written with numpy etc to a verilog implementation. I've been doing it manually for a while, and got sick of writing the boilerplate, so decided to make these patches.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Apr 28, 2019


Original Redmine Comment
Author Name: Pieter Kapsenberg
Original Date: 2019-04-28T23:49:00Z


Any movement on this Patrick? CMake is pretty popular, getting this in would make it so much easier to use Verilator in all kinds of other projects.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 24, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-24T19:27:01Z


Hey

I've rebased the cmake patch and updated it a bit.
It would be nice if someone would review them.

The code is at madebr/verilator#1

changes:

  • Rebased on top of current master
  • Added more documentaion to the cmake example
  • src/V3EmitMake.cpp: escape all variables
  • src/V3Options.cpp: add --make cmake option for cmake.
    --make gmake is the default. When --make cmake option is chosen, gmake wil not be built.
  • cmake and gmake can both be built using --make cmake --make gmake.
  • Add cmake support to the regression test. One test is using cmake.
  • Add general files of regression tests to distfiles
    This should probably be modified such that all tests can be built using cmake.
    But for that, I'll need further guidance.
  • verilator-config.cmake: prepend - to tested CFLAGS
  • verilator-config.cmake: rename NAME to PREFIX
  • verilator-config.cmake: default topmodule name is the name of first hdl file
  • verilator-config.cmake: default prefix is V + topmodule name
  • verilator-config.cmake: use properties and generator expressions to store and set the requested parameters for COVERAGE, TRACE, SYSTEMC and THREADS .
  • verilator-config.cmake: collect the compiler arguments from the configure script and add these to the verilator-config.cmake.
  • verilator-config-version.cmake: users can set a minimum version using e.g. find_package(verilator 4.0 REQUIRED)
  • Makefile.in: verilator-config.cmake is installed to $prefix/lib/cmake/verilator. VERILATOR_ROOT can still be overriden by setting it to another value.

questions/todos:

  • Is running one cmake test ok? How would you like to see the cmake test implemented?
  • cmake regression tests fail on travis because the wrong source directory is passed. It works when a test is run individually. How to fix?

Things I did not change:

  • I have kept SLOW_FLAGS and FAST_FLAGS because I feel these names are more cmake then OPT_FLOW and OPT_FAST. But tastes differ. Let me know if you insist to change.

Greetings

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 25, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-25T02:16:53Z


Nice job.

  1. Please sumbit a one-time independent patch to edit docs/CONTRIBNUTORS to
    add your name and certify this and future contributions are open sourced
    under the Developer Certificate of Origin.

  2. Separately or as part of above please patch the couple of typos you
    fixed (e.g. "Generally", "inatall") and anything else not cmake related
    (some additions to Makefile?)

    --- a/Makefile.in

    • $(INSTALL_DATA) verilator-config.cmake $(DESTDIR)$(datarootdir)/cmake/verilator
    • $(INSTALL_DATA) verilator-config-version.cmake $(DESTDIR)$(datarootdir)/cmake/verilator

Is what cmake ususally wants a "cmake" directory with all tools underneath it?

--- a/README.pod
+    examples/cmake              => Example building hello_world_c with CMake

I think this should be examples/cmake_c to match the other examples (to
leave room for a theoretical future cmake_sc).

--- a/bin/verilator
+    --make <build-system>       Generate scripts for specified build system

Given you call it build system I wonder if this should be --build, or you should say
"--make "?

+=item --make <build-system>
+
+Generates a script for the specified build system.

Please also describe that the options are gmake, cmake or both.

+=head2 CMake
+
+Verilator can be run using CMake, which takes care of both running verilator and

Please capitalize Verilator and Verilated.

Please add the cmake related files to the FILES documentation section.

--- a/configure.ac
@@ -183,6 +183,7 @@ AC_DEFUN([_MY_CXX_CHECK_SET],
 AC_DEFUN([_MY_CXX_CHECK_OPT],
    [# _MY_CXX_CHECK_OPT(flag) -- Check if compiler supports specific options
+    CXX_CHECKED_OPTS+=($2)

+CXX_CHECKED_OPTS=()
@@ -288,6 +290,8 @@ _MY_CXX_CHECK_OPT(CFG_CXXFLAGS_NO_UNUSED,-Wno-unused-variable)
+AC_SUBST(VERILATE_FLAGS, [[${CXX_CHECKED_OPTS[@]}]])

Why is VERILATE_FLAGS made this way versus using the existing CFG_CXXFLAGS_NO_UNUSED?
Also having MY_CXX_CHECK_OPT having to override this doesn't seem right.

+++ b/src/V3EmitCMake.cpp
+// Concatenate all strings in 'strs' with 'joint' between them.
+template<typename List>
+static string cmake_list(const List& strs, const string& joint=" ") {

Doesn't need to be templated. Move to inside V3EmitCMake class.

+        for (typename List::const_iterator i = ++strs.begin(); i != strs.end(); i++) {

Use "it" instead of "i". Use "++it" instead of "it++". Use "out" instead of "s".

+static void cmake_set_raw(std::ofstream& of, const string& name, const string& raw_value,
+                        const string& cache_type = "", const string& docstring = "") {
+static void cmake_set(std::ofstream& of, const string& name, const string& value,
+                        const string& cache_type = "", const string& docstring = "") {
+//Swap all backslashes for forward slashes, because of Windows
+static string normalize(string s) {

Likewise move inside V3EmitCMake.

+static string normalize(string s) {

Perhaps deslash instead of normalize? Input should be const string&.

+void V3EmitCMake::emit(AstNetlist* nodep) {
...
+    *of << "\n### Constants...\n";
+    cmake_set(*of, "PERL", normalize(V3Options::getenvPERL()), "FILEPATH", "\"Perl executable (from $PERL)\"");
+    cmake_set(*of, "VERILATOR_ROOT", normalize(V3Options::getenvVERILATOR_ROOT()), "PATH" ,"\"Path to Verilator kit (from $VERILATOR_ROOT)\"$
+    cmake_set(*of, "SYSTEMC_INCLUDE", normalize(V3Options::getenvSYSTEMC_INCLUDE()), "PATH", "\"SystemC include directory with systemc.h (fr$
+    cmake_set(*of, "SYSTEMC_LIBDIR", normalize(V3Options::getenvSYSTEMC_LIBDIR()), "PATH", "\"SystemC library directory with libsystemc.a (f$

I think there's a lot of duplication between here and V3EmitMk. Wonder if V3EmitMk should have a common
Netlist emitter that calls a base class, then we call that with a gmake and cmake base class?

+++ b/src/V3EmitCMake.h
+#endif // Guard

Nit, two spaces before any comment.

+++ b/src/V3File.cpp
+inline std::vector<string> V3FileDependImp::getAllDeps() {
+    std::vector<string> r;
+    for (std::set<DependFile>::iterator iter=m_filenameList.begin();

Should be able to be "V3FileDependImp::getAllDeps() const" and use a const_iterator. Ditto add const to related calling fuctions.

+++ b/src/V3Options.cpp
+                    fl->v3fatal("Unknown build system specified: " << argv[i]);

Use:

                     fl->v3fatal("Unknown build system specified: '"<<argv[i]<<"'");

(The convention recently changed to quote things in messages).

@@ -1114,6 +1124,10 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, char
+    // Make sure at least one build system is enabled
+    if (!m_gmake && !m_cmake) {
+        m_gmake = true;
+    }

Don't do this here as -f files will break, do it in Verilator.cpp after where it says e.g "Need -cc ...".
You'll probably need to make a new V3Options method "defaultOptions()" or something.

+++ b/verilator-config-version.cmake.in
+++ b/test_regress/CMakeLists.txt
+++ b/verilator-config.cmake.in

For these, please add a header of some sort to this, with Copyright and a DESCRIPTION at least.
Please add a space between "#" and starting comments, e.g. "# Check" not "#Check".
I don't know enough about cmake to comment about the real content.

+++ b/test_regress/driver.pl

Changes here seem reasonable. There's some commented out code you added,
not sure if that needs to be implemented or removed.

+++ b/test_regress/t/t_flag_make_cmake.pl

This needs to skip() if there's no cmake installed or < version 3.8.

Actually I think your example/cmake_c needs a similar check, for that see how SystemC is tested in the Makefile.

>Is running one cmake test ok? How would you like to see the cmake test implemented?

I think this is fine.

>cmake regression tests fail on travis because the wrong source directory
>is passed. It works when a test is run individually. How to fix?

Not sure why this is. Are you sure cmake is installed when travis runs?

>I have kept `SLOW_FLAGS` and `FAST_FLAGS` because I feel these names are 
>more cmake then `OPT_FLOW` and `OPT_FAST`. But tastes differ. Let me know
>if you insist to change.

I would prefer to stick with OPT_FAST and OPT_SLOW; I searched for SLOW_FLAGS and this doesn't seem any more standard (this thread is the first google hit ;).

Thanks, look forward to next patch set.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 25, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-07-25T10:34:40Z


Thanks for taking this on Maarten, I haven't had time for a while.
I've just a had a quick go with it, when I install it I end up with ``` set(PACKAGE_VERSION "4.017 devel") 0)


Also, the example doesn't build.

examples/cmake/CMakeLists.txt line 37 should be INCLUDE_DIRS

in verilator_config.cmake you've removed the bit that sets a default for TOP_MODULE when you don't specify it, so the example fails to build with 'cannot find module Vtop' (I think it ends up with --top-module --prefix Vtop, and sets both args to Vtop?).

You've moved the verilator-config.cmake file, which means it can't find verilated.cpp etc unless you explicitly set VERILATOR_ROOT. Part of the reason it was in the VERILATOR_ROOT folder is that it sets VERILATOR_ROOT automatically from the .cmake file location. VERILATOR_ROOT (<prefix>/share/<name>) is on the default cmake find_packages search path, it's equally valid to <prefix>/share/cmake/<name>. It also makes it possible to just set CMAKE_PREFIX_PATH to switch between verilator builds.

I've uploaded the fixes I made here for the minor issues above "here.":https://github.com/patstew/verilator/tree/madebrcmake Feel free to squash it into your PR.


Although SLOW_FLAGS itself isn't standard, CMake has lots of built in variables of the form *_FLAGS (https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html). It's not too important though.
@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 25, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-25T15:33:14Z


Hello Patrick,

Thanks for the comments!
I think I have incorporated your suggestions with some changes.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 25, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-25T15:34:52Z


Hello,

I think I have incorporated the comments in #5.

They can still be found at madebr/verilator#1

Because I've extracted the spelling fix to an external patch, I had to rebase and force push the series.

  • The search procedure of cmake is documented at: https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure .
    The previous location (/usr/lib/cmake/verilator) was correct, but I've changed it to $prefix/share/verilator/cmake.
    This location is better because using $ENV{VERILATOR_ROOT} as hint to find_package will pick up the correct Verilator.
  • I've renamed the help of --make to make-system and added gmake/cmake
  • configure.ac: I override the existing CFG_CXXFLAGS_NO_UNUSED because the configure script and verilate cmake script can be run on different computers and compilers. I added a comment to configure.ac.
  • src/V3EmitCMake.c: I renamed normalize to deslash
  • src/V3EmitCMake.c: I've made the argument of deslash const ref, but it does not matter because it returns a copy because it modifies its input.
  • `src/V3EmitCMake.c: I've kept the template for cmake_list because the arguments can be a V3StringList and V3StringSet
  • src/V3File.cpp: I've made V3FileDependImp::getAllDeps() const. The only user is V3File, which uses it in a static function.
  • src/V3Options.cpp: I've moved the lines that ensure that at least one make system is enabled to a notify function inside V3Options. This function should be called after all argument parsing is finished. I've borrowed this name from Boost.Program_options.
  • I have fixed my problem with the regression tests on travis.

Greetings

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 25, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-25T15:52:51Z


I've created a new pull request in which I add myself to docs/CONTRIBUTORS to certify that this and future contributions are open sourced under the Developer Certificate of Origin

Patch at madebr/verilator#2

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 25, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-25T22:50:43Z


Thanks for all the cleanups.

If you want just point me to a github branch and you can skip posting patch files.

configure.ac: I override the existing CFG_CXXFLAGS_NO_UNUSED because the
configure script and verilate cmake script can be run on different
computers and compilers. I added a comment to configure.ac.

Sorry, I'm not understanding how your change solves that problem.

+++ b/bin/verilator

  • --make Generate scripts for specified make system

s/-system-system/-system/

+=item --make

Use I before <> so pretty-prints nicely.

  • {prefix}.cmake

Please add trailing comment like other lines:

    {prefix}.cmake                      // CMake file for compiling


+template<typename List>
+static string cmake_list(const List& strs) {

Think earlier feedback in V3EmitCMake about removing template and moving to static class functions was missed.

+++ b/test_regress/driver.pl
+sub have_cmake {
  • return ($major > 3 ) || (($major == 3) && ($minor >= 8));

Please change to cmake_version, and have return floating version number which the caller then compares. (Some future new feature might require a larger version number and we'll be more ready.)

When I run examples I get:

-- Verilator cmake hello-world simple example
-- CMake ----------------
rm -rf build && mkdir build
cmake -S . -B build
CMake Error: The source directory "{...}/verilator/examples/cmake_c/build" does not appear to contain CMakeLists.txt.
Specify --help for usage, or press the help button on the CMake GUI.
Makefile:56: recipe for target 'run' failed

Note I do have VERILATOR_ROOT set.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 26, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-07-26T10:55:37Z


If you're putting it into share/verilator/cmake, you need to set the parent directory of ${CMAKE_CURRENT_LIST_DIR} on line 28 of verilator-config.cmake.in.

Wilson: You perhaps have cmake < 3.13 installed that doesn't support the -S and -B flags? Try: ```rm -rf build && mkdir build && cd build && cmake ..

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 26, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-26T11:20:42Z


I'm using cmake 3.10.2. I'm running "make examples" inside the parent git repo checkout (not installed).

 rm -rf build && mkdir build && cd build && cmake ..

This worked, presumably please update the examples/cmake_c/Makefile to work appropriately, thanks.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 26, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-26T14:22:12Z


Hello folks

@patrick Stewart

  • I have modified the make install to copy the cmake scripts to /usr/share/verilator (no cmake subdirectory). That way the cmake folder layout of a built Verilator and an installed Verilator is the same.

@wilson Snyder

-faligned-new -fbracket-depth=4096 -Qunused-arguments -Wno-bool-operation -Wno-parentheses-equality -Wno-sign-compare -Wno-uninitialized -Wno-unused-but-set-variable -Wno-unused-parameter -Wno-unused-variable -Wno-shadow)

On my machine, the following flags are valid (by gcc)

-faligned-new -Wno-bool-operation -Wno-sign-compare -Wno-uninitialized -Wno-unused-but-set-variable -Wno-unused-parameter -Wno-unused-variable -Wno-shadow

By using the first list, cmake users will detect all flags.
Let's assume a linux distribution builds Verilator with gcc, then a linux user that wants to compile his Verilated soucces with clang. CMake will not check for flags that the gcc compiler does not understand. The same applies to the link flags.

  • I don't understand completely what you want changed in V3EmitCMake.cpp but I've made it look similar to V3EmitMk.cpp.
  • While I was editing V3EmitCMake.cpp, I modified V3EmitMk.cpp a bit: it does not need to subclass a ast node visitor.
  • I've kept the template because it is used with V3StringList and V3StringSet. For example, V3Options::cFlags returns a V3StringList and V3Options::cppFiles returns a V3StringSet.
    Or is there a reason to not use templates?
  • It should now work with cmake 3.8

Greetings

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 26, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-26T16:59:02Z


I pushed the contributors patch and typos patch - threw some other typo fixes in after searching for the same typo. Thanks for those.

I think I understand now what you're saying about configure. My objection now is you are passing flags into cmake that were tested for reasons other than their needing to apply to verilated code, e.g. -Wno-null-conversion should not be used by cmake, and also you are using a low-level function that isn't intended to have side effects. I still think you should either use CFG_CXXFLAGS_NO_UNUSED, or have a new set of _MY_CXX_CHECK_OPT calls setting a new variable e.g. CFG_CXX_FLAGS_CMAKE even if that duplicates some checks.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 26, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-26T18:14:12Z


I've rewritten configure.ac to use a foreach loop.
I'll look into adding systemc support for the cmake script.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 29, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-29T15:58:52Z


I've added a systemc example. I've tested this using Accellera's systemc.

The test will only execute if systemc has been compiled with cmake support and if it has the same c++ standard as the verilated project.

Also, because cmake should be the one in charge for finding the systemc include and library paths, Verilator does not need to output the systemc include and library paths in the generated cmake script.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 30, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-30T00:57:51Z


Thanks, getting close.

I still can't "make examples" out of the box.

make examples
...
-- CMake ----------------
cd build && cmake ..
/bin/sh: 1: cd: can't cd to build



-        of.puts("# DESCR" "IPTION: Verilator output: Makefile for building Verilated archive or executable\n");
+        of.puts("# DESCRIPTION: Verilator output: Makefile for building Verilated archive or executable\n");

Please revert to leave the split as-was, because I have scripts that look for DESCRIPTION and they shouldn't match this line.

+++ b/Makefile.in
-install: all_nomsg install-all
+install: install-all

Why should make install not also build?

+++ b/src/Verilator.cpp
-    V3Options::getenvSYSTEMC_INCLUDE();
-    V3Options::getenvSYSTEMC_LIBDIR();

I believe these are needed to make rpm/apt distros work, why did you remove them?

Thanks for cleaning up configure.ac. Please add a comment

   _MY_CXX_CHECK_OPT(CFG_CXXFLAGS_NO_UNUSED,cflag)
+  # Cmake will test what flags work itself, so pass all flags through to it
   CFG_CXX_FLAGS_CMAKE="$CFG_CXX_FLAGS_CMAKE cflag"
@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 30, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-30T01:03:23Z


Also please test that "make test" is clean with VERILATOR_AUTHOR_SITE=1 in your environment before you configure.

     #dist/t_dist_manifest: %Error: Files mismatch with manifest: examples/cmake_c/build/CMakeCache.txt examples/cmake_c/build/CMakeFiles/CMak$
     #dist/t_dist_untracked: %Error: Files untracked in git or .gitignore: examples/cmake_sc/CMakeFiles/CMakeSystem.cmake
@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 30, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-30T13:08:17Z


  • I did remove the dependency of make install on all_nomsg because if some verilator source is changed between the last make all and sudo make install, the sudo make install will create files owned by root in my source/build tree.
    Imho, packagers should execute make all followed by sudo make install (or make install).
    That way the build step is clearly separated from the install step.
    The instructions at https://www.veripool.org/projects/verilator/wiki/Installing also list these steps.

  • I think I addressed your other concerns..

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 30, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-30T22:29:52Z


I agree users should do a "make" before "make install" however I think it's a feature if someone changes code to make sure the build reoccurs on an install. I had Emacs and Bison sources around and they also rebuild on "make install" so I'll leave this.

+++ b/src/Verilator.cpp
-    V3Options::getenvSYSTEMC_INCLUDE();
-    V3Options::getenvSYSTEMC_LIBDIR();

I think you forgot to comment on this - I believe these are needed to make rpm/apt distros work, why did you remove them?

+++ b/test_regress/t/t_flag_make_cmake_sc.pl
+$DEBUG_QUIET = "--debug --debugi 0 --gdbbt --no-dump-tree";
+    verilator_flags2 => [$DEBUG_QUIET, "-sc --trace"],

The t_a_first tests have this because sometimes Verilator crashes on the first test. Your test can remove these flags. (Just use "-sc")

More critically this test doesn't seem to use "--make cmake".

I merged your t_dist_manifest spell fix.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 31, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-07-31T00:36:47Z


I agree to the make/make install. I reverted that change.

+++ b/src/Verilator.cpp
-    V3Options::getenvSYSTEMC_INCLUDE();
-    V3Options::getenvSYSTEMC_LIBDIR();

V3Options::getenvSYSTEMC_INCLUDE and V3Options::getenvSYSTEMC_LIBDIR return the include and library directory of SystemC. These functions fail if those paths are unknown and a SystemC Verilated output is requested. I believe that's why they were put in src/Verilator.cpp in the first place: to ensure that the include and library paths are known in order to fail fast.

When using CMake, this include and library path is not needed because these paths are detected by CMake. When SystemC is configured, and installed using its CMake support, we are able to use its CMake scripts to link to SystemC (the same as users of Verilator would be able to use Verilator's CMake scripts).

The SystemC example shows how this works: it contains a find_package(SystemCLanguage) to look for the SystemC cmake scripts and at the end, it links to the SystemC target using target_link_libraries(example PUBLIC SystemC::systemc).

(Targets in CMake encapsulate information such as include directories, libraries, compile definitions, c++ standard, ...)

These 2 functions are/were currently needed for emitting the Makefile.

IMHO, when using CMake, Verilator should not try to do too much and let the user handle the SystemC paths.

compile(
     verilator_make_gmake => 0,
     verilator_make_cmake => 1,
     verilator_flags2 => ["-sc"],
     );

The t_flag_make_cmake_sc.pl test does test cmake because it sets verilator_make_cmake to 1. At least it does on my computer?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 31, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-31T10:58:19Z


Yes, the getenv stuff was added to reduce some questions I was getting about why SystemC would later break. I'll remove them in a separate patch so it's easier to bisect. By your philosophy I believe getenvSYSTEMC and getenvSYSTEMC_ARCH should also be removed? They didn't print errors so presumably that's why you left them.

CMake Error: Problem processing arguments. Aborting.
%Skip: CMake could not find SystemC. Please compile and install SystemC with CMake support.
% Make sure that CMAKE_CXX_STANDARD of the SystemC libray and this example match.

Can you please have this print a reference to the Verilator documentation, and in the CMake section you added please add pointer to where to find instructions on how to get SystemC with CMake installed? Likewise if the user uses "verilator -sc" themselves then runs cmake without SystemC installed. Lots of people will hit this, so we need to make this quite clear. Thanks

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 2, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-08-02T14:22:18Z


By your philosophy I believe getenvSYSTEMC and getenvSYSTEMC_ARCH should also be removed? They didn't print errors so presumably that's why you left them.

By "my philosophy", yes. When Verilator is only generating sources, it should not know about the location of a SystemC library. When specifying --exe, then it's another story. But I don't know whether this is used a lot and whether this is Verilator's job. These variables are only needed at build time, so I guess the makefile should fail (as does CMake when it cannot find SystemC).

I've updated the example and the documentation.

Currently, when the user Verilates a design, compiles and links, the compiler will complain about missing <systemc.h> and a linking error.

That's why I've added a convenience function verilator_link_systemc to automatically find and link to the SystemC library. I intentionally do not use the Verilator built-in SystemC variables because I believe Verilator is not a build system.

I hope that suffices to avoid an influx of build problems..

I'm working on the Python support now to be sure that the CMake scripts work as designed.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 3, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-03T02:10:25Z


Thought we were done... but not quite. Can you please change to use these variables:

SYSTEMC_INCLUDE=/example/path/to/systemc-2.3.3/usr/include
SYSTEMC_LIBDIR=/example/path/to/systemc-2.3.3/usr/lib/ubunto14.04

There's two problems I'm having with SYSTEMC_ROOT, first we didn't require it previously so it isn't backward compatible, and second in some systems such as one I use you can't assume it's just SYSTEMC_ROOT/include and SYSTEMC_ROOT/lib. FWIW I agree SYSTEMC_ROOT is a better name.

I'd suggest one of three options:

  1. Use only SYSTEMC_INCLUDE/SYSTEMC_LIBDIR as with current makefiles.

  2. Use SYSTEMC_INCLUDE if defined. Else use SYSTEMC/include (not SYSTEMC_ROOT as Verilator currently uses SYSTEMC where you use SYSTEMC_ROOT). Similar for SYSTEMC_LIBDIR.

  3. Use SYSTEMC_INCLUDE if defined. Else if SYSTEMC_ROOT is defined use SYSTEMC_ROOT/include. Else if SYSTEMC if defined use SYSTEMC/include. Similar for SYSTEMC_LIBDIR. Please also change other usages of SYSTEMC variable to allow SYSTEMC_ROOT. Update bin/verilator to mention SYSTEMC_ROOT.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 4, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-08-04T22:37:35Z


Hello,

I've gone for option 3:

First look in SYSTEMC_INCLUDE/SYSTEMC_LIBDIR, then try SYSTEMC_ROOT, then try SYSTEMC and then try using a CMake target provided by a SystemC installation.

This order is only relevant when a user uses the verilator_link_systemc convenience function. So I've added this documentation only to the CMake section.

It's not finished yet, but I've pushed pushed python support to the python branch in my fork.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 5, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-05T00:50:51Z


The sc example now works, thanks.

  1. The attached has misc whitespace and other fixups you can please check over and apply.

  2. The test_regress/t/t_flag_make_cmake_sc.pl is failing for me.

CMake Warning at CMakeLists.txt:122 (find_package):
By not providing "FindSystemCLanguage.cmake" in CMAKE_MODULE_PATH this
project has asked CMake to find a package configuration file provided by
"SystemCLanguage", but CMake did not find one.

With those fixed I will merge into trunk.

This bug has gotten quite long and with the cmake merge I'd like to be able to close it, so can you please file a new one for python support? Thanks.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 5, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-08-05T18:37:49Z


I have applied your patch and think I have fixed the problem with t_flag_make_cmake_sc.pl

I would like to delay merging this patch to master until after we have stabilized Python support using CMake because I'm not 100% sure all assumptions are valid.

For example, the current CMake script did unconditionally set VL_PRINTF=printf.
I removed this definition because Python users may want to override this.

Soon, I'll open a new bug report for the Python support (current work is in python branch).

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Aug 5, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-05T21:15:09Z


Ok, I'll hold off merging. If you could please merge from master into cmake, then merge cmake into your python branch. I can then in the future do a cmake..python diff to see your python changes.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 4, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-04T19:40:11Z


I've added a rebased version as a pull request on your new(?) github repo. I'm hoping we can get the CMake in at least, let me know if there's any issues.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 4, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-04T23:20:01Z


Note to self - this is: #2

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-10-10T13:40:39Z


Wilson Snyder wrote:

  1. Perhaps we can just use the existing SYSTEMC_CXX_FLAGS? I was going to compare the gmake vs cmake options passed to make by test_regrees, they should be similar, that this fails means something isn't similar.

As a test, I just compiled systemc in c++17 mode and now the makefiles do not work.

Users of cmake should be able to easily configure the c++ standard. Preferably using no SYSTEMC_CXX_FLAGS because -std=c++14 or -std=gnu++14 or ... are compiler dependent.

I'm in favour of modifying the cmake script of the regression test to detect the systemc c++ standard from the SYSTEMC_CXX_FLAGS (environment) variable and set the SYSTEMC_CPPSTD cmake variable accordingly.

  1. Try changing the default in driver.pl to cmake (temporarily) and run a full regression. There will be some breakages not worth fixing, but most should pass, your judgement. If there's generic fixes needed (not related to cmake) please separate them into a different patch I can pre-apply.

I have kicked of a job on travis ci (https://travis-ci.com/madebr/verilator/builds/131320730).

Let's see what that gives.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-10T14:07:06Z


Principally it looks like the tests where it compares the STDOUT to a golden file fail. I propose to just add a cmake_expect and cmake_expect_filename similar to what's done for other simulators, so that part of the test will effectively be skipped.

Alternatively for systemc, rather than trying to parse out a compiler dependent C++ standard flag we could just add $ENV{SYSTEMC_CXX_FLAGS} to the compiler flags and avoid setting any C++ standard in CMake if we're in systemc mode. Then people have the option to set a C++ standard of their choice using SYSTEMC_CXX_FLAGS or manually in their own CMakeLists.txt.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-10-10T14:51:46Z


Patrick Stewart wrote:

Principally it looks like the tests where it compares the STDOUT to a golden file fail. I propose to just add a cmake_expect and cmake_expect_filename similar to what's done for other simulators, so that part of the test will effectively be skipped.
Alternatively, rather than trying to parse out a compiler dependent C++ standard flag we could just add $ENV{SYSTEMC_CXX_FLAGS} to the compiler flags and avoid setting any C++ standard in CMake if we're in systemc mode. Then people have the option to set a C++ standard of their choice using SYSTEMC_CXX_FLAGS or manually in their own CMakeLists.txt.

I have implemented the latter.

If we use the SystemC library pointed by environment or cmake variables, we use the SYSTEMC_CXX_FLAGS cmake or environment variable.

So the SYSTEMC_CXX_FLAGS environment variable on the test system must contain "-std=c++14"

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-10T14:59:11Z


Principally it looks like the tests where it compares the STDOUT to a golden file fail. I propose to just add a cmake_expect and cmake_expect_filename similar to what's done for other simulators, so that part of the test will effectively be skipped.

I'd prefer not to have expecations be different, instead tests should be different - I'm not intending that you commit running everything under cmake, so this is not needed. If there are bugs you find in this process, consider extending the cmake test_regress or adding a new test_regress instead, then the normal "expect" will suit the job.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-10-10T15:41:29Z


Passing SYSTEMC_CXX_FLAGS will still fail the vltmt test because the C++11 flag of multithreaded will overrule the std=c++XX of SYSTEMC_CXX_FLAGS (because it is the latest compiler argument).

I am still in favour of https://www.veripool.org/issues/1363-Verilator-CMake-support#note-45 because that feels more like CMake.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-10T15:52:00Z


I suggest we should not set any language flags, I think any other policy will just lead to trouble with libraries. If the user gives SYSTEMC_CXX_FLAGS then those would be added as usual.

Note the current "gmake" tests do NOT use CFG_CXXFLAGS_STD_NEWEST nor CFG_CXXFLAGS_STD_OLDEST except in a special test.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-10T15:55:32Z


I was thinking we'd change the condition around setting C++11 to if(THREADING && !SYSTEMC) rather than if(THREADING). The cases are:

  1. systemC library that matches compiler default C++ version - just works.

  2. compiler that defaults to a different version - you need to set the right SYSTEMC_CXX_FLAGS.

In either case threading works if systemC library is >= C++11, otherwise it can't work whatever we do.

EDIT: Didn't see Wilson's post, I'm happy to go with that option

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-10T16:23:15Z


I think we should assume C++11, so not muck with flags. At present Verilator complains on any non-C11 system and says to contact us, and also says threading won't work. I know of only one site in that camp, and expect to cut them off next year (e.g. always require C++11 even single threaded).

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Maarten De Braekeleer
Original Date: 2019-10-10T16:50:10Z


Ok, cmake support will require the user to set the correct C++ standard and pass the correct SYSTEMC_CXX_FLAGS. It will also ignore the C++ standard, set by the SystemCLanguage's find_package.

This will not work on older compilers that have a default c++ version less then 11. This fails on travis-ci. (I'm still refering to #45)

Patrick, can you check my patstew_cmake branch? (There is a small fix in driver.pl)

Wilson Snyder wrote:

Note the current "gmake" tests do NOT use CFG_CXXFLAGS_STD_NEWEST nor CFG_CXXFLAGS_STD_OLDEST except in a special test.

This is not completely correct. The verilator tests will append CFG_CXXFLAGS_STD_NEWEST when threaded (in verilator.mk)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-10T23:35:58Z


Ok, I've made a few more tweaks. Most of the tests work now with (despite) this patch:

--- a/test_regress/driver.pl
+++ b/test_regress/driver.pl
@@ -849,6 +849,11 @@ sub compile {
      return 1 if $self->errors || $self->skips || $self->unsupporteds;
      $self->oprint("Compile\n") if $self->{verbose};

+    if ($param{verilator_make_gmake}) {
+        $param{verilator_make_gmake} = 0;
+        $param{verilator_make_cmake} = 1;
+    }
+
      compile_vlt_cmd(%param);

      if (!$self->{make_top_shell}) {
@@ -1019,8 +1024,8 @@ sub compile {
              $self->_run(logfile => "$self->{obj_dir}/vlt_cmake.log",
                          fails => $param{fails},
                          tee => $param{tee},
-                        expect => $param{expect},
-                        expect_filename => $param{expect_filename},
+                        # expect => $param{expect},
+                        # expect_filename => $param{expect_filename},
                          cmd => ["cd \"".$self->{obj_dir}."\" && cmake",
                                  "\"".$self->{t_dir}."/..\"",
                                  "-DTEST_VERILATOR_ROOT=$ENV{VERILATOR_ROOT}",



The ones that don't are either looking at the wrong log or using make_flags which cmake ignores, so I think they can be ignored. There's a commit at the end of the cmake pull which is just some minor tweaks to the test suite that fixes tests on cmake and shouldn't hurt the real tests.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 10, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-10T23:53:01Z


Great. Thanks for separating, I pushed the test suite change part. Looking at rest...

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 11, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-11T01:01:26Z


Maarten is right, without setting the C++ version threading fails on the Travis compiler. I've gone back to if(THREADING && !SYSTEMC) then C++11.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-16T01:55:28Z


Do you think this is ready to merge to master?

Took PATSTEW/cmake, merged master (which shouldn't affect this though) and got this:

v4make test_regress/t/t_a4_examples.pl --dist

-- Performing Test _latomic - Success
You have called ADD_LIBRARY for library verilated_secret_obj without any source files. This typically indicates a problem with your CMakeLis$
-- Executing Verilator...
CMake Error at /verilator/verilator-config.cmake:296 (target_link_libraries):
  Object library target "verilated_secret_obj" may not link to anything.
Call Stack (most recent call first):
  CMakeLists.txt:49 (verilate)


-- Configuring incomplete, errors occurred!
See also "verilator/examples/cmake_protect_lib/build/CMakeFiles/CMakeOutput.log".
See also "verilator/examples/cmake_protect_lib/build/CMakeFiles/CMakeError.log".
Makefile:49: recipe for target 'run' failed
make[1]: *** [run] Error 1
make[1]: Leaving directory 'verilator/examples/cmake_protect_lib'
Makefile:257: recipe for target 'examples' failed
make: *** [examples] Error 10
Use of uninitialized value $param{"logfile"} in concatenation (.) or string at ./driver.pl line 1444.
%Warning: dist/t_a4_examples: Exec of cd .. && make examples failed:

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-16T12:58:51Z


Yes, I think so.

I thought I'd changed that to be compatible with earlier versions of cmake, but the bit that builds both shared and static libraries still needs 3.12 as it turns out, so I've reverted the version check in the Makefile. If you want to test it with CMake <3.12 you can follow the comments in examples/cmake_protect_lib/CMakeLists.txt to only build the static library. Or replace it with:

cmake_minimum_required(VERSION 3.8)
project(cmake_protect_lib)

find_package(verilator HINTS $ENV{VERILATOR_ROOT} ${VERILATOR_ROOT})
if (NOT verilator_FOUND)
  message(FATAL_ERROR "Verilator was not found. Either install it, or set the VERILATOR_ROOT environment variable")
endif()

1. Create a static secret library
add_library(verilated_secret STATIC)

1. Create a new executable target that will contain all your sources
add_executable(example ../make_protect_lib/sim_main.cpp)
target_link_libraries(example PRIVATE verilated_secret)

1. Setup random seed
verilator_generate_key(KEY_INIT)
set(PROTECT_KEY ${KEY_INIT} CACHE STRING "Random seed for protection")
1. Add the Verilated modules to the targets
verilate(verilated_secret
  VERILATOR_ARGS --protect-lib verilated_secret
                  --protect-key ${PROTECT_KEY}
  DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/verilated_secret
  SOURCES ../make_protect_lib/secret_impl.v)

1. Include location of verilated_secret.sv wrapper
verilate(example
  VERILATOR_ARGS "-I${CMAKE_CURRENT_BINARY_DIR}/verilated_secret"
  SOURCES ../make_protect_lib/top.v)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-16T13:03:14Z


I am using Ubuntu 18.04 LTS which is the most recent LTS of Ubuntu. This has CMake 3.10.2 out-of-the-box.

I don't see how we can realistically expect people to have a newer version than this. (People won't want to/know how to upgrade their CMake to something beyond what is in the latest standard distro.)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-16T13:11:35Z


Well it's only the protect_lib example. How about I flip the default of that file so it only builds a static library but leave a comment about how to build both? Or maybe building both simultaneously isn't so important when it's so easy to change and I should just simplify it to static only?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-10-16T13:28:34Z


Sorry, I'm not completely up to date on this thread. But regarding static/shared libraries for --protect-lib, this is meaningful as some non-Verilator simulators require the shared library. It's not critical for the example project as that is Verilator-only. But it is needed for t_prot_lib.pl (as I test against other simulators) and for using the feature in general.

Am I understanding correctly that there is something about building shared libraries that is not supported until CMake 3.12? Or is it an issue with building both the static and shared versions of the library at the same time?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-16T13:41:21Z


It's just the specific way I've used to build both simultaneously here that needs >=3.12. Building one or the other is just s/STATIC/SHARED/. You can also build both by simply copying the same thing out twice on older versions of CMake.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-16T14:08:55Z


I've pushed another version that defaults to just building a static lib, that should work with older versions of CMake. You'll probably still get the "You have called ADD_LIBRARY for library verilated_secret_obj without any source files." warning, but it's spurious and shouldn't be fatal.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 16, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-16T18:26:12Z


I've added two trivial documentation changes after discussion with Maarten on github.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 17, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-17T02:56:21Z


  1. Please make the .pl files you are adding executable.

    $ v4make test_regress/t/t_flag_make_cmake.pl --vlt
    /svaha/wsnyder/bin/v4make: line 8: test_regress/t/t_flag_make_cmake.pl: Permission denied

  2. Then, running that test I get: (I really ran test_regress and this and some others failed with this message)

%Warning: vlt/t_flag_make_cmake: obj_vlt/t_flag_make_cmake/CMakeCache.txt does not exist
vlt/t_flag_make_cmake: %Error: obj_vlt/t_flag_make_cmake/CMakeCache.txt does not exist
vlt/t_flag_make_cmake: FAILED: obj_vlt/t_flag_make_cmake/CMakeCache.txt does not exist

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 17, 2019


Original Redmine Comment
Author Name: Patrick Stewart
Original Date: 2019-10-17T09:57:13Z


As in you ran "make test_regress" in the main directory and got that error? Any more context for it? Did CMake print any errors? I've run:

git checkout cmake
git clean -xdf
autoconf
./configure --enable-longtests
make
make test

and all tests pass for me except @t_prot_lib_inout_bad@ where the log doesn't match (the @v3fatalSrc@ on line 358/359 of @V3ProtectLib.cpp@ is split across both lines and GCC 9.1 gives @LINE@ as 358 whereas the golden file has 359). I've also had some random failures that I think are caused by @t_prot_lib@ running @t_prot_lib_secret@ while @t_prot_lib_secret@ is already running on its own.
I've fixed the permission you mentioned and a trailing whitespace issue.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Oct 17, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-17T23:54:25Z


Thanks for all your work on this.

This is pushed to git towards eventual 4.022 release.

I'm sure you'll have tweaks, please file a new issue/pull request for them.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 10, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-10T19:28:42Z


In 4.022.

@BracketMaster

This comment has been minimized.

Copy link

@BracketMaster BracketMaster commented Dec 29, 2019

It doesn't seem like the Python features got merged into master?
This would be extremely useful. I have been working on the Python Migen and nMigen HDLs for some time now rolling my own PyVerilator solution.
Is it possible to still get this merged?

This was referenced Dec 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.