Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support LXT2 file format natively #1333

Closed
veripoolbot opened this issue Aug 26, 2018 · 14 comments
Closed

Support LXT2 file format natively #1333

veripoolbot opened this issue Aug 26, 2018 · 14 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Aug 26, 2018


Author Name: Yu Sheng Lin (@johnjohnlin)
Original Redmine Issue: 1333 from https://www.veripool.org

Original Assignee: Yu Sheng Lin (@johnjohnlin)


I make a issue about "this development thread":https://www.veripool.org/boards/3/topics/2601-Verilator-A-way-to-support-LXT2-file-format-natively .

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 27, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-08-27T10:55:37Z


I used this repo rather than the patch: https://github.com/johnjohnlin/verilator_fork/tree/lxt2
no need to attach patches if you have a public git repo.

Great work! Here's the feedback on your patches. If any of these are hard/non-obvious I can clean them up for you.

  • I noted the lxt2_* functions are copyright 2016, that's ok by itself but did you check you have the most recent versions?

  • Please add documentation for --trace-lxt2 to bin/verilator.

  • "Also LXT2 format requires zlib, so add this to the Verilator flags."

Probably better to this automatically when --trace-lxt2 is specified with

             addLdLibs("-lz");
  • The ./Makefile.in will need some changes to install the new include/lxt2 files.

  • Please change include/verilated_lxt2* to use the Verilator standard indentation. (No tabs, GNU style. You can diff with verilated_vcd* to see it.) If you don't have Emacs to do this for you I can.

  • Thanks for making an enum for the trace type. Instead of a standard enum please use an enum class TraceFormat similar to how AstNumeric (in V3Ast.h) is done, then the related methods traceClassBase traceSourceName can become members of that.

  • Good idea to use lxt2vcd. Can you make this just

    lxt2vcd("$Self->{obj_dir}/simx.lxt2", "$Self->{obj_dir}/simx.vcd");

then add a new "sub lxt2vcd" to driver.pl? This can then give an appropriate error if it isn't installed (similar "sub vcd_identical").

  • Instead of a custom t_trace_ltx2.cpp, please have driver.pl create the appropriate code automatically when it sees the --trace-lxt2 switch. Also note you can't use C++11 f
    eatures (constexpr).

  • Probably all of the existing t_trace_* tests can get new t_trace_*_lxt2 versions which just point at the existing verilog (for the VCD version) with:

     top_filename("t/t_trace_WHATEVER.v");
    

Once that is done the t_trace_lxt2.v might not have anything special it in and can be removed.

I suspect once you do this you'll see you need the other full/chg functions ;)

  • The t_trace_lxt2 test for me gives this error:
         lxt2vcd obj_vlt/t_trace_lxt2/simx.lxt2 -o obj_vlt/t_trace_lxt2/simx.vcd
         LXTLOAD | 14 facilities
         LXTLOAD | Read 1 block header OK
         LXTLOAD | [0] start time
         LXTLOAD | [2002] end time
         LXTLOAD |
         LXTLOAD | block [0] processing 0 / 2002
         ***ERROR Unknown token '^S\200' on line 0 of obj_vlt/t_trace_lxt2/simx.lxt2
         ***ERROR Unknown token 'QT\$\301\246' on line 0 of obj_vlt/t_trace_lxt2/simx.lxt2
         ...

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 27, 2018


Original Redmine Comment
Author Name: Yu Sheng Lin (@johnjohnlin)
Original Date: 2018-08-27T17:01:42Z


Thanks for reviewing this patch. I just pushed a new version and fixed most of the problems except for these two:

  • The ./Makefile.in will need some changes to install the new include/lxt2 files.

It seems that it is not necessary, the Makefile installs @include/.[vch]@.

  • please have driver.pl create the appropriate code automatically when it sees the --trace-lxt2 switch

Sorry I don't know how to do that because I did not write Perl before.

  • Probably all of the existing t_trace_* tests can get new t_trace_*_lxt2 versions

Yes, it should be added in the future.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 27, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-08-27T23:38:59Z


Please pull from the #�_lxt2 branch of the http://git.veripool.org/git/verilator repository as I made some cleanups and additions to your version.

Then please fix the following tests

     test_regress/t/t_trace_array_lxt2.pl  --vlt
     test_regress/t/t_trace_complex_lxt2.pl  --vlt
     test_regress/t/t_trace_complex_params_lxt2.pl  --vlt
     test_regress/t/t_trace_complex_structs_lxt2.pl  --vlt
     test_regress/t/t_trace_packed_struct_lxt2.pl  --vlt

Also separately, for the changes you made to the lxt2 sources (changing the include to "", and the casts) can you please file a bug with Bybell to get these upstreamed to gtkWave.

Getting close, thanks!

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 28, 2018


Original Redmine Comment
Author Name: Yu Sheng Lin (@johnjohnlin)
Original Date: 2018-08-28T09:16:15Z


Hello, the new version is updated to "this branch":https://github.com/johnjohnlin/verilator_fork/tree/#�_lxt2 .
Also, the lxt functions were C code, which can be compiled gcc. I will report it to GTKWave but they might not accept the change.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 28, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-08-28T10:43:28Z


