VUnit / vunit

VUnit is a unit testing framework for VHDL/SystemVerilog
http://vunit.github.io/
Other
723 stars 258 forks source link

Add GHDL simulator support #24

Closed kraigher closed 9 years ago

kraigher commented 9 years ago

We would like to add support for GHDL. There are some blockers though:

  1. GHDL does not allow setting top level generics during elaboration from the command line. We rely on this feature quite a lot to pass information from the Python-runner to the test bench. Just being able to set 'string' generics would cover our basic needs. Having support for integer, boolean and real would be nice though since it is used by some of our examples.
  2. GHDL does not support breaking simulation when a signal changes value to 'true'. We use the 'exit_without_errors' boolean to break the simulation in the test_runner_cleanup function.

We could try to contact the author of GHDL and see if the above features could be implemented so that we can add GHDL support to VUnit.

LarsAsplund commented 9 years ago

I've added a ticket to the GHDL project.

kraigher commented 9 years ago

When they fix 1. as they promised I will have a go at adding GHDL support.

kraigher commented 9 years ago

I see they have fixed 1 on their development branch. Now we just have until there is a binary release.

kraigher commented 9 years ago

I have managed to build ghdl from the latest commit using the LLVM backed and started ironing out the problems of supporting it. I have found a bug in ghdl when compiling the VUnit code: http://sourceforge.net/p/ghdl-updates/tickets/41/

kraigher commented 9 years ago

I have successfully run a modified user_guide example using VUnit and GHDL now with just a few work-arrounds.

kraigher commented 9 years ago

The test_run.py acceptance test works with GHDL now.

kraigher commented 9 years ago

I have pushed my work in progress to the ghdl branch. It only works with GHDL built from the latest source. I have tested it using the LLVM backed on Ubuntu 14.04 LTS. There are some issues and work-arrounds still but many things work such as uart example, array example, osvvm_integration example, generate_tests example etc. It will only work with VHDL 2008 since for the other standards we have not implemented any stop mechanism for GHDL. Even though the VHDL-2008 supports seems quite large there is no support for contexts so I removed those. Also OSVVM 2015.03 has two LRM violations which GHDL errors on so I temporarily fixed those in this branch. I have posted the violations to the OSVVM forum and Jim Lewis has already promised to fix one of them in the next OSVVM release.

@LarsAsplund There are some LRM violations in our code especially the mocked code that GHDL errors out on. I will try to gather a list of things we need to fix. One common issue seems to be that unlike modelsim GHDL will error out on having shared variables of non-protected type for std >= 2002.

I will keep grinding at this by building the latest GHDL from source and report tickes and hopefully when they release their next official binary release we will have VUnit support for it.

LarsAsplund commented 9 years ago

@kraigher Nice progress! I'll take a look at the non-protected shared variables to see if they can be easily removed. Have you spoken to the GHDL team about context support? It's very convenient to have that.

kraigher commented 9 years ago

@LarsAsplund Yes I have submitted a feature request for it. You can look at their Tickets many of the latest ones are mine. Some of the recently closed tickets are bugs I found in ghdl on using subprogram aliases and ambiguity checks which they quickly fixed. At first our code would not even compile on ghdl but now it does.

I am actually quite impressed by the latest GHDL development branch, it has basic vhdl 2008 support and with the LLVM backend it runs fast and is not hard to build from source unlike the gcc backend.

kraigher commented 9 years ago

They just fixed https://sourceforge.net/p/ghdl-updates/tickets/47/ so that generics without default values will not cause error even though they were specified by the -gflag. I have force pushed the ghdl branch where I have removed the work arround of having default values on all generics in the examples.

kraigher commented 9 years ago

They just fixed https://sourceforge.net/p/ghdl-updates/tickets/51/ which prevented the output binary generated by ghdl from being an absolute path. I have removed our work around for it and force pushed the ghdl branch.

kraigher commented 9 years ago

I have rebased the ghdl branch on master after com was added and applied work the required workarounds. It crashes when with a failing assert of msg_codecs_pkg.vhd and I will investigate why and send a reproducer to the GHDL team. They are usually quick to fix such things.

kraigher commented 9 years ago

