Code coverage issues

[Blebowski]

Hello,

I am trying code coverage of NVC on a small design I have made (SHA 512 unit + simple directed TB), see the design attached.

I am running into several issues/bugs/improvements, I will try to comment:

1. The coverage is generated "per-file" instead of "per-instance". IMHO per-instance is much more important. "Per-file" can be generated as sub-set of per-instance. Do I understand it correctly that cover tags are held for each instance in elaborated tree, therefore they are held "per-instance", and merging "per-file" is done only at the end? Or does the loc attribute of cover_tag_t correspond to a file, and if I instantiate and entity multiple times, I will not get separate cover tags?

2. In the following FSM, I have two problems: case is reported as uncovered, but each state of FSM was visited. Also, if in S_HSH_IDLE is not reported as covered despite the fact that both its conditions were met. Both of these fact are visible from waves if simulation is executed.

3. In the following combo process (output decoder of an FSM), I can see multiple "default" assignments to a signal as uncovered, despite the fact they are for sure covered.

4. Another case of process in which all lines / statements / conditions / branches (true, else, implicit else) are covered, but NVC reports them as "uncovered":

5. Coverage on "with-select" (I don't know about "when-else") does not actually generate coverage "bins" for each possible choice in the multiplexor, therefore not giving any information from the coverage analysis:

I was wondering whether you would accept a PR if I started to work on the code coverage, and whether you would provide me some guidance ? Some months ago I was asking a bit about PSL. I understand that you have the project as hobby, and it is much more fun to write the code by yourself, OTOH no project of such complexity such as HDL simulator can be whole written by a single person :)

I would be happy to know abut future directions of NVC, since it seems you are putting a lot of effort into it in last weeks. I am eager for the FOSS comunity to finally build a simulator which would be comparable to "big-3" in following: VHDL, Verilog, SV, coverage (statement, expression, toggle), Integrated waveform viewer, SDF back-annotation of GLS, full-UVM support. I see pieces (verilator, GHDL, NVC, slang, UHDM), but not an integrated solution. For ASIC projects this is very problematic, and does not allow using anything for sign-off, since each project has at least one IP core in VHDL one in Verilog (mixed language needed), and GLS is always needed... coverage_example.tar.gz

[nickg]

1. The coverage is generated "per-file" instead of "per-instance". IMHO per-instance is much more important. "Per-file" can be generated as sub-set of per-instance. Do I understand it correctly that cover tags are held for each instance in elaborated tree, therefore they are held "per-instance", and merging "per-file" is done only at the end? Or does the loc attribute of cover_tag_t correspond to a file, and if I instantiate and entity multiple times, I will not get separate cover tags?

The tagging is per instance. That happens for free because NVC fully expands the design at elaboration time (you can see that if you do export NVC_ELAB_VERBOSE=1). The coverage report then collapses it back to per-file by accumulating counts in the cover_file_t and cover_line_t structures.

I was wondering whether you would accept a PR if I started to work on the code coverage, and whether you would provide me some guidance ? Some months ago I was asking a bit about PSL. I understand that you have the project as hobby, and it is much more fun to write the code by yourself, OTOH no project of such complexity such as HDL simulator can be whole written by a single person :)

I would be happy to know abut future directions of NVC, since it seems you are putting a lot of effort into it in last weeks. I am eager for the FOSS comunity to finally build a simulator which would be comparable to "big-3" in following: VHDL, Verilog, SV, coverage (statement, expression, toggle), Integrated waveform viewer, SDF back-annotation of GLS, full-UVM support. I see pieces (verilator, GHDL, NVC, slang, UHDM), but not an integrated solution. For ASIC projects this is very problematic, and does not allow using anything for sign-off, since each project has at least one IP core in VHDL one in Verilog (mixed language needed), and GLS is always needed...

My immediate focus is improving the VHDL-2008 support to the point where it can run OSVVM, UVVM, and VUnit. It seems these are pretty much required to be a useful simulator today.

After that the three big things I want to do are:

• Multi-threaded simulation.
• Verilog support (using iverilog or another existing tool as a frontend).
• PSL support (I know you asked about this before).

