add code coverage support for ghdl

[LudvigVidlid]

This adds:

• Handle for letting the user know if the version of ghdl supports coverage (i.e. if it uses the gcc back-end)
• Puts the compile-time gcno with the compiled .o-files, and run-time gcda folders within the output folder for each test

Each test case produces a separate .gcda file, and it is then left to the user to merge them as desired. This is in line with what is desired in issue #437.

This change will break the workflow of those who expect the output to just appear in the default location. For compilation these users would currently have problems with the gcno files not being updated at recompile (which is the main reason for this change). For simulation, maybe I should change the behaviour so that vunit only controls the run-time .gcda if the "enable_coverage" flag is set?

Feel free to bash on the code, I'm not a Python wizard and think it needs some feedback :-)

[LudvigVidlid]

The "36-unit" tests fails, and I don't understand why.

The failures occurred due to me putting the check_output function within the SimulatorInterface class, to make it accessible when running its _compile_source_file from within GHDLInterface. It would probably be better if that change could be avoided somehow.

[eine]

Hi @ludli505! Incidentally, some weeks ago I tried to extend the existing coverage example, which is currently supported for non-GHDL simulators only. My motivation and work-in-progress have some subtle differences with yours, hence please bear with me:

• As discussed in Added ghdl/vunit:* images for CI environments ghdl/docker#11, a user requested some coverage related packages to be added to ghdl/vunit:gcc* images. Precisely, lcov and gcovr.
• Since this repo contains an example about how to use code coverage with other simulators, I modified it to use GHDL (see Added ghdl/vunit:* images for CI environments ghdl/docker#11 (comment)), so I could test docker images.
• Then, I tried to improve it, in order to have the test executed as part of the regular test suite. The work in progress is in branch ghdl-coverage of my fork: master...eine:ghdl-coverage

Overall, I'd be glad if you could have a look at that WIP. Nonetheless, it would be good to split the modifications in this PR into multiple smaller commits that are easier to review. One of the targets (to detect if GHDL's backend supports coverage) is common to both implementations; while moving the results is considered here only.

Regarding the issue with modifying the mock, I think a better approch would be to handle moving the results in:

vunit/vunit/sim_if/ghdl.py

Lines 275 to 325 in a45f4d4

	def simulate( # pylint: disable=too-many-locals
	self, output_path, test_suite_name, config, elaborate_only
	):
	"""
	Simulate with entity as top level using generics
	"""
	script_path = join(output_path, self.name)
	if not exists(script_path):
	os.makedirs(script_path)
	ghdl_e = elaborate_only and config.sim_options.get("ghdl.elab_e", False)
	cmd = self._get_command(config, script_path, ghdl_e)
	if elaborate_only and not ghdl_e:
	cmd += ["--no-run"]
	if self._gtkwave_fmt is not None and not ghdl_e:
	data_file_name = join(script_path, "wave.%s" % self._gtkwave_fmt)
	if exists(data_file_name):
	os.remove(data_file_name)
	if self._gtkwave_fmt == "ghw":
	cmd += ["--wave=%s" % data_file_name]
	elif self._gtkwave_fmt == "vcd":
	cmd += ["--vcd=%s" % data_file_name]
	else:
	data_file_name = None
	status = True
	try:
	proc = Process(cmd)
	proc.consume_output()
	except Process.NonZeroExitCode:
	status = False
	if self._gui and not elaborate_only:
	cmd = ["gtkwave"] + shlex.split(self._gtkwave_args) + [data_file_name]
	init_file = config.sim_options.get(self.name + ".gtkwave_script.gui", None)
	if init_file is not None:
	cmd += ["--script", "{}".format(abspath(init_file))]
	stdout.write("%s\n" % " ".join(cmd))
	subprocess.call(cmd)
	return status

as this seems to be specific to GHDL. Is it expected to be used with other simulators too?

[LudvigVidlid]

Thanks for your feedback! I'll look through your WIP later today, and split into multiple commits.

I've found two issues with my changes:

Problem 1
Creating one output .gcda per test and source-file means that merging taking insanely long time, in my case ~20 minutes for less than 100 tests. The motivation for creating one output per test case was to give the user as much control as possible, but perhaps it is not worth it.

The alternative is to have gcov merge all the results while running the tests. If the user has specific requirements on which coverage to merge they'd have to make sure to clean any previous test outputs and run the specific tests desired. For me personally it seems better this way than to have the merging be super-slow. Do you agree that I should change it?

Problem 2
If we have a source file axi_pkg.vhd, the resulting .gcda file is axi_pkg.gcda. This causes a conflict when there are multiple files named axi_pkg.vhd (which is the case if using vunit and tsfpga). The solution is to use the -p or -pg flag to get full paths in the filename, but unfortunately it is not accepted by GHDL. It seems like the option is commented out in a Makefile when building GHDL with gcc. I'll check if this can be changed.

