Coverage reported too low (lower than with modelsim)

[obruendl]

I am integrating NVC Code Coverage into the Open Logic design flow. It'd be great to have an open-source tool for coverage reporting - so first of all: Thank you for your efforts!

I have set-up a branch to implement this Open Logic feature/nvc-coverage. I pushed everything needed to reproduce the issue reported here.

To reproduce, simply clone the repo and run simulations through the commandline below. Of course VUnit must be installed (5.0.0.dev7). I am currently using NVC version f880ffc (from #1512 ). I notice that NVC reorts significantly lower statement and branch coverage than modelsim - for some entities even no coverage at all.

Example - olo_fix_sample_hold:

** Note: code coverage results for: olo_fix_sample_hold.vhd
** Note:      statement:     N.A.
** Note:      branch:        N.A.
** Note:      toggle:        80.0 % (4/5)
** Note:      expression:    N.A.
** Note:      FSM state:     N.A.
** Note:      functional:    N.A.
** Note:      average:       80.0 % (4/5)

Modelsim reports the following:

=================================================================================
=== File: /home/oli/work/olo/open-logic/src/base/vhdl/olo_base_sample_hold.vhd
=================================================================================
    Enabled Coverage              Bins      Hits    Misses  Coverage
    ----------------              ----      ----    ------  --------
    Branches                         5         5         0   100.00%
    Statements                       4         4         0   100.00%

Reproduction

To reproduce:

1. Navigate to [open-logic-root]/sim
2. Execute python3 run.py --nvc --coverage -p 16
3. Execute python3 run.py --modelsim --coverage
4. Execute 'python3 AnalyzeCoverage.py`

On your disk you now have two files inside the [open-logic-root]/sim folder to compare:

• nvc_coverage.txt (nvc)
• coverage_report.txt (modelsim)

In many cases NVC report code uncovered that I am pretty sure really is covered by the test-benches.

Could you look into it? I think Open Logic serves as a good test-load for NVC coverage.

[Blebowski]

Hi @obruendl,

the author of NVC is currently taking break from processing / responding to community issues / pull requests.
More on that in: #1515

I will try to look into your issue in the nearest days, and see if I can fix it.

[obruendl]

@Blebowski Thank you for letting me know.

[Blebowski]

Hi @obruendl,

unfortunately, my version of modelsim (the freeware IntelFPGA 20.1) tells me:

# ** Error: This product is not licensed for Code Coverage.

so, I am unable to do the comparison.

But looking at the snippet above, I see you are comparing "per source file" code coverage of 2 different source files: olo_fix_sample_hold.vhd vs. olo_base_sample_hold.vhd. Since I can't run the modelsim, I can for now only compare
what NVC gives me for olo_base_sample_hold.vhd since you provided Modelsim numbers for that file.

There NVC gives me:

** Note: code coverage results for: olo_base_sample_hold.vhd
** Note:      statement:     100.0 % (8/8)
** Note:      branch:        100.0 % (3/3)
** Note:      toggle:        66.7 % (4/6)
** Note:      expression:    100.0 % (2/2)
** Note:      FSM state:     N.A.
** Note:      functional:    N.A.
** Note:      average:       89.5 % (17/19)

There I see NVC has 100 %, the same as Modelsim. The absolute number of bins is different, but different simulators may emit coverage bins differently, there is no standard on how to do it. The toggle metrics is not provided in the Modelsim snippet, so I can't compare those.

Could it be, this is just confusion in comparing different files ?
If not, could you please provide generated HTML reports for both NVC and Modelsim where the mismatch can be seen in detail ?

[obruendl]

Sirry for picking the wrong file. You are right on that - i'll send you the modelsim coverage of the correct file later.

I see dozens of differences (modelsim reports 100% fir everything, NVC not - thats why it is so easy to spot). It definetely is nor just obe file where I looked at the wrong one.

Good news is that my coverage results also come from a free modelsim version, so you should be able to reproduce at zero cost:

My version is

Questa Intel Starter FPGA Edition-64 vsim 2022.4 Simulator 2022.10 Oct 18 2022

[obruendl]

@Blebowski

I have attached the coverage report from Modelsim. You can ignore anything that does not start with "olo_" (those are VUnit files) and anything that ends in "_tb" (those are testbenches, I actually do not care bout the TB coverage).

But all production code open logic files should have 100% statement and 100% branch coverage (like reported by modelsim).

coverage_report.txt

The good thing about Open Logic having 100% statement/branch coverage for all production code is that you actually do not even have to install modelsim to find out where the differences are. Every line reported as uncovered by NVC is a difference (however, of course this does not mean that I am 100% sure modelsim does have zero issues).

I hope I could provide everything needed for you to proceed. If not, let me know what I can provide.

[Blebowski]

OK, I think I know where is the problem. The NVC coverage databases (.ncdb) are being overwritten.

There are multiple simulations (tests) for each testbench top. By default NVC dumps the simulation data to NCDB
named equally as the top level entity. If the file already exists, it overwrites the file, it does not "append the data".
After running your NVC regression, I see all the NCDB files in sim folder. There is much less NCDB files than
the tests that were executed, which confirms the overwriting.

So, the coverage results you see, are only from one test / configuration of generics for each testbench (the one that started the last).

It is up to the user to satisfy that NCDBs will not overwrite.
You can achieve this by using --cover-file switch. You will need to iterate all configurations (test runs)
created by Vunit and append an elab option with unique NCDB path. So you will end with sth like: olo_tb.olo_intf_sync_tb.SyncStages_g_2_RstLevel_g_0_ResetValue.ncdb instead of olo_intf_sync_tb-sim.ncdb.
Then you will get unique coverage database per each simulation run. When you merge the results
together, you should see numbers closer to Modelsim.

It is bit more work to set this up in NVC, since Modelsim likely merges into the coverage database
with each run incrementally. In NVC, user has to take care of it manually. This could be implemented in
NVC too, but IMHO it is not worth the complexity since frameworks like Vunit allow iterating across all
tests / simulations and appending test specific elab options very easily. I think we should add a warning
when NVC writes new database sth like:

Overwriting existing blabla.ncdb, you may possibly loose some coverage data. 
Consider using --cover-file to get per simulatin unique coverage database

[obruendl]

@Blebowski

OK, I implemented a workaround for the NCDB overwriting in my branch. Most files now look fine but a very few still have coverage deviations from Modelsim (amongst others two CIC filters, just that you know what to look out for).

Could you check those remaining differences?

[Blebowski]

Both CIC filters seems to have uncovered loops whose upper threshold is determined by generic.
Also, both are in process, which if executed (e.g. statements above these loops are covered),
the loops must be executed.

I will need to examine this more deeply. My bet is this is due to some optimization.

[obruendl]

@Blebowski

From a logical perspective, I am sure those loops are executed - and modelsim agrees with me.

And yes, of course coverage and optimizations are a difficult combination. I fully understand that this is non-trivial and I am very happy that you support me this actively. Thank you for this.

So: Take yourself the time and investigate. You don't need to make it a night shift - I am not blocked. It's simply that I will release NVC coverage only once the reports are identical to modelsim. But if this takes a day more or less does not matter much to me - next release will be around end of may.

Overall: Thank you for the continued support. You guys are doing a great job with NVC!

[Blebowski]

Btw. you can improve simulation performance, and also reduce number of thinks to go through if you don't collect code coverage for TB. You can do that by "coverage specification file" (--cover-spec). I adapted your run.py locally so that I don't need to filter by eyes the "tb" entities in the report:

              config.set_sim_option("nvc.elab_flags", ["--cover=statement,branch", f"--cover-file={tb.name}_{uuid.uuid4()}.ncdb", "--cover-spec=nvc_cover_spec.txt"])

With coverage spec file being:

+hierarchy OLO_TB.OLO_AXI_LITE_SLAVE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_AXI_MASTER_FULL_TB.I_DUT.*
+hierarchy OLO_TB.OLO_AXI_MASTER_SIMPLE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_AXI_PL_STAGE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_ARB_PRIO_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_ARB_RR_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_ARB_WRR_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CAM_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_BITS_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_HANDSHAKE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_N2XN_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_PULSE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_RESET_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_SIMPLE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_STATUS_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CC_XN2N_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CRC_APPEND_CHECK_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CRC_APPEND_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CRC_BE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CRC_CHECK_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CRC_INITVAL_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_CRC_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_DECODE_FIRSTBIT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_DELAY_CFG_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_DELAY_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_DYN_SFT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_FIFO_ASYNC_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_FIFO_PACKET_HS_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_FIFO_PACKET_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_FIFO_SYNC_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_FLOWCTRL_HANDLER_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_LATENCY_COMP_LOLEVEL_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_LATENCY_COMP_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_PKG_ARRAY_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_PKG_LOGIC_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_PKG_MATH_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_PKG_STRING_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_PL_STAGE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_PRBS4_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_RAM_SDP_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_RAM_SP_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_RAM_TDP_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_RATE_LIMIT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_RESET_GEN_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_SAMPLE_HOLD_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_STROBE_DIV_BACKPRESSONLY_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_STROBE_DIV_FIXRATIO_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_STROBE_DIV_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_STROBE_GEN_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_TDM_MUX_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_WCONV_N2M_78_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_WCONV_N2M_BE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_WCONV_N2M_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_WCONV_N2XN_TB.I_DUT.*
+hierarchy OLO_TB.OLO_BASE_WCONV_XN2N_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_ABS_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_ADDSUB_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_ADD_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_BIN_DIV_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_CIC_DEC_PAR_TDM_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_CIC_DEC_TDM_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_COMPARE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_CORDIC_ROT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_CORDIC_VECT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_FROM_REAL_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_LIMIT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_MULT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_NEG_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_PKG_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_PKG_WRITER_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_RESIZE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_ROUND_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_SAMPLE_HOLD_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_SATURATE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_SIM_CHECKER_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_SIM_STIMULI_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_SUB_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_TO_REAL_TB.I_DUT.*
+hierarchy OLO_TB.OLO_FIX_VC_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_CLK_MEAS_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_DEBOUNCE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_I2C_MASTER_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_SPI_MASTER_FIXSIZE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_SPI_MASTER_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_SPI_SLAVE_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_SYNC_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_UART_CLKDRIFT_TB.I_DUT.*
+hierarchy OLO_TB.OLO_INTF_UART_TB.I_DUT.*

Unfortunately, the coverage spec filtering works only on "per-hierarchy" basis, not on "per-source file name". Also, you can put wildcard only at one place (at the end to match all coverage items below). So, you can either keep the spec file manually, or again somehow generate by Vunit, since that has list of TB tops, and all DUTs have equal hierarchical name. Should be 1 line in run.py to generate it.

With that, and sorting in the HTML report its possible to easily see design files that have less than 100 %:

[obruendl]

Thank you for the note.

However, I think I prefer not maintaining an additional file I zave to touch for every entity I add.

Performance is not an issue with NVC anyways (take it as compliment...).

And for filtering and displaying coverage the way I want I have my AnalyzeCoverage.py (that also creates the badges fir documentation and fsils CI of sny file is below 95%).

[tasgomes]

Unfortunately, the coverage spec filtering works only on "per-hierarchy" basis, not on "per-source file name".

@Blebowski you can use "per-source file name" using +block, so actually the "entity" name. See below the available options that I am aware of:

# Syntax:
#   +block <entity>              - Include specific design unit (entity name only)
#   -block <entity>              - Exclude specific design unit
#   +hierarchy <library.path.*>  - Include hierarchy
#   -hierarchy <library.path.*>  - Exclude hierarchy