Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  IPC::Locker
  Parallel::Forker
  Voneline
General Info
  Papers

Issue #1363

CMake support

Added by Patrick Stewart about 1 year ago. Updated 9 days ago.

Status:
Closed
Priority:
Normal
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

cmake.patch View (5.97 KB) Wilson Snyder, 10/08/2019 11:42 PM

History

#1 Updated by Wilson Snyder about 1 year 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 about 1 year 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 7 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 4 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 4 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 4 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 4 months ago

Hello Patrick,

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

#8 Updated by Maarten De Braekeleer 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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.

#29 Updated by Patrick Stewart about 2 months ago

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.

#30 Updated by Wilson Snyder about 2 months ago

Note to self - this is: https://github.com/verilator/verilator/pull/2

#31 Updated by Wilson Snyder about 2 months ago

Ok, thanks for rebasing. Looked through this again and a few things:

examples/cmake    -> rename to examples/cmake_c
examples/cmake_c  -> rename to examples/cmake_tracing_c
examples/cmake_sc -> rename to examples/cmake_tracing_sc
README.pod:
examples/cmake_c            => Example building hello_world_c with CMake
examples/cmake_sc           => Example building hello_world_sc with CMake

Please rename so similar to other examples & update readme.pod as was wrong (with old names) & missing one.

Finally, I'm not that up on CMake. I'm hoping as cmake-related issues arise you'll be willing to support it.

#32 Updated by Wilson Snyder about 1 month ago

  • Status changed from AskedReporter to Assigned

I renamed the old examples to have "make" in the name so users can tell make vs cmake. Can you please rename yours to match? Note also the example names are a few places in Makefile.in; feel free to clean that up.

+ examples/cmake_hello_c      => Example CMake simple Verilog->C++ conversion
+ examples/cmake_tracing_c    => Example CMake Verilog->C++ with tracing
+ examples/cmake_tracing_sc   => Example CMake Verilog->SystemC with tracing
  examples/make_hello_c       => Example GNU-make simple Verilog->C++ conversion
  examples/make_hello_sc      => Example GNU-make simple Verilog->SystemC conversion
  examples/make_tracing_c     => Example GNU-make Verilog->C++ with tracing
  examples/make_tracing_sc    => Example GNU-make Verilog->SystemC with tracing

#33 Updated by Patrick Stewart about 1 month ago

I've renamed the examples in the PR.

Finally, I'm not that up on CMake. I'm hoping as cmake-related issues arise you'll be willing to support it.

I do intend to keep cmake+python working in the future as I have a reasonable amount of test code that depends on it, so I'm willing to work on bugs that crop up or helping out if changes elsewhere in verilator require changes to cmake/python. I can't really deal with macos issues because I don't have access to one, and I won't necessarily have time to add features (as opposed to bugs).

#34 Updated by Wilson Snyder about 1 month ago

Few more things:

Suggest we standardize on two spaces in cmake files per indent level as that seems what others recommend and is what Emacs wants. I can do this if you prefer as few keystrokes in Emacs.

Can you split cmake lines longer than 95 or so characters please? I don't care about lines with long "....strings....") but rather ones with other content that is important not to get lost on right of screen. Found this hint: https://stackoverflow.com/questions/7637539/how-to-split-strings-across-multiple-lines-in-cmake

list(FIND TEST_VERILATOR_ARGS "-sc" _INDEX)
if(NOT _INDEX EQUAL -1)
  set(TEST_SYSTEMC 1)
  list(REMOVE_AT TEST_VERILATOR_ARGS ${_INDEX})
endif()
list(FIND TEST_VERILATOR_ARGS "--sc" _INDEX)
if(NOT _INDEX EQUAL -1)
  set(TEST_SYSTEMC 1)
  list(REMOVE_AT TEST_VERILATOR_ARGS ${_INDEX})
endif()

Sometimes you checked for --flag and flag and sometimes only one. Probably this code should work either way with all flags? I'd suspect there's a way to use regexp match, but perhaps a better thing is to make a function that removes an arbitrary flag (with - or -) and sets the given variable?

There's duplicate calls to notify() as this was added in master earlier.

I'd be inclined to remove using File::Which as it removes a dependency and I think `which cmake` will be fine given we don't support testing in non-Windows. (Every dependency causes a little friction for some new users, and slower CI runs.) I think which might not be needed at all, as the following test of --version returning something can see the regexp doesn't match and return "";

