Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-29T21:50:00Z

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.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-29T22:07:57Z

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":http://veripool.org/boards/3/topics/2601

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-29T22:10:03Z

Also see how the LXT2 stuff was added

git show acf4a3f

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T07:17:35Z

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

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T08:40:46Z

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: !fst.jpg!

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T09:53:24Z

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T09:55:50Z

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.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T10:03:05Z

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.

Original Redmine Comment
Author Name: Iztok Jeras (@jeras)
Original Date: 2018-09-30T11:38:05Z

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.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T11:58:57Z

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.

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T12:18:38Z

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T13:58:07Z

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.

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T13:59:11Z

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");
</code>

Generates something like:

struct outer_struct {
	bit mymember;
	struct inner_struct {
		bit mymember2;
	};
};
</code>

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T20:05:40Z

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.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T20:50:38Z

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.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T21:02:02Z

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.

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T21:26:14Z

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
</code>

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T21:33:39Z

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T21:35:21Z

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

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-09-30T22:37:19Z

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.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-30T23:13:08Z

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

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-10-01T06:10:18Z

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);
</code>

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.

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-10-01T07:08:29Z

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
</code>

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-10-01T10:24:13Z

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

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-10-01T14:55:39Z

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-10-01T15:07:37Z

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?

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-10-01T15:46:56Z

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());
}
</code>

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-10-01T22:30:11Z

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

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-10-02T09:39:53Z

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-10-02T10:35:04Z

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.

Original Redmine Comment
Author Name: Sergi Granell (@xerpi)
Original Date: 2018-10-02T16:02:05Z

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
</code>

while using FST and then fst2vcd produces:

$var wire 5 " state $end
</code>

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'
</code>

I attach my complete FST patch anyways.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-10-02T22:44:34Z

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.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-10-06T14:13:53Z

In 4.004, thanks for your contribution.