I have found the problem with msg_codecs_pkg.vhd and condensed it into a ticket: https://sourceforge.net/p/ghdl-updates/tickets/55/

kraigher commented 9 years ago

They have now added support for context clauses so I have force pushed an updated ghdl branch with those workarounds removed.

kraigher commented 9 years ago

They have fixed https://sourceforge.net/p/ghdl-updates/tickets/55/ which was caused by msg_codecs_pkg.vhd. Now there is a problem with scoreboard.vhd which I will investigate.

kraigher commented 9 years ago

I have located the bug which causes scoreboard.vhd to fail: https://sourceforge.net/p/ghdl-updates/tickets/59/ They have now fixed it.

They have also used to have an error when a the timestamp of an already analyzed file had changed. This caused problems with git since it will often change timestamps as well as with preprocessed and generated codec files. They now check the sha1 hash of the file instead of the timestamp: https://sourceforge.net/p/ghdl-updates/tickets/48/

With these fixes GHDL runs the com example without problems.

LarsAsplund commented 9 years ago

How many of the acceptance tests have successfully run with GHDL. What's left?

kraigher commented 9 years ago

@LarsAsplund Some work at least. I need to refactor the acceptance test code to handle multiple simulators. Also at least initially I think we can only support GHDL with VHDL-2008 since there is no stop mechanism available otherwise. In modelsim we use the wait on test_runner_exit to not rely on stop. A future enhancement might be to add a report with severity failure as a possibility for GHDL < 2008 but there is really no good reason for anyone to use a 2008 compatible simulator without using 2008. I will be away on travel until Sunday so I will have a bit more limited time working at this. But I will gather a list of what fails after I have refactored the acceptance test code.

LarsAsplund commented 9 years ago

@kraigher I push an update to master which removes unprotected shared variables

kraigher commented 9 years ago

@LarsAsplund They have added the option of turning shared variable error into a warning with -frelaxed-rules yesterday. Tank you anyway. By the way I think we should seriously consider dropping support for 93 and even 2002, it costs alot of extra code especially for 93 and it makes the interface worse than it could have been. If we cannot support Vivado xsim then there are no simulators which we are about to support which do not have some subset of 2008 and there is no reason to not use 2008 when simulating.

kraigher commented 9 years ago

