Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  BugVise
  CovVise
  Force-Gate-Sim
  Gspice
  IPC::Locker
  Rsvn
  SVN::S4
  Voneline
  WFH
General Info
  Papers

Issue #1363

CMake support

Added by Patrick Stewart 11 months ago. Updated about 2 months ago.

Status:
AskedReporter
Priority:
Low
Category:
Unsupported
% Done:

0%


Description

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.

merge.patch View (12.9 KB) Wilson Snyder, 08/05/2019 12:44 AM

History

#1 Updated by Wilson Snyder 11 months ago

  • Category set to Unsupported
  • Status changed from New to AskedReporter
  • Assignee set to Patrick Stewart

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.

#2 Updated by Patrick Stewart 11 months ago

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.

#3 Updated by Pieter Kapsenberg 5 months ago

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.

#4 Updated by Maarten De Braekeleer about 2 months ago

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 https://github.com/madebr/verilator/pull/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

#5 Updated by Wilson Snyder about 2 months ago

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 <make-system>"?

+=item --make &lt;build-system&gt;
+
+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&lt;typename List&gt;
+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&lt;string&gt; V3FileDependImp::getAllDeps() {
+    std::vector&lt;string&gt; r;
+    for (std::set&lt;DependFile&gt;::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: '"<&lt;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.

#6 Updated by Patrick Stewart about 2 months ago

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)
at the top of the verilator-config-version.cmake, the extra 0) stops cmake loading the module.

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

#7 Updated by Maarten De Braekeleer about 2 months ago

Hello Patrick,

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

#8 Updated by Maarten De Braekeleer about 2 months ago

Hello,

I think I have incorporated the comments in #5.

They can still be found at https://github.com/madebr/verilator/pull/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

#9 Updated by Maarten De Braekeleer about 2 months ago

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 https://github.com/madebr/verilator/pull/2

#10 Updated by Wilson Snyder about 2 months ago

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 &lt;make-system-system&gt;       Generate scripts for specified make system

s/-system-system/-system/

+=item --make &lt;make-system&gt;

Use I before <> so pretty-prints nicely.

+    {prefix}.cmake

Please add trailing comment like other lines:

{prefix}.cmake                      // CMake file for compiling
+template&lt;typename List&gt;
+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.

#11 Updated by Patrick Stewart about 2 months ago

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

#12 Updated by Wilson Snyder about 2 months ago

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.

#13 Updated by Maarten De Braekeleer about 2 months ago

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
  • I push my patches to https://github.com/madebr/verilator/tree/cmake (the cmake branch)
  • `configure.ac`: the configure script will test some compiler flags and libraries. Right now it detects the following flags:
-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

#14 Updated by Wilson Snyder about 2 months ago

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.

#15 Updated by Maarten De Braekeleer about 2 months ago

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

#16 Updated by Maarten De Braekeleer about 2 months ago

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.

#17 Updated by Wilson Snyder about 2 months ago

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"

#18 Updated by Wilson Snyder about 2 months ago

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

#19 Updated by Maarten De Braekeleer about 2 months ago

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

#20 Updated by Wilson Snyder about 2 months ago

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.

#21 Updated by Maarten De Braekeleer about 2 months ago

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?

#22 Updated by Wilson Snyder about 2 months ago

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

#23 Updated by Maarten De Braekeleer about 2 months ago

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.

#24 Updated by Wilson Snyder about 2 months ago

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.

#25 Updated by Maarten De Braekeleer about 2 months ago

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.

#26 Updated by Wilson Snyder about 2 months ago

  • File merge.patch View added
  • Subject changed from CMake and Python support to CMake support

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.

#27 Updated by Maarten De Braekeleer about 2 months ago

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

#28 Updated by Wilson Snyder about 2 months ago

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.

Also available in: Atom