I've spent a lot of time thinking about these and starting writing some code already but nothing public yet.

I'm not completely against accepting patches from other people but as you know this is just an evening/weekend thing for me that I have to balance with my regular full-time job. So I'm very nervous about the coordination and communication overhead becoming too high and then getting burnt out.

That said I'm not working on the coverage part at the moment and I don't have any plans to, so if you want to try improving it then please go ahead :-). I wrote that a long time ago just as a proof-of-concept. I'm not sure if anyone really used it so it's not surprising there's bugs.

[Blebowski]

Hi,

thanks for quick answer, I will start looking into the coverage.

As for the maintenance, I completely understand the fear of burnout in case of overload, so no pressure (despite the fact that pressure is there just by saying that)... Anyway, to the fact this is just weekend thing, the amount of work you did is amazing.

With regards to next directions:

1. Multithreaded simulation - How do you want to approach this? Scheduling events from event queue in parallel? Or partitioning the design based on hierarchy into "separately" executed simulations which communicate together?
2. Verilog support - As for the verilog, maybe a good parser for the needs of nvc is: https://github.com/MikePopoloski/slang

[nickg]

1. Multithreaded simulation - How do you want to approach this? Scheduling events from event queue in parallel? Or partitioning the design based on hierarchy into "separately" executed simulations which communicate together?

The former. Partitioning sounds good but I think it's hard to do in practice and if you're aiming to implement the VHDL LRM exactly (which NVC does) that basically mandates syncing up at the end of every delta cycle anyway, which means the partitions can't really execute separately.

2. Verilog support - As for the verilog, maybe a good parser for the needs of nvc is: https://github.com/MikePopoloski/slang

Thanks, it looks like quite a good fit. I was looking at iverilog before because I did some work on it a long time ago and it has a nice API for adding plug-in backends.

[Blebowski]

Hi, thanks for the lower-case names. I am looking into the coverage and I would like to do some extensions. It is going to take some time though (I am EE HW engineer so I have only rudimentary knowledge about compilers). So far I have identified following possible extensions:

1. For T_IF and T_CASE, I would add a separate coverage tag for each condition, for IFs also for "implicit else". I find "implicit else" useful since it allows you to optimize FSMs (remove un-needed conditions).
2. I would add the "hit-counter" value into the "covdb" file. At elaboration time, I write the value to zero, during "clean-up" post simulation I don't dump the report, but store only the resulting hit counts from 'cover_stmts' and 'cover_conds'.
3. I would like to implement new binary "nvc_cov" (or maybe just "-c" command to nvc). You could pass multiple "covdb" files and into would generate merged coverage database. I find this approach the most practical (other simulators use the same approach), since typically ASIC/FPGA designs have single TB with fixed hierarchy and many tests.
4. HTML report would be generated again as command of "nvc_cov". What do you think?

[nickg]

1. For T_IF and T_CASE, I would add a separate coverage tag for each condition, for IFs also for "implicit else". I find "implicit else" useful since it allows you to optimize FSMs (remove un-needed conditions).

2. I would add the "hit-counter" value into the "covdb" file. At elaboration time, I write the value to zero, during "clean-up" post simulation I don't dump the report, but store only the resulting hit counts from 'cover_stmts' and 'cover_conds'.

This sounds ok. Another thing you might like to do is change the way the tags are allocated. At the moment it does one pass over the AST allocating tags before code generation (cover_tag). But instead it might be better to allocate the tags on-demand when the code for each individual if/case/etc. is generated (lower_cond_coverage which calls cover_is_tagged at the moment). The problem with the current system is that an if-statement that appears in multiple instances might only get tagged once as the AST shares structure where possible.

3. I would like to implement new binary "nvc_cov" (or maybe just "-c" command to nvc). You could pass multiple "covdb" files and into would generate merged coverage database. I find this approach the most practical (other simulators use the same approach), since typically ASIC/FPGA designs have single TB with fixed hierarchy and many tests.

4. HTML report would be generated again as command of "nvc_cov".
   What do you think?

I'd prefer to keep only one binary for end users, so nvc -c sounds better.