@LarsAsplund One of the acceptance test to fail is tb_logging.vhd where the exact contents of log messages are compared. Log messages with times in them fail due to ModelSim timeimage formatting withpswhereas ghdl formats withfs`. time'image will return values with the unit defined by the current simulator resolution which we have set to ps in Modelsim. Maybe we have to set it to fs in Modelsim asvwell to be consistent unless we can make our own time to string function.

LarsAsplund commented 9 years ago

@kraigher Yes, 93 support is adding quite a lot of work. I'm not too concerned with the lack of xsim support. I know that Xilinx has been talking about supporting VHDL 2008 as well, not sure when though. There's also a question about corporate standards. I know some major companies have restrictions but it's unclear to me whether those restrictions apply to testbenches as well. I will try to find out how widespread such restrictions are.

LarsAsplund commented 9 years ago

@kraigher I'll have a look at the logging testbench. I guess I could allow any unit as long as the string represent the expected time.

kraigher commented 9 years ago

@LarsAsplund Any restriction on using VHDL-2008 in test benches would fall apart quite quickly if questioned. There is just no good reason. There might be some good reasons to use 93 only for the code intended to be used by the synthesis tool but even this it is becoming less so. I think it costs us a lot to support 93 for no good reason.

kraigher commented 9 years ago

@LarsAsplund The com example fails due to the impure split_group procedure from com_debug_codec_builder.vhd being called in pure decode functions. The split_group function is impure since it uses check which uses a shared variable.

LarsAsplund commented 9 years ago

@kraigher I removed the checks.

kraigher commented 9 years ago

The com example runs with GHDL now.

kraigher commented 9 years ago

tb_logging.vhd runs with both GHDL and ModelSim now as I use time'image to create the reference strings instead of hardcoding it making it more independent of simulator resolution and time'image format.

kraigher commented 9 years ago

@LarsAsplund com had some problems with reading out parameters. I fixed it in 6d1a9ef532db62723945a260a661f2bc22278e92 on the ghdl branch. What do you think?

kraigher commented 9 years ago

@LarsAsplund tb_check_stable failed with GHDL. It seems it dependend on process execution order. The log count was verified in the same delta-cycle as it was set and on GHDL the proceses executed in different order from modelsim. I have fixed this in the ghdl branch by commit 11541981d264d6cd13d09453cf89417a131fc45c. Seems reasonable, or could it hide some problem?

kraigher commented 9 years ago

Now all acceptance test pass except test_com_debug_vhdl_2008 and test_com_vhdl_2008. I have skipped all tests on 2002 and 93 standards since the current GHDL interface does not support them. I have also skipped the test_end_to_end.py since most test there depend on the vunit_finished signal to stop which GHDL cannot support. I will refactor tests using vunit_finished signal to instead call a vunit_stop procedure which can do VHDL-2008 stop when using ghdl.

The com tests fails have been condensed to the following two tickets: https://sourceforge.net/p/ghdl-updates/tickets/73/ https://sourceforge.net/p/ghdl-updates/tickets/72/

LarsAsplund commented 9 years ago

@kraigher I haven't looked at the details but where is a variable like receipt read? Logically it's an output and it would be better if the interface reflect that. Something that can be solved with local temporary variables?

kraigher commented 9 years ago

@LarsAsplund com.vhd:652-658 for instance:

  if receipt.status = ok then
    messenger.send(message.sender, receiver, message.request_id, message.payload.all, receipt);
  end if;

  if receipt.status = ok then
    notify(net);
  end if;

Could also be solved with a local variable.

LarsAsplund commented 9 years ago

@kraigher Ok, in that case I would prefer the local variable. Another question regarding ticket 72 and 73. Have I used

proc(s(4 to 15) => "Hello world!");

in com?

proc(s => (4 to 15 => "Hello World"));

seems more clear to me.

kraigher commented 9 years ago

It looks like this in tb_com_codecs.vhd:

proc((4 to 15 => "Hello World"));

The other cases I found while investigating and so I reported them as well.

kraigher commented 9 years ago

@LarsAsplund I fixed the com reading output violations by using local variables and pushed to master.

kraigher commented 9 years ago

@LarsAsplund On line 53 of com_types.vhd there is a comment about modelsim not being able to handle time'high. I tried it on my machine and it works. The problem is 1000 hr will overflow in GHDL. I printed time'high in modelsim and it was 9223372036854775807 ps in GHDL it was 9223372036854775807 fs. It would be a nice solution. Otherwise a negative time can be used as well to denote no timeout.

LarsAsplund commented 9 years ago

@kraigher I can't remember exactly why I had a problem with time'high, it was some time ago. Maybe there was another problem.... Let me try again later on tonight. If I can't see any problems I will change back. If there is a problem a negative time is a better solution.

LarsAsplund commented 9 years ago

@kraigher I made a quick test. My com example gets a fatal error when I use time'high. Windows + Modelsim Altera Edition 10.1d. Get back when I have some more details but here is the output

# ** Fatal: (SIGSEGV) Bad handle or reference.
#    Time: 0 ps  Iteration: 6  Process: /tb_card_shuffler/scoreboard/main File:
D:/Programming/github/vunit/examples/com/vunit_out/preprocessed/tb_shuffler_lib/
scoreboard.vhd
# Fatal error in Process main at D:/Programming/github/vunit/examples/com/vunit_
out/preprocessed/tb_shuffler_lib/scoreboard.vhd line 61
#
# HDL call sequence:
# Stopped at D:/Programming/github/vunit/examples/com/vunit_out/preprocessed/tb_
shuffler_lib/scoreboard.vhd 61 Process main
#
#
# Stack trace result from 'tb' command
#  D:/Programming/github/vunit/examples/com/vunit_out/preprocessed/tb_shuffler_l
ib/scoreboard.vhd 61 [address ff2c39c1] Process main
#
#
# Surrounding code from 'see' command
#   56 :     self := create("scoreboard");
#   57 :     subscribe(self, find("test runner"), status);
#   58 :     subscribe(self, find("monitor"), status);
#   59 :     loop
#   60 :       receive(net, self, message);
# ->61 :       case get_msg_type(message.payload.all) is
#   62 :         when reset_shuffler =>
#   63 :           n_received         := 0;
#   64 :           received_checksum  := (others => '0'); loaded_checksum := (ot
hers => '0');
#   65 :           received_checksum2 := (others => '0'); loaded_checksum2 := (o
thers => '0');
#
LarsAsplund commented 9 years ago

@kraigher The wait statement in the wait_for_messages procedure in com.vhd is timing out immediately despite receive_timeout = 9223372036854775807 ps. The fatal error is because my tb doesn't check message.status and tries to decode a null payload. A negative timeout doesn't help us, we still need a very large timeout for the wait statement. I guess 1 hour would be large enough and it wouldn't overflow GHDL. Right?

procedure wait_for_messages (
  signal net               : in  network_t;
  constant receiver        : in  actor_t;
  variable status          : out com_status_t;
  constant receive_timeout : in  time := max_timeout_c) is
begin
  if messenger.deferred(receiver) then
    status := deferred_receiver_error;
  else
    status := ok;
    if not messenger.has_messages(receiver) then
      wait on net until messenger.has_messages(receiver) for receive_timeout;
      if not messenger.has_messages(receiver) then
        status := timeout;
      end if;
    end if;
  end if;
end procedure wait_for_messages;
kraigher commented 9 years ago

@LarsAsplund The idea was not to simply pass the negative time onto the wait statement but rather to have an if statement surrounding the wait statement where if the timeout was negative the for timeout part would be removed from the wait statement. Anyway 1 hr works for GHDL. That is what i have pushed to the branch.

kraigher commented 9 years ago

Seems (15 downto 4 => "Hello world!") is a vhdl 2008 construct which GHDL does not support yet. https://sourceforge.net/p/ghdl-updates/tickets/72/

I will rewrite the code.

kraigher commented 9 years ago

@LarsAsplund It seems time'low in GHDL is -2^63 instead of -2^63+1 as modelsim. You claim in a comment that the standard says it must be the symmetric range [-2^63+1, 2^63-1]. I cannot find this claim in the 2008 LRM.

The only predefined physical type is type TIME. The range of TIME is implementation dependent, but it is guaranteed to include the range –2147483647 to +2147483647.

Thus the encoding method does not work for time'low since the sign is flipped when it is negative.

LarsAsplund commented 9 years ago

@kraigher Will your handling of the unsupported ranges affect the codecs ability to maintain the array range(s)?

My comment is badly phrased. What i meant was that Modelsim's position with a fixed integer range and a variable resolution is ok.

kraigher commented 9 years ago

@LarsAsplund missing ticket 72 only affects the test bench: 22b0ef97dce866178e5c8e900bf40a70daeadea0

LarsAsplund commented 9 years ago

@kraigher Ok, I don't mind those changes.

kraigher commented 9 years ago

@LarsAsplund Found some more violations which caused GHDL errors and ModelSim warnings: e5810094777165a1ae6f8e8e343971ebc7b0935e Using for example bit'value("0") is not valid it should be bit'value("'0'").

kraigher commented 9 years ago

@LarsAsplund There are failures with GHDL in com_string.vhd when data is range 3 downto -3:

  function to_detailed_string (
    constant data : ufixed)
    return string is
    variable unsigned_data : ieee.numeric_std.unsigned(data'length - 1 downto 0) := ieee.numeric_std.unsigned(data);
  begin

Modelsim also fails though not always:

    constant data : ufixed(3 downto -3) := to_ufixed(6.5, 3, -3);
    constant unsigned_data1 : ieee.numeric_std.unsigned(data'length - 1 downto 0) := ieee.numeric_std.unsigned(to_ufixed(6.5, 3, -3)); -- Works
    constant unsigned_data2 : ieee.numeric_std.unsigned(data'length - 1 downto 0) := ieee.numeric_std.unsigned(data); -- Fails
** Error: bug.vhd(16): (vcom-1167) Index value -3 (of type std.STANDARD.NATURAL) is out of range 0 to 2147483647.
kraigher commented 9 years ago

I fixed the above with: fe2d854a4ecd0e7959c9f7950416399c2926ed81

kraigher commented 9 years ago

Now the only failure in the com acceptance test is the encoding/decoding of time'low.