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

Support FST

Added by Sergi Granell 3 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Unsupported
% Done:

0%


Description

It seems like this dump format supported by GTKWave would allow to include struct member names and enum names to the dump.

verilator_fst.patch View (1.59 KB) Sergi Granell, 09/29/2018 09:49 PM

verilator_infrastructure_fst.patch View (6.48 KB) Sergi Granell, 09/30/2018 07:17 AM

fst.jpg View (127 KB) Sergi Granell, 09/30/2018 08:40 AM

verilator_fst.patch View - Initial proof-of-concept patch (371 KB) Sergi Granell, 09/30/2018 09:50 AM

verilator_fst.patch View (371 KB) Sergi Granell, 09/30/2018 12:18 PM

test_fst.patch View (31.9 KB) Wilson Snyder, 09/30/2018 01:56 PM

0002-FST-Add-test-files.patch View (34.1 KB) Sergi Granell, 09/30/2018 09:25 PM

0001-FST-Initial-support.patch View (372 KB) Sergi Granell, 09/30/2018 09:25 PM

0003-README-Fix-typo-LTX2-LXT2.patch View (815 Bytes) Sergi Granell, 09/30/2018 09:25 PM

0004-FST-Add-lz-addLdLibs.patch View (1.91 KB) Sergi Granell, 09/30/2018 09:25 PM

combined.patch View (406 KB) Sergi Granell, 10/01/2018 07:08 AM

FST.patch View (405 KB) Sergi Granell, 10/02/2018 04:01 PM

History

#1 Updated by Sergi Granell 3 months ago

I'm trying to link with libfst.a from GTKWave but autotools is too complicated for me.

Steps I've tried:
  1. Copy gtkwave3/src/helpers/fst to verilator/include
  2. Add --disable-fst configure option (enabled by default)
  3. In verilator/configure.ac: If CFG_WITH_FST is true, do AC_CONFIG_FILES([include/fst/Makefile])
  4. In src/Makefile_obj.in: If CFG_WITH_FST is true, update CPPFLAGS and LIBS to include fst

Now when running make, it tries to link with libfst.a but this file doesn't exist since I don't know how to tell autotools it's a dependency for verilator.

I've included the patch for more details.

#2 Updated by Wilson Snyder 3 months ago

  • Category set to Unsupported
  • Status changed from New to Feature
  • Assignee set to Sergi Granell

Rather than using the shared library are there .h/.cpp files that can be compiled in just as with FXT2?

If not, the configure stuff isn't used for post-verilation makefiles, instead do a addLibraryFile call inside Verilator when using FST.

Also see the notes on structs just added to Topic 2601

#3 Updated by Wilson Snyder 3 months ago

Also see how the LXT2 stuff was added

git show acf4a3fa998875d832fd8b0660941b3d31f76bcc

#4 Updated by Sergi Granell 3 months ago

Thanks, I followed the LXT2 patch and now I have the basic infrastructure for FST setup. Now it's time to use the fstapi.

#5 Updated by Sergi Granell 3 months ago

Update: I've managed to get a very basic valid FST dump (only dumps bit variables)!

Since the FST API supports native SystemVerilog types, I think it's a good moment to start thinking about improving the "declFoo", "chgFoo", etc callback mechanism into some richer one that provides structs, enums, etc definitions. Maybe adding a switch that changes from the old (current) callback mechanism to a new one that emits complex types would be a way to do it, and then only for FST we should enable the newer cb mechanism.

Screenshot:

#6 Updated by Sergi Granell 3 months ago

Here's the initial FST patch, it's just a Proof of concept for now.

All the include/fst were copied from GTKWave's SVN, I had to modify fstapi.c to use a custom "fst_config.h" instead of <config.h>.

#7 Updated by Wilson Snyder 3 months ago

Excellent! Your patch didn't show the new fst files (I suspect you forgot to "git add" them), can you attach a new patch when at a good point?

I'd suggest we get the FST dumping with old types complete and into trunk, then next step we'll add the new formats. The --trace-fst flag will be sufficient to enable the new API for declarations. For simplicity though I may update the other formats too. verilated.h in VerilatedVarType and VerilatedVarFlags which are currently used for VPI purposes are probably part of what we should pass in.