Excellent, made a few more minor cleanups and pushed to git towards 4.000.

Thanks for your good work.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 30, 2018


Original Redmine Comment
Author Name: Frederic Requin
Original Date: 2018-08-30T16:11:01Z


Hello, the linker fails if I trace a vector with more than 32 bits.

I have an undefined reference to "VerilatedLxt2::quad2Str(unsigned long, int)"

g++    main.o verilated_dpi.o verilated.o verilated_lxt2_c.o Vecp5_jtag_spi_top__ALL.a   -lz  -o Vecp5_jtag_spi_top -lm -lstdc++
Vecp5_jtag_spi_top__ALL.a(Vecp5_jtag_spi_top__ALLsup.o):Vecp5_jtag_spi_top__ALLsup.cpp:(.text+0x1821) : référence indéfinie vers « VerilatedLxt2::quad2Str(unsigned long, int) »
Vecp5_jtag_spi_top__ALL.a(Vecp5_jtag_spi_top__ALLsup.o):Vecp5_jtag_spi_top__ALLsup.cpp:(.text+0x1821): relocalisation tronquée pour concorder avec la taille : R_X86_64_PC32 vers le symbole indéfini VerilatedLxt2::quad2Str(unsigned long, int)
Vecp5_jtag_spi_top__ALL.a(Vecp5_jtag_spi_top__ALLsup.o):Vecp5_jtag_spi_top__ALLsup.cpp:(.text+0x3466) : référence indéfinie vers « VerilatedLxt2::quad2Str(unsigned long, int) »
Vecp5_jtag_spi_top__ALL.a(Vecp5_jtag_spi_top__ALLsup.o):Vecp5_jtag_spi_top__ALLsup.cpp:(.text+0x3466): relocalisation tronquée pour concorder avec la taille : R_X86_64_PC32 vers le symbole indéfini VerilatedLxt2::quad2Str(unsigned long, int)
collect2: error: ld a retourné le statut de sortie 1
make: *** [Vecp5_jtag_spi_top.mk:69: Vecp5_jtag_spi_top] Error 1

I am using GCC 7.3.0 under Cygwin 64-bit.

I have double-checked the generated code, nothing seems to be wrong.

Verilog code :

 reg [47:0] r_er1_dr;

Vecp5_jtag_spi_top.h :

VL_SIG64(ecp5_jtag_spi_top__DOT__r_er1_dr,47,0);

Vecp5_jtag_spi_top__Trace__Slow.cpp :

vcdp->fullQuad (c+41,(vlTOPp->ecp5_jtag_spi_top__DOT__r_er1_dr),48);

Vecp5_jtag_spi_top__Trace.cpp :

vcdp->chgQuad (c+41,(vlTOPp->ecp5_jtag_spi_top__DOT__r_er1_dr),48);

Any clue ?

Regards,

Frederic

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 30, 2018


Original Redmine Comment
Author Name: Yu Sheng Lin (@johnjohnlin)
Original Date: 2018-08-30T16:53:20Z


Quad-word dumping is tested correctly in @t_trace_complex_lxt2@ (acf4a3).
The function is in @verilated_lxt2_c.cpp@.
Can you run this to check whether that function is defined?

It should give something like

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 31, 2018


Original Redmine Comment
Author Name: Frederic Requin
Original Date: 2018-08-31T07:57:27Z


Hello,
I have got :

                  U VerilatedLxt2::quad2Str(unsigned long, int)
0000000000003bd0 T VerilatedLxt2::quad2Str(unsigned long long, int)

The first line is from "Vecp5_jtag_spi_top__ALLsup.o", the second one from "verilated_lxt2_c.o".

Those do match the linker error.

Why vluint64_t does not generate the same type in both files, I do not know.

Regards,

Frederic

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 31, 2018


Original Redmine Comment
Author Name: Frederic Requin
Original Date: 2018-08-31T08:42:16Z


By putting warning pragmas in verilatedos.h, I can see that Vecp5_jtag_spi_top__ALLsup.cpp got compiled with "typedef unsigned long vluint64_t;" (good behaviour) and other .cpp files with "typedef unsigned long long vluint64_t;" (wrong behaviour).

If I revert back to VCD tracing, I see the same typedef issue but the linker does not complain.

Very strange.

Frederic

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 31, 2018


Original Redmine Comment
Author Name: Frederic Requin
Original Date: 2018-08-31T08:59:52Z


Solved by including <sys/types.h> in verilatedos.h.

It looks like an old bug that never got caught.

Frederic

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 31, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-08-31T10:53:01Z


Thanks for debugging! I added a sys/types include, this seems to set __WORDSIZE. It has to be conditional on the OS, so please see if the current git works for you.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 3, 2018


Original Redmine Comment
Author Name: Frederic Requin
Original Date: 2018-09-03T11:55:28Z


Yep, it works. Thank you.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 16, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-09-16T21:27:19Z


In 4.002, thanks for your work.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 2, 2018


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


Note based on your LXT2 code/changes we just committed adding FST format to git, please give it a try, if there are issues please file a new bug. Our hope is to eventually have fancier SystemVerilog type support in FST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.