-
Notifications
You must be signed in to change notification settings - Fork 244
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
[ci/processor] test all GHDL backends, test VUnit master too #126
Conversation
Regarding this error (https://github.com/stnolting/neorv32/pull/126/checks?check_run_id=3056287922#step:7:973):
There is a typo in neorv32/rtl/templates/processor/neorv32_ProcessorTop_stdlogic.vhd Lines 215 to 218 in e6da52c
|
@stnolting that is correct. Thanks for fixing it! It allowed to clean #116! However, it does still not explain why one version of VUnit ignores that bug but a newer version fails. It's the same NEORV32 codebase in both cases, same testbench. Therefore, since the bug existed, CI should have failed as soon as you pushed that. However, it slipped into the repo because the current stable VUnit and latest GHDL do not complain about it. |
Is this really a problem of VUnit or might it be a problem of/with GHDL? I am currently using an older version (I think) of GHDL using no VUnit at all on my Windows WSL system and that version also ignored the bug.
|
I'm pretty sure it's because of some change in VUnit's VCs, which get connected to the NEORV32 signals. During elaboration, the error is reported in the NEORV32 source, but it is motivated by where is that connected to outside. My perception is that VUnit might have some explicit casting in the stable version which was removed in master. That casting might have hidden the bug. You can try the following: git clone https://github.com/VUnit/vunit
PYTHONPATH=$(pwd)/vunit
./sim/run.py
# That should fail
cd vunit
git checkout -b stable v4.5.0
cd ..
rm -rf vunit_out
./sim/run.py
# That should pass |
e5b2d9f
to
a9d1e18
Compare
This seems to be fixed with the updates to NEORV32 during the last 1-2 months. I'm closing it, since there is fortunately no point in spending time into this one. |
This PR is NOT to be merged. It's for illustrating an issue.
As commented in #116, when changing the container image used for running VUnit, it would fail. In this PR, the VUnit test is executed six times:
All three jobs using VUnit stable do work, but the ones using VUnit master fail.
The problem seems to be in the NEORV32 codebase, so it's surprising to me why changing the version of VUnit triggers it. @LarsAsplund, any guess?