#8 Updated by Wilson Snyder 3 months ago

Our messages crossed, I see the new files now, seem ok once you obviously extend out the commented code. Please add the appropriate test/*fst files also to match the _lxt2 files.

#9 Updated by Iztok Jeras 3 months ago

I tried to compile pa packed struct example in Icarus Verilog (10.1 included in Ubuntu 18.04), but I only got wires and registers inside the FST file. So I do not think I would be able to use Icarus to generate gold files.

I checked GTKWave sources and was not able to find FST examples except for two simple ones. There is some Java code which seems to be FST related, but I did not read it yet.

I also think that although GTKWave differentiates between this data types, it might not be able to show the structure. Maybe in the SST window, but not in the Signal and Waveform windows. Professional waveform tools show a structure in the Signal window, where the entire vector value is shown, then the value can be expanded to show its elements and types and values for each element. I do not think GTKWave would be able to show both the value of a structure and its elements.

#10 Updated by Wilson Snyder 3 months ago

Tony Bybell, the author of GTKwave has indicated he's receptive to improvements. Once we have support in Verilator we can work with him to make appropriate improvements to both Verilator and GTKWave to implement whatever extra features make for effective debug.

#11 Updated by Sergi Granell 3 months ago

I have ported all the supported types of the LXT2 backend to FST and fixed signal aliases since the latest patch. I haven't tested the float, double, quad and array types and haven't added test files (I'm not sure which is the proper way to write them).

#12 Updated by Wilson Snyder 3 months ago

Please add the following patch to your set and get the test(s) to pass (test_regress/t/t_trace_fst.pl)

The calls in fstapi.cpp need to cast the malloc returns, plus there's probably other stuff. (Once clean, I'll feed this change upstream.)

Your patches look good, so once the tests pass I think it's ready to commit if you think so too.

#13 Updated by Sergi Granell 3 months ago

I've playing around with the fstapi and it's quite useful, for example:
    fstWriterSetScope(m_fst, FST_ST_VCD_STRUCT, "outer_struct", NULL);
    fstHandle handle1 = fstWriterCreateVar(m_fst, FST_VT_SV_BIT, FST_VD_IMPLICIT, 1, "mymember", 0);

    // We are in the "outer_struct" context
    fstWriterSetScope(m_fst, FST_ST_VCD_STRUCT, "inner_struct", NULL);
    fstHandle handle2 = fstWriterCreateVar(m_fst, FST_VT_SV_BIT, FST_VD_IMPLICIT, 1, "mymember2", 0);

    // Move up the "outer_struct" context
    fstWriterSetUpscope(m_fst);

    // Move up the "top" context
    fstWriterSetUpscope(m_fst);

    fstWriterEmitValueChange(m_fst, handle1, "1");
    fstWriterEmitValueChange(m_fst, handle2, "0");

Generates something like:

struct outer_struct {
    bit mymember;
    struct inner_struct {
        bit mymember2;
    };
};

#14 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

Please add the following patch to your set and get the test(s) to pass (test_regress/t/t_trace_fst.pl)

The calls in fstapi.cpp need to cast the malloc returns, plus there's probably other stuff. (Once clean, I'll feed this change upstream.)

Your patches look good, so once the tests pass I think it's ready to commit if you think so too.

I'm not sure how to run the tests to be honest, 'make test' gives the following output: https://pastebin.com/raw/pcW3vbDx

EDIT: Do you want me to modify fstapi.cpp to make the pointer casts explicit (required by C++)? Although I don't like this idea very much since it's code I've copied from GTKWave and we would have to copy and update it every time GTKWaves changes it instead of copying, I can't think of any other way to silent those warnings. Actually, I had to pass -fpermissive to turn them from errors to warnings.

#15 Updated by Wilson Snyder 3 months ago

Type

test_regress/t/t_trace_fst.pl

Or safer, in case you changed something in the sources,

make && test_regress/t/t_trace_fst.pl

For more details do a "make info" then see internals.txt in the top level directory.

#16 Updated by Wilson Snyder 3 months ago

Also you should add to your ~/.bashrc or equivalent

export VERILATOR_AUTHOR_SITE=1

Then rerun ./configure. Then a "make test" will run every test not just the simple ones, and also warnings will be turned on.

#17 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

Type

test_regress/t/t_trace_fst.pl

Or safer, in case you changed something in the sources,

make && test_regress/t/t_trace_fst.pl

For more details do a "make info" then see internals.txt in the top level directory.

test_regress/t/t_trace_fst.pl gives me:
======================================================================
dist/t_trace_fst: ==================================================
-Skip: dist/t_trace_fst: scenario 'dist' not enabled for test
dist/t_trace_fst: %Skip: Skip: scenario 'dist' not enabled for test

Not sure that's the intended output. I've also tried with VERILATOR_AUTHOR_SITE=1 but it gives the same.

Btw I've attached all the patches I've done up to this point (I haven't modified fstapi.cpp to add explicit casts yet, do you want me to do that?, so we can get rid of the warnings and avoid passing -fpermissive).

#18 Updated by Wilson Snyder 3 months ago

Sorry, to test

make && test_regress/t/t_trace_fst.pl --vlt

Going forward can you please send just a combined patch

git diff origin/master...HEAD > combined.patch

(This assumes origin/master is the verilator upstream repo. If you're on a branch presumably you know how to modify this.)

#19 Updated by Wilson Snyder 3 months ago

I did push the LTX2 typo fix, thanks. So do a "git pull" before the diff.

#20 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

Sorry, to test

make && test_regress/t/t_trace_fst.pl --vlt

Going forward can you please send just a combined patch

git diff origin/master...HEAD > combined.patch

(This assumes origin/master is the verilator upstream repo. If you're on a branch presumably you know how to modify this.)

Now I'm getting tons of errors related to the implicit pointer casts (among other errors).

I guess now it's a good point to fix those warnings? Maybe another option would be to compile fstapi using gcc instead of g++. I've also tried including the fstapi.c inside an extern "C" block but g++ spits the same errors.

#21 Updated by Wilson Snyder 3 months ago

Yes, please fix the casts and don't turn off new warnings, nor use non-g++.

#22 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

Yes, please fix the casts and don't turn off new warnings, nor use non-g++.

There are tons of errors, even to things non related to casts such as:
include/fst/fstapi.c:3984:38: error: invalid suffix on literal; C++11 requires a space between literal and string macro [-Werror=literal-suffix]
         if(xc->timezero) fprintf(fv, "$timezero\n\t%"PRId64"\n$end\n", xc->timezero);
...
/home/xerpi/Desktop/verilator/test_regress/../include/fst/fstapi.c:6393:14: error: this statement may fall through [-Werror=implicit-fallthrough=]
    case 10: c+=((uint32_t)k[9]<<16);

I'll try to fix them as a short-term solution but I think the best would be to make GTKWave install fstapi globally (as libfstapi) along with it, so for example pkg-config would be able to find it. This would not only be useful for Verilator but for all the projects that want to use/use fstapi.

Meanwhile this happens (if it ever does), I think we should build libfstapi.a using its own Makefile locally and install it alongside Verilator. Then add addLdLibs("-lfstapi"); to V3Options.

#23 Updated by Sergi Granell 3 months ago

I've fixed all the errors, now it compiles fine.

Now when I run 'make && test_regress/t/t_trace_fst.pl --vlt' I get the following errors (I'll check the outputs and try to figure out what's wrong):
======================================================================
vlt/t_trace_fst: ==================================================
vlt/t_trace_fst: Compile
    perl ../bin/verilator --prefix Vt_trace_fst --x-assign unique -cc -Mdir obj_vlt/t_trace_fst -OD --debug-check --comp-limit-members 10 --clk clk  -f input.vc +define+TEST_OBJ_DIR=obj_vlt/t_trace_fst --trace-fst t/t_trace_fst.v    > obj_vlt/t_trace_fst/vlt_compile.log
vlt/t_trace_fst: GCC
    make -C obj_vlt/t_trace_fst -f /home/xerpi/Desktop/verilator/test_regress/Makefile_obj VM_PREFIX=Vt_trace_fst TEST_OBJ_DIR=obj_vlt/t_trace_fst CPPFLAGS_DRIVER=-DT_TRACE_FST    Vt_trace_fst    > obj_vlt/t_trace_fst/vlt_gcc.log
make: Entering directory '/home/xerpi/Desktop/verilator/test_regress/obj_vlt/t_trace_fst'
/usr/bin/perl /home/xerpi/Desktop/verilator/test_regress/../bin/verilator_includer -DVL_INLINE_OPT=inline Vt_trace_fst__main.cpp /home/xerpi/Desktop/verilator/test_regress/../include/verilated.cpp /home/xerpi/Desktop/verilator/test_regress/../include/verilated_fst_c.cpp Vt_trace_fst.cpp Vt_trace_fst__Trace.cpp Vt_trace_fst__Syms.cpp Vt_trace_fst__Trace__Slow.cpp > Vt_trace_fst__ALLboth.cpp
g++  -I. -Wall -Wextra -Wfloat-conversion -Wlogical-op -Werror -MMD -I/home/xerpi/Desktop/verilator/test_regress/../include -I/home/xerpi/Desktop/verilator/test_regress/../include/vltstd -DVL_PRINTF=printf -DVM_COVERAGE=0 -DVM_SC=0 -DVM_TRACE=1 -faligned-new -Wno-bool-operation -Wno-sign-compare -Wno-uninitialized -Wno-unused-but-set-variable -Wno-unused-parameter -Wno-unused-variable -Wno-shadow      -DVERILATOR=1 -DVL_DEBUG=1 -DTEST_OBJ_DIR=obj_vlt/t_trace_fst -DVM_PREFIX=Vt_trace_fst -DVM_PREFIX_INCLUDE="<Vt_trace_fst.h>" -DT_TRACE_FST    -c -o Vt_trace_fst__ALLboth.o Vt_trace_fst__ALLboth.cpp
g++    -g Vt_trace_fst__ALLboth.o   -lz  -o Vt_trace_fst -lm -lstdc++ 2>&1
make: Leaving directory '/home/xerpi/Desktop/verilator/test_regress/obj_vlt/t_trace_fst'
vlt/t_trace_fst: Run
    obj_vlt/t_trace_fst/Vt_trace_fst    > obj_vlt/t_trace_fst/vlt_sim.log
*-* All Finished *-*
- t/t_trace_fst.v:39: Verilog $finish
%Warning: vlt/t_trace_fst: VCD hier mismatch obj_vlt/t_trace_fst/simx-fst2vcd.vcd t/t_trace_fst.out

vlt/t_trace_fst: %Error: VCD hier mismatch obj_vlt/t_trace_fst/simx-fst2vcd.vcd t/t_trace_fst.out

(I renamed all instances of fstvcd to fst2vcd by the way).

I also attach the full FST patch.

EDIT: After further inspection, I see what's going on: I'm generating SystemVerilog-specific signal types (bit, for instance) and vcddiff only understands regular Verilog types (https://github.com/veripool/vcddiff/blob/master/vcddiff.c#L272). I'll change the FST backend to emit regular Verilog types for now. In case you are curious, this is the vcddiff error:
$ vcddiff test_regress/obj_vlt/t_trace_fst/simx-fst2vcd.vcd test_regress/t/t_trace_fst.out
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
Error- Unknown variable type bit
WARNING - Ignoring signal top.clk (!) - types don't match
Segmentation fault (core dumped)

#24 Updated by Wilson Snyder 3 months ago

Great progress, almost there. Does gtkwave provide a diff tool? That would be perfect if so.

#25 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

Great progress, almost there. Does gtkwave provide a diff tool? That would be perfect if so.

There's a missing piece, which is making it generate the proper hierarchy (scoping).

The name passed to the declXXX functions is a space-separated string containing the full symbol hierarchy up to the new symbol. For the LXT2 backend this is very convenient as you only have to change spaces to '.' and call lxt2_wr_symbol_add.

This is trickier for FST because fstWriterSetScope and fstWriterSetUpscope act as a push/pop stack, and if you call SetScope multiple times (given the same name), it creates duplicates, so you have to make sure you create all the vars (fstWriterCreateVar) inside the SetScope/SetUpscope pair. Can I assume that Verilator does that (all the declXXX calls are "in order")? Otherwise handling the current scope gets a bit tricky (list of strings, comparing the list with the new full symbol name, etc).

#26 Updated by Wilson Snyder 3 months ago

They should be in-order. Also I'm thinking we should rename include/fst and include/lxt2 to just include/gtkwave, as more obvious where they come from and then new formats won't need more directories, ok?

#27 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

They should be in-order. Also I'm thinking we should rename include/fst and include/lxt2 to just include/gtkwave, as more obvious where they come from and then new formats won't need more directories, ok?

Sure, I'll move them to include/gtkwave.

Implementing the hierarchy in FST is turning out more difficult than expected since I need to know when the variable declarations of the hierarchy are done to be able to call the latest fstWriterSetUpscope.

The pseudocode would be like:
void struct_node_visit(node_t *node)
{
    fstWriterSetScope(FST_ST_VCD_STRUCT, node->name());

    for (auto &child: node->children() {
        if (child->isStruct()
            struct_node_visit(child);
        else /* regular variable */
            fstWriterCreateVar(child->name(), ...);
    }

    fstWriterSetUpscope();
}