[Blebowski]

Thanks, I did not realize that. Btw. I have seen that "simp" pass converts all "VHDL specific" parallel statements to sequential statements. So is it safe to assume that T_IF, T_CASE, T_FOR, T_WHILE are the only statements which diverge the code flow? Or is there something else?

[nickg]

Btw. I have seen that "simp" pass converts all "VHDL specific" parallel statements to sequential statements. So is it safe to assume that T_IF, T_CASE, T_FOR, T_WHILE are the only statements which diverge the code flow? Or is there something else?

Yes there should be anything else after analysis. All the concurrent statements get converted to "equivalent processes" according to the rules in the LRM.

[Blebowski]

Hi, I am going through the code, and managed to do at least some basic modifications. I am wondering how to uniquely identify coverable object in the dumped file coverage file. Currently, there is tag, which identifies offset in run-time counters under cover_stmts and cover_conds. This however assumes, that the design hierarchy is always the same.

I am thinking about following use case: Testbench with top level generics. Based on value of generic (e.g. G_TEST_NAME), a procedure from a "test package" is selected and called in TB top process (lets call it test_exec_process). I have seen this approach to VHDL TB in many projects in different companies or OS HW projects.

In such case, if we dump cover_tagging from run of multiple tests (different value of G_TEST_NAME), content of test procedure will affect tag value stored for our DUT (there we want to collect coverage). If we wanted to "merge" coverage databases for DUT from such two simulations, it would be impossible to find tags corresponding to equal statements in VHDL. This leads me to following:

Merging code coverage data from two simulations if cover tagging is incrementally indexed, is possible only if the hierarchies from these two simulations are 100% equal (nothing shaped by generics).

So, I am thinking about how to uniquely identify each cover_tag. IMHO the best way would be its hierarchical path in final design (post-lower pass). If this information is dumped to the coverage database file, then if merging multiple files with part of the hierarchy different, it is still possible to find corresponding statements from multiple coverage databases.

But, to do this, there needs to be an unique hierarchical path to each statement. Currently, if a tree_t has label, label is used as its ident, else line number is used. This is again not unique, since you can have multiple statements on single line (and I have seen CPU designs from "old school" designers with that :D ).

Would it be OK, if I replaced default naming of sequential and parallel statements with an incrementing counter per hierarchical scope? In that case, each statement can be uniquely identified in the hierarchy.

Also, I have noticed that you keep hierarchy path during elaboration as part of elaboration context. When you do elab_stmts, you add new symbol table entry for each parallel statement (hpathf), that is done for both INSTANCE_NAME and PATH_NAME. Would it be ok for you, if I add per-tree attribute with something like tree_inst_name or tree_path_name, set during elab ? Is there some potential performance impact ? AFAIK, the identifiers are already added in the symbol table, so should not be a big deal, or is there something I miss?

With these two changes, I would be able to uniquely identify each statement in the hierarchy.

One thing I still need to figure out is naming of concurrent assign statements, since these are converted to processes in "simp". In such case, resulting process has only single procedural statement in it, so having hierarchical name like: is un-needed (one for process one for statement within it...)

[nickg]

Would it be OK, if I replaced default naming of sequential and parallel statements with an incrementing counter per hierarchical scope? In that case, each statement can be uniquely identified in the hierarchy.

You mean replace the current default name of _lineXX? I think that would be OK but IIRC the 2008 LRM now specifies a way to construct these names so maybe it would be better to adopt that.

Also, I have noticed that you keep hierarchy path during elaboration as part of elaboration context. When you do elab_stmts, you add new symbol table entry for each parallel statement (hpathf), that is done for both INSTANCE_NAME and PATH_NAME. Would it be ok for you, if I add per-tree attribute with something like tree_inst_name or tree_path_name, set during elab ? Is there some potential performance impact ? AFAIK, the identifiers are already added in the symbol table, so should not be a big deal, or is there something I miss?

I'd prefer to store as little information in the tree_t structure as possible to minimise disk/memory usage. In this case I don't think it's necessary and also wouldn't work with something like:

entity sub is
end entity;

architecture a of sub is
    signal s : integer;