In driver.pl please use a space on both sides of =>. (Generally you did well to copy the style, but this was a style improvement for newer code as of a year or so ago.)

Merged your rename of verilator_make_gmake as seemed a good step to reduce the patch size.

#35 Updated by Patrick Stewart about 1 month ago

Suggest we standardize on two spaces in cmake files per indent level as that seems what others recommend and is what Emacs wants. I can do this if you prefer as few keystrokes in Emacs.

Done

Can you split cmake lines longer than 95 or so characters please? I don't care about lines with long "....strings....") but rather ones with other content that is important not to get lost on right of screen.

Done, I left the long message("...") calls as is, because the string line breaks in cmake need to start with no indentation which I find worse that a long line. All the functional lines should be <95 chars now.

Sometimes you checked for --flag and flag and sometimes only one. Probably this code should work either way with all flags? I'd suspect there's a way to use regexp match, but perhaps a better thing is to make a function that removes an arbitrary flag (with - or -) and sets the given variable?

Done

There's duplicate calls to notify() as this was added in master earlier.

Oops, the fix for the got into the python patchset.

I'd be inclined to remove using File::Which as it removes a dependency and I think `which cmake` will be fine given we don't support testing in non-Windows. (Every dependency causes a little friction for some new users, and slower CI runs.) I think which might not be needed at all, as the following test of --version returning something can see the regexp doesn't match and return "";

Done

In driver.pl please use a space on both sides of =>. (Generally you did well to copy the style, but this was a style improvement for newer code as of a year or so ago.)

Done

#36 Updated by Wilson Snyder about 1 month ago

Tried your branch quickly.

0. Got

CMake Error at verilator/verilator-config.cmake:311:
  Parse error.  Expected a command name, got unquoted argument with text
  "$<$&lt;BOOL:$&lt;TARGET_PROPERTY:VERILATOR_PYTHON&gt;>:VL_PYTHON>".

I ripped out all lines related to python and made progress.

1. I tried "make examples" and got errors. The first problem was SystemC which I fixed:

diff --git a/examples/cmake_hello_sc/CMakeLists.txt b/examples/cmake_hello_sc/CMakeLists.txt
@ -39,7 +40,6 @ add_executable(example ${CMAKE_CURRENT_SOURCE_DIR}/../make_hello_sc/sc_main.cpp)
 # Add the Verilated circuit to the target
 verilate(example SYSTEMC
   INCLUDE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/../make_hello_sc" 
-  VERILATOR_ARGS -f ${CMAKE_CURRENT_SOURCE_DIR}/../make_hello_sc/input.vc -O2 -x-assign 0
   SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/../make_hello_sc/top.v)
verilator_link_systemc(example)

2. That test passes but coverage in examples/cmake_tracing_c is broken, please fix (probably sc also):

.../verilator/bin/verilator_coverage --annotate logs/annotated logs/coverage.dat
%Error: Can't read logs/coverage.dat

#37 Updated by Patrick Stewart about 1 month ago

Apologies I mucked up keeping the changes separate after changing the indentation. I'll check in new version soon, I missed that systemc issue.

#38 Updated by Patrick Stewart about 1 month ago

Ok, I think it's correct now. I've also changed Maarten's email address as he requested.

#39 Updated by Wilson Snyder about 1 month ago

Sorry I'm pestering you with these small batches; I'm learning as I use it.

Got some warnings:

test_regress/t/t_flag_make_cmake.pl
Use of uninitialized value in concatenation (.) or string at ./driver.pl line 1028.
Use of uninitialized value in concatenation (.) or string at ./driver.pl line 1028.

I cleaned these and other minor stuff up, see attached patch. Please check this & commit.

-- Performing Test _latomic
-- Performing Test _latomic - Success
-- Configuring done

Configuring takes several seconds, maybe these aren't properly getting cached? Is there some way to also cache all tests (e.g. use cmake -C <somewhere>)?

verilator-config.cmake

This seems to be the file for verilated output, not for verilator itself. Will this conflict if/when we change verilator itself (i.e. the program) to use cmake? I note the rule you define is called "verilate" which seems perfect. Should this file then be renamed verilate-config.cmake? Probably obviously we have three stages:

verilator  -- cmake the program (some day)
verilate   -- run verilator on some code
verilated  -- final output (e.g. gmake verilated files ala include/verilated.mk)

verilator-config-version.cmake is perhaps correctly named as both verilator itself and the verilate cmake all are the same version?

