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

Unittest failure in coupling.d(48) #1

Closed
gmlewis opened this issue Mar 4, 2019 · 11 comments
Closed

Unittest failure in coupling.d(48) #1

gmlewis opened this issue Mar 4, 2019 · 11 comments

Comments

@gmlewis
Copy link

gmlewis commented Mar 4, 2019

When I run dub test on Linux Mint, everything passes except:

$ dub test
...
Testing Coupling
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Coupling.v
make -f VCoupling.mk
core.exception.AssertError@source/dhdl/tests/coupling.d(48): unittest failure
----------------
??:? _d_unittestp [0x6f499c2d]
source/dhdl/tests/coupling.d:48 void dhdl.tests.coupling.__unittest_L40_C18() [0x6f44bb10]
??:? void dhdl.tests.coupling.__modtest() [0x6f44be30]
??:? int core.runtime.runModuleUnitTests().__foreachbody2(object.ModuleInfo*) [0x6f4ce623]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*)) [0x6f498db2]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody2(ref rt.sections_elf_shared.DSO) [0x6f4a1535]
??:? int rt.sections_elf_shared.DSO.opApply(scope int delegate(ref rt.sections_elf_shared.DSO)) [0x6f4a16c8]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))) [0x6f4a14c1]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) [0x6f498d89]
??:? runModuleUnitTests [0x6f4ce401]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll() [0x6f49bfe8]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) [0x6f49bf73]
??:? _d_run_main [0x6f49bee0]
??:? main [0x6f3c4a69]
??:? __libc_start_main [0x1b9b1b96]
...
$ dmd --version
DMD64 D Compiler v2.085.0
Copyright (C) 1999-2019 by The D Language Foundation, All Rights Reserved written by Walter Bright

Also, are there any example designs using dhdl?

@luismarques
Copy link
Owner

Hi @gmlewis, thanks for the bug report. Can you please post the generated rtl/Coupling/obj_dir/VCoupling.h (or the entire /rtl/Coupling/obj_dir folder) somewhere for me to take a look? My current best guess of what might be happening here is that there is a bug computing the offset of where that signal is represented in memory, so having that header file would help diagnose it. Is it a 32- or 64-bit OS?

This DHDL design is not very good, and I published it mostly just as a stop gag. Fortunately, at the end of last year I did a redesign, which I am much happier with. Then I started a new job and I dropped work on this for a while. By coincidence, just this past weekend I picked this up again, and I started converting my DConf demo to use the new design. Over the following weeks, I'm going to be finishing the conversion, cleaning up the demo hardware design and I'll publish both the new DHDL version (in this repo) and my demo hardware (in another repo). I'm also working on a compiler that will serve as a testbed for several D-related experiments, including as a frontend for DHDL, so that you can design hardware with a much more natural syntax than the DHDL library affords, but that will take longer to achieve.

@gmlewis
Copy link
Author

gmlewis commented Mar 4, 2019

Hi @luismarques - sure, I've made gists of VCoupling.h and VCoupling.cpp for you.
This is a 64-bit Linux machine I'm running on.

But after reading your second paragraph, I'm really excited to try out your new design, so if you want to ignore this report and focus on the new design, that is totally fine with me! 😄
That sounds totally awesome and I look forward to checking it out!

@luismarques
Copy link
Owner

The new design still uses the same/similar code for getting the signal offsets, so it's still important to get to the bottom of this issue. Thanks for the files, I'll take a look at them.

@luismarques
Copy link
Owner

luismarques commented Mar 5, 2019

@gmlewis In the meanwhile, if you could upgrade to the latest version of Verilator (it should be easy to compile it yourself) and try running the tests again, that would also help.

@gmlewis
Copy link
Author

gmlewis commented Mar 6, 2019

Hi @luismarques - it turns out I already was using the latest version:

$ verilator --version
Verilator 4.010 2019-01-27 rev UNKNOWN_REV

@luismarques
Copy link
Owner

@gmlewis Thanks. When I find the time I'll send you a (source code) program that will diagnose this issue by running the simulation both from C++ and D, as well as looking at the signal memory addresses, to try to find the discrepancy.

@luismarques
Copy link
Owner

@gmlewis Before I write the program to diagnose this situation, I thought I should see if I could reproduce it locally, as that would be easier to debug. I installed Linux Mint but I couldn't reproduce the issue (all tests passed). Could you please provide more information, such as:

  • Linux Mint version
  • Which D compiler was used (DMD, LDC2, GDC...)
  • D compiler version
  • G++ version (because of Verilator)
  • Dub version
  • Confirm the steps to reproduce (is it a pristine repo clone? just dub test? etc.)
  • Anything else you think could be relevant.

Thanks!

@gmlewis
Copy link
Author

gmlewis commented Mar 8, 2019

I'm now away from my machine. I should have access again next week and will get the answers for you.

@gmlewis
Copy link
Author

gmlewis commented Mar 15, 2019

Sorry for the delay!

  • Operating system: Linux Mint 19 Cinnamon
  • Cinnamon Version: 3.8.9
  • Linux Kernel: 4.15.0-45-generic
  • Processor: Intel Core i7-7500U CPU @ 2.70GHz x 2
  • Memory: 15.5 GiB
  • Using dmd compiler version: DMD64 D Compiler v2.085.0
  • g++ --version: g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
  • dub --version: DUB version 1.14.0, built on Mar 2 2019