begin
    p: process (s) is
    begin
        if s > 10 then
            -- ....
        end if;
    end process;
end architecture;

entity test is
end entity;

architecture a of test is
begin
    u1: entity work.sub;
    u2: entity work.sub;
end architecture;

After elaboration there is only one T_SIGNAL_DECL instance for S and one T_PROCESS for p, but they are pointed at from multiple locations in the hierarchy. You can see that if you edit dump.c to set #define DUMP_ADDRESS 1 and do NVC_LOWER_VERBOSE=1 nvc -e test. So you can't assign either of these objects a unique instance name.

If you try the suggestion above of generating the tags on-demand in lower_cond_coverage rather than in the tree_visit callback then it's easy to pass the path/instance information in (top_scope->hier is a T_HIER instance which holds the path/instance names).

[Blebowski]

Hi,

yes, I meant replacement of the default value _lineXX. Section 19.4.2 of 2008 LRM explains it, pretty much naming: _PX for processes or anything equivalent to processes, of _LX for loops. I would even extend it for any sequential statement which is also non-loop to something like _SX.

As for the second thing, I see, thanks for example.

[Blebowski]

Hi @nickg,

I am slowly moving forward with the coverage, although I don't have anything really working yet.

I decided to split the coverage into 3 categories for now:

• Statement (same as before, covered if generated code corresponding to it was executed)
• Branch / decision (sort-of like the COVER_COND before, only it will be only for T_IF, T_CASE, T_LOOP etc...). The ones with conds + value will have two bins: Evaluated to True, Evaluated to False (the same as now). The ones with assocs will have only Evaluated to True bin. For each choice there will be a bin.
• Toggle coverage - For each std_logic or std_ulogic signal there will be two bins: 0 -> 1 and 1 -> 0 transitions.

For now, I don't consider "subconditions" as in your original implementation. Later, full expression coverage can be implemented as another type of coverage. I am focusing only on those 3 above.

I am dealing with approach to configuring the coverage and designing waiver file.

So far I have something like: nvc -e --cover=s,t,b to enable collection of different types of coverage. I don't consider "per-hierarchy" selection during dumping.

But, for waiver file, I need text file (typically in other simulators) with something like:

exclude statement <hierarchy>._S0
exclude branch <hierarchy>.B0.TRUE
exclude branch <hierarchy>.B0.FALSE
exclude toggle <hierarchy>.signal_name

Typically, other simulators use Tcl for such files, then it allows to even define the hierarchies as variables. What do you think about using Tcl/Tk for this? Would it be OK for you to introduce such dependency?

IMHO, it has added benefit, that then it is possible to implement simulation via TCL shell, and it is easy user interface if you want to do this like dumping waveforms only during some time of simulation (which is now not possible with current --include=<hier_path>.). E.g. something like:

nvc -r --tcl <top_entity>
%nvc_shell: run 10 ms
%nvc_shell: dump --include <hierarchy_i_want_to_include_in_the_waves_from_10_to_20_ms>
%nvc_shell run 10 ms
%nvc_shell: dump --reset
%nvc_shell: run

[nickg]

exclude branch .B0.TRUE exclude branch .B0.FALSE

Is that syntax with B0, etc. for individual conditions taken from a commercial simulator like ModelSim? TBH I never used that feature before so I'm not aware of it.

Typically, other simulators use Tcl for such files, then it allows to even define the hierarchies as variables. What do you think about using Tcl/Tk for this? Would it be OK for you to introduce such dependency?

Actually there used to be a TCL shell for controlling the simulation but I removed it because I wasn't really maintaining it and I'm pretty sure no one used it:

https://github.com/nickg/nvc/blob/1cb3773073/src/rt/shell.c

I'm not opposed to restoring it. Although perhaps with TCL as an optional dependency.

[Blebowski]

The syntax is just a proposal, its not exactly like in Modelsim or Xcelium, but its similar. I think Xcelium had something like:

exclude -type statement <hierarchy_path>

As for the TCL, I will have a look at the commit you sent. IMHO having TCL shell gives you more flexibility than configuring everything before the start of simulation.

[Blebowski]