Does this mean the find_package need verilate versus verilator?

verilate(example SYSTEMC COVERAGE TRACE
  INCLUDE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc" 
  VERILATOR_ARGS -f ${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc/input.vc -O2 -x-assign 0
  SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc/top.v)
verilator_link_systemc(example)

Can you show me a rough example CMakeLists.txt that verilates two designs into two libraries, then make an executable file containing both?

INCLUDE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc" 
VERILATOR_ARGS -f ${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc/input.vc -O2 -x-assign 0
SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc/top.v)

Are directories relative to the cmake file? Can the ${CMAKE_CURRENT_SOURCE_DIR}/ prefix be removed? (It works, but maybe I'm missing something.)

define_property(TARGET
  PROPERTY VERILATOR
  BRIEF_DOCS "Verilator" 
  FULL_DOCS "Verilator" 
)

Is this a property that is true if Verilator is used? Or? Please update the FULL_DOCS.

Would it be helpful to have a bin/verilator EXAMPLE C++ WITH CMAKE section that walks through exact steps? Perhaps some material moves or is cross-referenced from the larger cmake section. Ok to wait to do this.

Note there's a potential collision with between the protect (bug1490) and cmake feature (bug1363), with you both almost ready to merge. I propose both merge and we cleanup "--make cmake --protect-lib" in a later patch set. I'll file a bug once both hit master.

#40 Updated by Patrick Stewart about 1 month ago

I cleaned these and other minor stuff up, see attached patch. Please check this & commit.

Thanks, added

-- Performing Test _latomic -- Performing Test _latomic - Success -- Configuring done

Configuring takes several seconds, maybe these aren't properly getting cached? Is there some way to also cache all tests (e.g. use cmake -C <somewhere>)?

This should already just work for examples as long as you don't clean the build directory. Do you mean when running the perl tests? If you want to keep the cache from run to run we can remove
unlink "$self->{obj_dir}/CMakeCache.txt";
from line 1011 of driver.pl. CMake should take care of rerunning when it detects changes of course, but it perhaps increases the chance of stale data helping tests pass. Shall I make that change?

I don't think there's a good way to avoid repeating these system/compiler detection tests across different examples or different perl tests. We could of course just not do so many system/compiler tests in CMake, or only run them in one specific perl test.

verilator-config.cmake

This seems to be the file for verilated output, not for verilator itself. Will this conflict if/when we change verilator itself (i.e. the program) to use cmake? I note the rule you define is called "verilate" which seems perfect. Should this file then be renamed verilate-config.cmake? Probably obviously we have three stages:

verilator -- cmake the program (some day) verilate -- run verilator on some code verilated -- final output (e.g. gmake verilated files ala include/verilated.mk)

verilator-config-version.cmake is perhaps correctly named as both verilator itself and the verilate cmake all are the same version?

Does this mean the find_package need verilate versus verilator?

If you change the main program to compile with CMake one day, you would use a CMakeLists.txt file, you'd never need a verilator-config.cmake for building verilator itself. xxx-config.cmake is to be installed to /usr/share/xxx/xxx-config.cmake to provide features to users via find_package(xxx). You can also add a development directory to CMAKE_PREFIX_PATH and find_package will use that instead of the installed version. As an example, if verilator ever provided some kind of shared library for users, then we could add a library target to verilator-config.cmake and they could pick it up without dealing with paths.

I'd be against changing to verilate-config.cmake as we'd either have to put it in /usr/share/verilate or break find_package(). I think most people would expect to use find_package(verilator).

verilate(example SYSTEMC COVERAGE TRACE INCLUDE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc" VERILATOR_ARGS -f ${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc/input.vc -O2 -x-assign 0 SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/../make_tracing_sc/top.v) verilator_link_systemc(example)

Can you show me a rough example CMakeLists.txt that verilates two designs into two libraries, then make an executable file containing both?

If you just want two modules in one executable it's:
project(cmake_multi)
find_package(verilator)
add_executable(example sim_main.cpp)
verilate(example SOURCES mod1.v)
verilate(example SOURCES mod2.v)
verilate(target ...) simply appends the necessary rules to the given target so you can call it repeatedly. If you want to build libraries for reuse in multiple executables it's only slightly more complicated:
project(cmake_multi_lib)
find_package(verilator)
add_library(mod1 STATIC) # or SHARED for a .so/.dll
add_library(mod2 STATIC)
add_executable(example sim_main.cpp)
target_link_libraries(example PRIVATE mod1 mod2)
verilate(mod1 SOURCES mod1.v)
verilate(mod2 SOURCES mod2.v)

Are directories relative to the cmake file? Can the ${CMAKE_CURRENT_SOURCE_DIR}/ prefix be removed? (It works, but maybe I'm missing something.)

Yes you can, that only becomes an issue when you include() one CMakeLists file from another (which you shouldn't do, you should use add_subdirectory()). I think it was needed at some point in development, but we've set WORKING_DIRECTORY for verilator to ${CMAKE_CURRENT_SOURCE_DIR} so relative paths work correctly now. I'll remove those.

define_property(TARGET PROPERTY VERILATOR BRIEF_DOCS "Verilator" FULL_DOCS "Verilator" )

Is this a property that is true if Verilator is used? Or? Please update the FULL_DOCS.

Yes I think so. It's used to enable the verilator LDFLAGS on this target, but since it's only ever set to ON that could be done unconditionally. I'll remove it and VERILATOR_PREFIXES which doesn't seem to be used.

Would it be helpful to have a bin/verilator EXAMPLE C++ WITH CMAKE section that walks through exact steps? Perhaps some material moves or is cross-referenced from the larger cmake section. Ok to wait to do this.

I've tweaked the documentation under CMake so it shows how to build that example, is that sufficient?

Note there's a potential collision with between the protect (bug1490) and cmake feature (bug1363), with you both almost ready to merge. I propose both merge and we cleanup "--make cmake --protect-lib" in a later patch set. I'll file a bug once both hit master.

Rebased and added example since it's fairly trivial

#41 Updated by Patrick Stewart about 1 month ago

I've added an additional change to use --protect-key in the cmake_protect_lib example, otherwise you can get problems when the filenames change. Using 'ninja' instead of 'make' works OK, and running 'make' again gets rid of the problem when it occurs, but it's annoying for running the tests.

#42 Updated by Wilson Snyder about 1 month ago

  • Priority changed from Low to Normal

Got thinking about --protect-key in that test; I committed a fix to add a new option --generate-key as that seems useful, if you could change to use that instead of RANDOM which doesn't seem to be cryptographically secure.

#43 Updated by Wilson Snyder about 1 month ago

Very close.

1. Please use --generate-key mentioned previously.

2. Please remove the unlink "$self->{obj_dir}/CMakeCache.txt";, that solved my always-configing problem. I'm sure cmake isn't any worse than make itself; very rarely I find I do need to blow away test_regress/obj* after making some huge make restructuring, that's ok, worth it for faster tests in the normal case.

3. If cmake isn't installed the which returns "" not undef, so suggest this in driver.pl:

sub cmake_version {
    chomp(my $cmake_bin = `which cmake`);
    if (!$cmake_bin) {
        return undef;
    }
    my $cmake_version = `cmake --version`;
    if ($cmake_version !~ /cmake version (\d+)\.(\d+)/) {
        return undef;
    }
    $cmake_version = "$1.$2";
    return version->declare($cmake_version);
}
4. Only test failure is the below; note the non-threaded version works ok.

V4$ test_regress/t/t_flag_make_cmake_sc.pl  --vltmt
==SUMMARY: Left 1  Passed 0  Failed 0  Unsup 0  Time 0:00
======================================================================
vltmt/t_flag_make_cmake_sc: ==================================================
        cd "obj_vltmt/t_flag_make_cmake_sc" && cmake "verilator/test_regress/t/.." -DTEST_VERILATOR_ROOT=/x$
-- Configuring done
-- Generating done
-- Build files have been written to: /verilator/test_regress/obj_vltmt/t_flag_make_cmake_sc
        cmake --build obj_vltmt/t_flag_make_cmake_sc   > obj_vltmt/t_flag_make_cmake_sc/vlt_cmake_build.log
Scanning dependencies of target Vt_flag_make_cmake_sc
[ 12%] Building CXX object CMakeFiles/Vt_flag_make_cmake_sc.dir/Vt_flag_make_cmake_sc__main.cpp.o
[ 25%] Linking CXX executable Vt_flag_make_cmake_sc
CMakeFiles/Vt_flag_make_cmake_sc.dir/Vt_flag_make_cmake_sc__main.cpp.o: In function `__static_initialization_and_destruction_0(int, int)':
Vt_flag_make_cmake_sc__main.cpp:(.text+0x314): undefined reference to `sc_core::sc_api_version_2_3_3_cxx201103L<&sc_core::SC_DISABLE_VIRTUAL_BIND_UNDEFINED_>::sc_api_version_2_3_3_cxx201103L(sc_core::sc_writer_policy)'
CMakeFiles/Vt_flag_make_cmake_sc.dir/CMakeFiles/Vt_flag_make_cmake_sc.dir/Vt_flag_make_cmake_sc.dir/Vt_flag_make_cmake_sc.cpp.o: In function `__static_initialization_and_destruction_0(int, int)':
Vt_flag_make_cmake_sc.cpp:(.text+0x8e0): undefined reference to `sc_core::sc_api_version_2_3_3_cxx201103L<&sc_core::SC_DISABLE_VIRTUAL_BIND_UNDEFINED_>::sc_api_version_2_3_3_cxx201103L(sc_core::sc_writer_policy)'
CMakeFiles/Vt_flag_make_cmake_sc.dir/CMakeFiles/Vt_flag_make_cmake_sc.dir/Vt_flag_make_cmake_sc.dir/Vt_flag_make_cmake_sc__Syms.cpp.o: In function `__static_initialization_and_destruction_0(int, int)':
Vt_flag_make_cmake_sc__Syms.cpp:(.text+0xd3): undefined reference to `sc_core::sc_api_version_2_3_3_cxx201103L<&sc_core::SC_DISABLE_VIRTUAL_BIND_UNDEFINED_>::sc_api_version_2_3_3_cxx201103L(sc_core::sc_writer_policy)'
collect2: error: ld returned 1 exit status
CMakeFiles/Vt_flag_make_cmake_sc.dir/build.make:222: recipe for target 'Vt_flag_make_cmake_sc' failed
make[2]: *** [Vt_flag_make_cmake_sc] Error 1
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/Vt_flag_make_cmake_sc.dir/all' failed
make[1]: *** [CMakeFiles/Vt_flag_make_cmake_sc.dir/all] Error 2
Makefile:83: recipe for target 'all' failed
make: *** [all] Error 2
%Warning: vltmt/t_flag_make_cmake_sc: Exec of cmake failed: Scanning dependencies of target Vt_flag_make_cmake_sc

vltmt/t_flag_make_cmake_sc: %Error: Exec of cmake failed: Scanning dependencies of target Vt_flag_make_cmake_sc
vltmt/t_flag_make_cmake_sc: FAILED: Exec of cmake failed: Scanning dependencies of target Vt_flag_make_cmake_sc
==SUMMARY: Passed 0  Failed 1  Unsup 0  Time 0:01

My gcc is 7.4.0 on Ubuntu 18.04 LTS. Thought thi

#44 Updated by Patrick Stewart about 1 month ago

Wilson Snyder wrote:

Very close.

1. Please use --generate-key mentioned previously.

2. Please remove the unlink "$self->{obj_dir}/CMakeCache.txt";, that solved my always-configing problem. I'm sure cmake isn't any worse than make itself; very rarely I find I do need to blow away test_regress/obj* after making some huge make restructuring, that's ok, worth it for faster tests in the normal case.

3. If cmake isn't installed the which returns "" not undef, so suggest this in driver.pl:

I've done these, thanks for the perl

4. Only test failure is the below; note the non-threaded version works ok. [...]

My gcc is 7.4.0 on Ubuntu 18.04 LTS. Thought thi

Looks like your comment got cut off there. Anyhow, this appears to be down to this somewhat bizarre decision to create link errors when mixing C++ versions. https://forums.accellera.org/topic/5738-support-for-c1114-in-systemc-232/ https://forums.accellera.org/topic/6119-build-error-with-232/

I think the issue arises because in threaded mode the CMake script sets C++11 as the minimum version. For whatever reason I'd bet cmake thinks it needs to set --std=c++11 on your version of GCC but not on mine, or presumably Maarten's. Then the C++ version that the code is compiled with does not match the systemc library and it refuses to link. As far as I can tell there's no way to find out what version of the library is installed, so there's no way to deal with it automatically. I've chosen to just force it to C++14 when systemc is enabled, which ought to be equivalent to what the Makefile does (that uses --std=gnu++14 in threading mode) so I assume this will work for you at least. If it doesn't could you try playing with SC_CPLUSPLUS and cxx_std_XX in verilator-config.cmake please?

#45 Updated by Maarten De Braekeleer about 1 month ago

I should admit I did not test systemc with multithreaded verilator.

Hardcoding the c++ standard to 14 is imho a bad choice because when someone compiles systemc in c++17 mode, this will break again. The user must feed us this information (or SystemC's cmake find_package module)

I propose the following solution:

- When we fetch SYSTEMC_INCLUDEDIR and SYSTEMC_LIBRARY from the (environment) variables, get the C++ standard from the SYSTEMC_CPPSTD (environment) variable. - When we use SystemCLanguage's find_package, use the SystemC_CXX_STANDARD CMake variable - Else, when no standard is set, print a warning and leave it alone. The user should handle it.

I have something working at https://github.com/madebr/verilator/tree/patstew_cmake

#46 Updated by Wilson Snyder about 1 month ago

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.

I was debugging before work and here's what I got to:

2. please remove the "-Wall -Wno-error" as a default. This is up to the user and cmake versus gmake should be identical.

3. Apply this to fix --verbose:

diff --git a/test_regress/driver.pl b/test_regress/driver.pl
--- a/test_regress/driver.pl
+++ b/test_regress/driver.pl
@@ -1020,12 +1020,12 @@ sub compile {
                                 "-DTEST_CSOURCES=\"@csources\"",
                                 "-DTEST_VERILATOR_ARGS=\"@vlt_args\"",
                                 "-DTEST_VERILATOR_SOURCES=\"$param{top_filename} @{$param{v_other_filenames}}\"",
-                                "-DTEST_VERBOSE=\"".($opt_verbose ? 1 : 0)."\"",
+                                "-DTEST_VERBOSE=\"".($self->{verbose} ? 1 : 0)."\"",
                                 "-DTEST_SYSTEMC=\"" .($self->sc ? 1 : 0). "\"",
                                 "-DCMAKE_PREFIX_PATH=\"".(($ENV{SYSTEMC_INCLUDE}||$ENV{SYSTEMC}||'')."/..\""),
                                 "-DTEST_OPT_FAST=\"" . ($param{benchmark}?"-O2":"") . "\"",
                                 "-DTEST_VERILATION=\"" . $::Opt_Verilation . "\"",
-                                ($self->{verbose} ? "--verbose" : ""),
+                                ($self->{verbose} ? "-DCMAKE_VERBOSE_MAKEFILE:BOOL=ON" : ""),
                         ]);
             return 1 if $self->errors || $self->skips || $self->unsupporteds;
         }

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

#47 Updated by Maarten De Braekeleer about 1 month ago

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.

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

#48 Updated by Patrick Stewart about 1 month ago

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.

#49 Updated by Maarten De Braekeleer about 1 month ago

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"

#50 Updated by Wilson Snyder about 1 month ago

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.

#51 Updated by Maarten De Braekeleer about 1 month ago

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.

#52 Updated by Wilson Snyder about 1 month ago

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.

#53 Updated by Patrick Stewart about 1 month ago

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

#54 Updated by Wilson Snyder about 1 month ago

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

#55 Updated by Maarten De Braekeleer about 1 month ago

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)

#56 Updated by Patrick Stewart about 1 month ago

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.

#57 Updated by Wilson Snyder about 1 month ago

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

#58 Updated by Patrick Stewart about 1 month ago

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.

#59 Updated by Wilson Snyder about 1 month ago

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:

#60 Updated by Patrick Stewart about 1 month ago

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

# Create a static secret library
add_library(verilated_secret STATIC)

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

# Setup random seed
verilator_generate_key(KEY_INIT)
set(PROTECT_KEY ${KEY_INIT} CACHE STRING "Random seed for protection")
# 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)

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

#61 Updated by Wilson Snyder about 1 month ago

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

#62 Updated by Patrick Stewart about 1 month ago

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?

#63 Updated by Todd Strader about 1 month ago

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?

#64 Updated by Patrick Stewart about 1 month ago

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.

#65 Updated by Patrick Stewart about 1 month ago

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.

#66 Updated by Patrick Stewart about 1 month ago

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

#67 Updated by Wilson Snyder about 1 month ago

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

#68 Updated by Patrick Stewart about 1 month ago

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.

#69 Updated by Wilson Snyder about 1 month ago

  • Status changed from Assigned to Resolved

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.

#70 Updated by Wilson Snyder 9 days ago

  • Status changed from Resolved to Closed

In 4.022.

Also available in: Atom