[qarlosalberto]

@ludli505 it's really interesting, I use code coverage in GHDL regularly. And we support it in @TerosTechnology

https://youtu.be/tgr1KGIitIQ?t=170

I have the same problem that you with the output folders. If I could choose (regardless of performance):

1. One output .gcda per test.
2. Merged .gcda
3. Code coverage % per test in output result file.
4. Code coverage % total with merged results in output result file.

Actually I do --clean when I want to regenerate the code coverage. But it's really slow when I have to compile a lot of libraries...

[LudvigVidlid]

@eine:

• I've copied your commit regarding the supports_coverage function.
• Regarding the mock: the results are already moved in the chunk you refer to. The compile-time .gcno are moved in an overloaded version of compile_output. I've cleaned this up a bit now.
• Messing around with environment variable seems dangerous. I'd be grateful if you know whether this
  this is safe or not when running multiple simulation threads.
• It seems like I should add a merge_coverage procedure as well, and expand the example/test as you have done on your branch.

@qarlosalberto:

• The slow merging was due to my stupid. Merging with gcov_tool seems fast.
• I'd be happy to implement a merge_coverage function, when I know these changes are on the right track :-)

[LudvigVidlid]

I've now added merge_coverage for ghdl. It puts merges the gcda files from all tests and adds them together with the gcno files in the correct folder, so that they can be easily handled with gcovr

[LukasVik]

To follow the convention in e.g. modelsim.py:258 this option should be moved to the ghdl interface class and be enabled based on the enable_coverage sim option.

Maybe ghdl.a_flags should be handled the same way. For riviera and modelsim the user has to manually set compile option since they have to make a decision on what coverage to enable. For ghdl that choice is not there, so ghdl.a_flags could be set in the class solely based on enable_coverage sim option.

[LudvigVidlid]

This was my original idea, but I opted not to do it as it seems like the enable_coverage sim option should not affect compilation.

But it would be more convenient, I'll change it.

[LudvigVidlid]

The sim option -Wl,-lgcov, has now been moved into ghdl.py

The compilation function only works on the SourceFile object, which is neat but means that the enable_coverage simulation option is not conveniently available. I've added a new enable_coverage compilation option. This also means that coverage may be enabled only for the desired files. Proof of concept on my tsfpga branch

[LukasVik]

I have reviewed it again and think it looks good to merge. It works well, both in the coverage example as well as in the tsfpga code base.

[LudvigVidlid]

Thanks @LukasVik .

@eine, could you have a look at the changes?

[eine]

Currently example coverage is not added to acceptance tests. Would you mind adding eine@3e16fc5#diff-ec10270706c0b42d3ef658d092217a3a to this PR?

It seems that Problem 1 from #627 (comment) is not an issue anymore, is it? What about Problem 2?

Overall, I think it is good to merge (after these minor issues are fixed). However, I am unsure about how to handle backwards compatibility. @LarsAsplund, @kraigher, as commented above, "this change will break the workflow of those who expect the output to just appear in the default location". Should we wait to merge this when we bump to v5.0.0 or is it ok to include it in v4.4.0?

[LudvigVidlid]

@eine, thanks for your review. I've updated according to your comments.

Currently example coverage is not added to acceptance tests. Would you mind adding eine/vunit@3e16fc5#diff-ec10270706c0b42d3ef658d092217a3a to this PR?

Done as well

It seems that Problem 1 from #627 (comment) is not an issue anymore, is it? What about Problem 2?

That problem still exists. I've not looked into if this can be solved in GHDL. I consider it a separate problem.

Overall, I think it is good to merge (after these minor issues are fixed). However, I am unsure about how to handle backwards compatibility. @LarsAsplund, @kraigher, as commented above, "this change will break the workflow of those who expect the output to just appear in the default location". Should we wait to merge this when we bump to v5.0.0 or is it ok to include it in v4.4.0?

After reading your comment, I realized that only one additional check for enable_coverage flag in _compile_source_file of ghdl.py was needed for backwards compatibility. I've added this now.

[eine]

@ludli505, so quick! Thanks!

That problem still exists. I've not looked into if this can be solved in GHDL. I consider it a separate problem.

Fair enough!

After reading your comment, I realized that only one additional check for enable_coverage flag in _compile_source_file of ghdl.py was needed for backwards compatibility. I've added this now.

Can you please add a very brief description about this? Ideally, something should be added to the docs (in http://vunit.github.io/py/opts.html#simulation-options and/or vunit.github.io/py/vunit.html#vunit.ui.results.Results.merge_coverage). However, I think it is ok with adding a description to the commit message, at least.

[LudvigVidlid]

My last commit adds description of the new enable_coverage flags, and updated the description for the old one in py/opts.html.

Is that the information you're looking for or did I miss something?

Cheers!

[eine]

Is that the information you're looking for or did I miss something?

That's exactly what I was looking for. Thanks!