I am slowly progressing with the code coverage. I managed to add options for separate coverage types, and emit the coverage during "lower" phase. For now, I have statement coverage pretty much the same as before, and branch coverage for: T_IF, T_CASE, T_LOOP_CONTROL, T_WHILE. Apart from T_CASE they are more-less the same as previously: one bit for true part of, one for false part of on the "control expression". On T_CASE, I emit code for checking each assoc, and also "when others" alternative.

I also added "coverage merging" from multiple "covdb" files into single one, however I don't have report generation yet.

I am thinking about how to implement toggle coverage, e.g. similarly as in: https://www.aldec.com/en/support/resources/documentation/articles/1511

I would like to trace 0->1 and 1->0 toggles on all std_logic (including elements in sub-records or in arrays) internal signals and ports of an instance. Compared to URL above, it will be enough with simple flag stating that the transition occurred, counter for each transition is not necessary. The tricky thing here is the distinction between signal / port and "net". I think I will try to track each signal / port separately despite the fact they can correspond to single net.

However, I am bit lost here. I am trying to understand the interaction between exported functions from rtkern.c and cgen.c. The thing is, that the toggle needs to be checked when the signal / port gets updated. AFAIK, assignment to signal plans the driver update via sched_waveform. Once the waveform is executed in the simulation queue, it results in rt_update_driver which in turn updates the value in rt_update_driving of memory location corresponding to the signal.

Esentially, I would need at this time to execute a comparison with "previous value" and check transitions. However, I don't understand the code enough to add some sort of "callback" into the code which would do it, and set bits in generated coverage data.

If I emit the code for this lower_signal_assign_target, it would be executed at the time when the process in which the signal is driven is executed, but that not correct, since there can be non-zero "after" duration in waveform update.

Do you maybe have some suggestion where to look here? How could I emit code during signal assignment, which would cause that once a signal / port value changes, it calls a callback?

I thought about generating an extra process such as:

process(my_signal)
begin
   if (my_signal != my_signal'last_value) then
       <here lives code which modifies the coverage data>
   end if;
end process

but it seemed like overkill to me...

[nickg]

Do you maybe have some suggestion where to look here? How could I emit code during signal assignment, which would cause that once a signal / port value changes, it calls a callback?

You can do it the same way the waveform dumper does: call rt_set_event_cb for each signal you want to monitor and the callback will fire every time there's an event on that signal. You can get a pointer to the current value with rt_signal_value and for std_logic it'll just be an array of int8_t. There isn't a function to get the last value at the moment but it's trivial:

const void *rt_signal_last_value(rt_signal_t *s)
{
   return s->shared.data + s->shared.size;
}

Otherwise you shouldn't need to modify rtkern.c.

BTW I'm refactoring a lot of this code at the moment. It shouldn't change the functionality but I'm moving things around and cleaning it up which might cause merge/rebase issues.

[Blebowski]

Thanks for explaining, I will give it a try. I noticed lot of JIT stuff moved. My current work is rebased on latest change from yesterday, so for now its fine. I will stick to rebasing often after some changes, so that I don't get some crazy conflicts eventually...

[Blebowski]

HI @nickg, again after some time getting back to the coverage.

I managed to set-up the callbacks, however, I have trouble tracing index of entry in cover_tagging_t for toggle coverage) to rt_signal_t.

I emit toggle coverage tag for each bit of signal with ultimate base type of std_ulogic (including multi-dimensional or nested arrays) during lower phase (elaboration). As part of tag, I dump also the hierarchical path (build during lower with vcode_unit_name).

Then, during start of simulation, I moved rt_reset_coverage after rt_initial, so that run-time structs for signals are already created, and as part of rt_reset_coverage, I can set-up the callbacks. I walk the design (de-facto the same as in wave.c), and I am able to get each signal via rt_find_signal. However, at this point, I have no way how to get tag which was created during lower phase (to find out offset into array of coverage data), so that I can pass it to the callback being registered.