$ mv dhdl dhdl-original
glenn@glenn-Inspiron-17-7779 ~/src/github.com/luismarques  $ git clone git@github.com:luismarques/dhdl
Cloning into 'dhdl'...
remote: Enumerating objects: 122, done.
remote: Total 122 (delta 0), reused 0 (delta 0), pack-reused 122
Receiving objects: 100% (122/122), 30.58 KiB | 2.78 MiB/s, done.
Resolving deltas: 100% (67/67), done.
glenn@glenn-Inspiron-17-7779 ~/src/github.com/luismarques  $ cd dhdl
glenn@glenn-Inspiron-17-7779 ~/src/github.com/luismarques/dhdl (master) $ dub test
Generating test runner configuration 'dhdl-test-library' for 'library' (library).
Excluding package.d file from test due to https://issues.dlang.org/show_bug.cgi?id=11847
Performing "unittest" build using /usr/bin/dmd for x86_64.
openmethods 1.1.0: target for configuration "library" is up to date.
dhdl ~master: building configuration "dhdl-test-library"...
Linking...
To force a rebuild of up-to-date targets, run again with --force.
Running ./dhdl-test-library 
Testing Queue!(UInt!16, 8)
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Queue.v
make -f VQueue.mk
Testing Arithmetic
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Arithmetic.v
make -f VArithmetic.mk
Testing Bundles1
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Bundles1.v
make -f VBundles1.mk
Testing Bundles2
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Bundles2.v
make -f VBundles2.mk
Testing Clocked
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Clocked.v
make -f VClocked.mk
Testing Outer
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Outer.v
make -f VOuter.mk
Testing Concatenation
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Concatenation.v
make -f VConcatenation.mk
Testing Conditionals
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Conditionals.v
make -f VConditionals.mk
Testing Coupling
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Coupling.v
make -f VCoupling.mk
core.exception.AssertError@source/dhdl/tests/coupling.d(48): unittest failure
----------------
??:? _d_unittestp [0x6c21dc2d]
source/dhdl/tests/coupling.d:48 void dhdl.tests.coupling.__unittest_L40_C18() [0x6c1cfb10]
??:? void dhdl.tests.coupling.__modtest() [0x6c1cfe30]
??:? int core.runtime.runModuleUnitTests().__foreachbody2(object.ModuleInfo*) [0x6c252623]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*)) [0x6c21cdb2]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody2(ref rt.sections_elf_shared.DSO) [0x6c225535]
??:? int rt.sections_elf_shared.DSO.opApply(scope int delegate(ref rt.sections_elf_shared.DSO)) [0x6c2256c8]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))) [0x6c2254c1]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) [0x6c21cd89]
??:? runModuleUnitTests [0x6c252401]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll() [0x6c21ffe8]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) [0x6c21ff73]
??:? _d_run_main [0x6c21fee0]
??:? main [0x6c148a69]
??:? __libc_start_main [0xd3ba8b96]
Testing HelloWorld
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp HelloWorld.v
make -f VHelloWorld.mk
Testing Indexing
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Indexing.v
make -f VIndexing.mk
Testing Matching
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Matching.v
make -f VMatching.mk
Testing Memories
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Memories.v
make -f VMemories.mk
Testing Operators
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Operators.v
make -f VOperators.mk
Testing Registers
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Registers.v
make -f VRegisters.mk
Testing Slicing
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Slicing.v
make -f VSlicing.mk
Testing VectorPorts
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp VectorPorts.v
make -f VVectorPorts.mk
Testing VectorReg
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp VectorReg.v
make -f VVectorReg.mk
Testing VectorROM
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp VectorROM.v
make -f VVectorROM.mk
Testing Wires
verilator -CFLAGS -fPIC -LDFLAGS -shared --cc --exe lib.cpp Wires.v
make -f VWires.mk
1/21 unittests FAILED
Program exited with code 1
glenn@glenn-Inspiron-17-7779 ~/src/github.com/luismarques/dhdl (master) $ 

@luismarques
Copy link
Owner

luismarques commented Mar 24, 2019

@gmlewis Thanks for the bug report. The problem was the reset logic. Before the fix (3d4514f), DHDL did:

reset = 1;
clock = 1;
eval();

reset = 0;
clock = 0;
eval()

It now does:

reset = 1;
eval();
clock = 1;
eval();
reset = 0;
eval();
clock = 0;
eval()

I'm not quite sure how Verilog's event ordering requirements and the Verilator eval() semantics are supposed to interact, and the Verilator manual doesn't really delve into such details, AFAIK. I guess that, in the case of the old code, the signals would be considered as changing at the same time. In that case Verilog does not specify any ordering requirements, I believe, so Verilator was free to evaluate them in any order. The old Verilator version probably evaluated reset, clock, while the new one probably evaluates clock, reset, breaking the reset logic, since the reset signal is still 0 when the clock edge arrives. There already was a code smell regarding this: for the regular (non-reset) time stepping logic, DHDL already did eval(); clock = 1; eval(), to ensure that the signals changed by the user were properly propagated. The fact that the reset logic did something different was a red flag. I believe what happened is that I tested it without the extra evals, saw that it worked properly (on the older Verilator versions), and decided to keep it that way, since if there was no difference we would save a little time in the simulation. For a while I did have that newer Verilator version installed on my computer, but I guess I never ran the DHDL tests with it.

Notes for future improvement:

  • Set a CI infrastructure that runs the tests with multiple Verilator versions (e.g. the latest one, plus a large number of older versions)
  • Despite this issue not relating to differences in the Verilator-generated headers, as I suspected would be the case, it might be a good idea to move the logic to compute the memory addresses of the signals to the C++ code. That way the D code just reads some table entry or return value of a C++ function, and it doesn't have to parse the C++ headers.
  • Add more unittests, to try to catch such subtle signal update issues.

@gmlewis
Copy link
Author

gmlewis commented Mar 24, 2019

That fixed it for me, @luismarques! Thank you!

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

No branches or pull requests

2 participants