void scope_node_visit(node_t *node)
{
    fstWriterSetScope(FST_ST_VCD_SCOPE, node->name());

    for (auto &child: node->children() {
        if (child->isScope())
            scope_node_visit(child);
        else if (child->isStruct()
            struct_node_visit(child);
        else /* regular variable */
            fstWriterCreateVar(child->name(), ...);
    }

    fstWriterSetUpscope();
}

void emit_vars()
{
    scope_node_visit(module->getRoot());
}

#28 Updated by Wilson Snyder 3 months ago

FYI I moved include/lxt2 to include/gtkwave, please pull.

#29 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

FYI I moved include/lxt2 to include/gtkwave, please pull.

Thanks, pulled!

Btw as I said I can't properly finish the FST backend unless we add a way to know when the declarations are done, maybe we could add ->declsStart() and ->declsEnd() callbacks (they would be empty on VCD and LXT2) to be able to close the uppermost context of the hierarchy (needed by FST)?

#30 Updated by Wilson Snyder 3 months ago

The declarations are triggered by the "m_initcb" callback execution. Presently this is called in a loop in VerilatedFst::open(), so you can just add the appropriate start/end calls inside VerilatedFst::open, no Verilator source changes needed.

#31 Updated by Sergi Granell 3 months ago

Wilson Snyder wrote:

The declarations are triggered by the "m_initcb" callback execution. Presently this is called in a loop in VerilatedFst::open(), so you can just add the appropriate start/end calls inside VerilatedFst::open, no Verilator source changes needed.

Thanks, that was useful.

I think I've managed to get the hierarchy properly working but I don't know why it doesn't pass the test. It might be related to fst2vcd, since it seems like it doesn't output the range ([X:Y]) of buses, for example, the expected output is:
$var wire 5 " state [4:0] $end
while using FST and then fst2vcd produces:
$var wire 5 " state $end
This makes vcddiff print the following error (it thinks they are two separate signals since the range doesn't match):
WARNING - Ignoring signal top.state (") - not defined in both files
      Defined in file 'test_regress/obj_vlt/t_trace_fst/simx-fst2vcd.vcd' and not file 'test_regress/t/t_trace_fst.out'

I attach my complete FST patch anyways.

#32 Updated by Wilson Snyder 3 months ago

  • Status changed from Feature to Resolved

Great, pushed to git plus a few minor changes that GTKwave updated.

If you're ready to start on more advanced tracing please open a new bug, and note if you have a proposal for how the Verilator calls should look.

#33 Updated by Wilson Snyder 2 months ago

  • Status changed from Resolved to Closed

In 4.004, thanks for your contribution.

Also available in: Atom