I was thinking about comparing hierarchical paths during walk of the tree with hierarchical path of each coverage tag in cover_tagging (stupid quadratic search), but the path differs (run-time path lacks library name, its not capitalized and separators are ":" instead of "."). During iteration of the tree, I am able to prefix the library name, and build the path correctly, but anyway, this seems to be an overkill solution due to slow performance on large designs...

I was also thinking about using hash table (similarly as in your current implementation of coverage), to get KEY-> VALUE from TREE(DECLARATION)->INDEX, but the I would need to store the hash table between Elab and Run (I store only cover_tagging_t array items). Also, tree_t addresses are not persistent between Elab and Run (correct?). So, even if I linked tree_t of declaration to each cover_tag_t during Elab, dump it into coverage file, then load it again during Run, I can't simply iterate over loaded tags and compare tree_t reference from tag with declaration that I am walking over during callback set-up...

Do you maybe know how it would be best to implement this?

One solution which comes to mind, is extending rt_signal_t (shared part of the structure between Generated code, and written code) with this information, and placing it there via emit_init_signal, then Run-time will be able to get such index. However, I don't want to extend size of structure for each signal just due to coverage information. Plus, it would need to stay there also when toggle coverage is not enabled...

[nickg]

I think the best way is to add a new VCODE_OP_COVER_TOGGLE and a corresponding

void emit_cover_toggle(vcode_reg_t signal, uint32_t tag);

The call would go in lower_sub_signals, the same place we call emit_resolution just after emit_init_signal which creates the signal object. Then you can add a cgen_op_cover_toggle that emits a call to a runtime function __nvc_cover_toggle passing the signal and tag (note I moved these entry points to src/jit/jit-exits.c). Have that call a function x_cover_toggle in cover.c and then you have the rt_signal_t * and tag you need.

void x_cover_toggle(sig_shared_t *ss, uint32_t tag)
{
   rt_signal_t *s = container_of(ss, rt_signal_t, shared);

   // Setup callback here
}

You probably want to rebase your existing changes on the latest master because I moved a lot of things around. Basically the stuff that was in src/rt/rtkern.c has moved to either src/rt/model.c (the code that deals with the actual simulation model) or src/jit/jit-exits.c (the interface with the generated code). I'm trying to decouple the runtime from the codegen a bit to make adding a JIT/interpreter easier.

[nickg]

Another advantage of doing it this way is you don't need to walk over the design to find the signals.

[Blebowski]

Thanks :) I am going to try it. Sorry for all the questions, but compiler development really is not my field, so working with the various parts of code translation is quite abstract for me.

[Blebowski]

I managed to make it work, thanks :)

I setup callback for each "homogenous" signal, and iterate all values in array indices within the callback. That seemed to me the easiest to find out the toggles on each index of array. Btw. my whole progress is at: https://github.com/Blebowski/nvc/tree/code-coverage I did not start looking into the tests yet. Now that I have all types of coverage and merging implemented, I will start with generation of the Coverage report.

Btw. I rebased on your latest master with rt_kern functions moved to model.c. There is one issue though. When running run_sim_cb via jit_with_abort_handler, calling std.finish in TB causes jit_abort to be called. This sets m->force_stop which should softly exit the event queue loop. But, jit_abort at the same time transfers code execution to last set value of abort_env (longjmp), which kills the model_run immediately. The consequence is, that emit_coverage and global event RT_END_OF_SIMULATION are not called. For now I have it fixed by introducing model_finish and always calling it after exiting jit_with_abort_handler(run_sim_cb, &args);.

[nickg]

Btw. I rebased on your latest master with rt_kern functions moved to model.c. There is one issue though. When running run_sim_cb via jit_with_abort_handler, calling std.finish in TB causes jit_abort to be called. This sets m->force_stop which should softly exit the event queue loop. But, jit_abort at the same time transfers code execution to last set value of abort_env (longjmp), which kills the model_run immediately. The consequence is, that emit_coverage and global event RT_END_OF_SIMULATION are not called. For now I have it fixed by introducing model_finish and always calling it after exiting jit_with_abort_handler(run_sim_cb, &args);.

Thanks for reporting this. I fixed it in a slightly different way in d6170a2f.

[Blebowski]

Closing since most of the issues are now not relevant.